fix(legacy-parity): wire prometheus_additional_labels + loud unconfigured-datasource logs#513
Merged
Merged
Conversation
…ured-datasource logs
Two gaps surfaced migrating a customer from the legacy agent to the Go agent
on their existing (legacy) chart:
- The Go agent reads PROMETHEUS_ADDITIONAL_LABELS, but the chart only wired
globalConfig.prometheus_url/prometheus_headers — so the legacy
prometheus_additional_labels map (e.g. {k8s_cluster: ...}) was dropped. Wire
it, converting the YAML map to the key=value,key=value form the runner parses.
- Missing config failed silently: 'Prometheus disabled' was only inferable from
the absence of a 'prometheus enabled' line. Add explicit logs:
* WARN when PROMETHEUS_URL is unset (names the common cause: image-tag-only
migration on a chart that never set the env)
* INFO 'logs provider selected' naming the winner, so a masked provider
(stray ELASTICSEARCH_URL hiding SigNoz) is a one-line grep
Chart bumped to 0.1.8.
Contributor
|
📦 Image Tags Updated |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for configuring additional Prometheus labels in the Helm chart, adds explicit warning logs when Prometheus is disabled, and logs the selected logs provider at startup. Feedback highlights opportunities to improve robustness, such as using %v in Helm templates to handle non-string label values, replacing non-ASCII characters in log messages to avoid encoding issues, and adding a defensive nil check in selectedLogsProvider.
- prometheus_additional_labels: printf %v so non-string values (ints/bools) render cleanly instead of %!s(int=...) - drop em-dash from the prometheus-disabled warn for SIEM/log-parser safety
Contributor
|
📦 Image Tags Updated |
RamanKharchee
approved these changes
Jul 9, 2026
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
2 participants
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.
Why
Diagnosing a customer migrating from the legacy (Robusta) agent to the Go agent surfaced two gaps that made a simple misconfig take far too long to find. The customer bumped only
runner.image.tagto the Go image on their legacy chart, which routesglobalConfig.prometheus_urlinto the Robustaglobal_configConfigMap (what the Python agent read) — never into thePROMETHEUS_URLenv var the Go agent reads. Result: SigNoz Connected, Prometheus Disconnected, with correct values and a valid token.What
1. Chart: wire
globalConfig.prometheus_additional_labels.The Go agent reads
PROMETHEUS_ADDITIONAL_LABELS(main.go), but the chart only wiredprometheus_url/prometheus_headers— so the legacy multi-cluster label map (e.g.{k8s_cluster: aws-otr-eks-staging}) was silently dropped even on a clean migration. Now converted from the YAML map to thekey=value,key=valueform the runner parses. Verified viahelm template:2. Agent: make "not configured" loud instead of silent.
WARN prometheus disabled: PROMETHEUS_URL not set and none autodiscovered …— names the common cause (image-tag-only migration on a chart that never sets the env). Previously this was only inferable from the absence of aprometheus enabledline.INFO logs provider selected provider=… url=…— logs-provider selection is priority-ordered (pinot → ES → signoz → loki), so a configured provider can be masked by a higher-priority one (a strayELASTICSEARCH_URLhiding SigNoz). Making the winner explicit turns "I configured X but see Y" into a one-line grep.Tests
TestSelectedLogsProvider_Precedence— keeps the startup summary in sync with the prober (incl. stray-ES-doesn't-mask-SigNoz).helm templaterenders the label env only when the map is set (empty map → nothing).go build,go vet,go test ./..., gofmt all pass.Chart bumped to 0.1.8.
Note
This helps future migrations onto the new chart. The immediate customer is on the legacy chart and needs to move to the new chart (or set the env explicitly) — a chart migration, not an image-tag bump. That process gap is called out for follow-up.