metrics: fix application_instances_per_channel disagreeing with UI instance counts - #1580
Draft
riyacore404 wants to merge 1 commit into
Draft
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes the nebraska_application_instances_per_channel Prometheus gauge to align with UI instance counts by making the underlying query consistent with the rest of the “active instance” logic and by preventing instances from being dropped/merged incorrectly.
Changes:
- Add an activity-window filter (
last_check_for_updateswithinvalidityInterval) to stop counting stale instances. - Switch to
LEFT JOIN+COALESCEso instances in groups without a channel are still counted (in an explicit “no channel” bucket). - Add
archto the grouping and metric labels to avoid merging same-named channels across architectures, and update unit tests accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| backend/pkg/metrics/metrics.go | Adds arch label handling when exporting the gauge. |
| backend/pkg/api/types/metrics.go | Extends the metric row type with an Arch field (with -1 sentinel for no-channel). |
| backend/pkg/api/metrics_test.go | Adds/updates tests to cover stale-instance filtering, channel-less groups, and arch separation. |
| backend/pkg/api/internal/dbreads/metrics.go | Rewrites the SQL query to apply the freshness filter, use LEFT JOIN, and group by arch. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
150
to
+154
| for _, metric := range aipcMetrics { | ||
| appInstancePerChannelGaugeMetric.WithLabelValues(metric.ApplicationName, metric.Version, metric.ChannelName).Set(float64(metric.InstancesCount)) | ||
| archLabel := noChannelArchLabel | ||
| if metric.Arch >= 0 { | ||
| archLabel = types.Arch(uint(metric.Arch)).String() | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
application_instances_per_channel metric undercounts/miscounts instances
The
nebraska_application_instances_per_channelgauge had three bugs relativeto the rest of the codebase:
last_check_for_updates, so instances that stopped checking in stayed inthe metric indefinitely instead of aging out like every other
activity-based query (
GetApp,GetApps,GetInstances, etc).joins across
groups/channel, so any instance whose group had nochannel assigned (
groups.channel_idcan be NULL) disappeared from themetric entirely rather than being counted.
by channel name, so two channels sharing a name across architectures
(e.g. "stable" for amd64 and arm64) were folded into a single row.
This fixes all three: adds the same freshness filter used elsewhere, switches
to
LEFT JOINfor groups/channel so channel-less instances land in anexplicit "no channel" bucket instead of vanishing, and adds
archto thegrouping so per-architecture counts are reported separately.
Fixes #1562
How to use
No config or API changes — the fix is internal to how
GetAppInstancesPerChannelMetricscomputes theapplication_instances_per_channelPrometheus gauge. Reviewers can validate by scraping
/metricson aNebraska instance with some inactive instances and/or a channel-less group
and confirming the counts now match what's shown in the instances UI.
Note: this PR adds a new
archlabel to theapplication_instances_per_channelgauge. This is a breaking change for any existing dashboards/alerts that group
by
application/version/channelalone — they'll now see one series per(channel, arch) pair instead of one per channel. Flagging this explicitly in
case maintainers would rather split the label change into a separate PR from
the activity-window/no-channel fixes.
Testing done
Added/updated unit tests in
backend/pkg/api/metrics_test.gocovering allthree bugs, run against
sample_data.sqlvia the local Postgres test service:Output
All targeted tests pass successfully.