Repository navigation
fix(exporter): omit phpfpm_pod on statically configured targets - #22
Merged
Merged
Conversation
Only Kubernetes auto-tracking has a pod name to put in phpfpm_pod. Static --phpfpm.scrape-uri targets published phpfpm_pod="" on every series, which is noise on VM deployments: Prometheus treats an empty label value as equivalent to the label being absent, so it conveyed nothing. The label set now follows the discovery mode. NewExporter takes options and internal/server passes phpfpm.WithPodLabel() only under --k8s.autotracking. The choice has to be per exporter rather than per pool, because client_golang refuses two descriptors that share a metric name but disagree on label names. That is safe because the server runs in exactly one discovery mode. Kubernetes output is unchanged. Because an empty label already equalled an absent one, dropping it does not change series identity for existing VM targets either. Label names are now constants, since two label-set families would otherwise repeat each string. The grafana/ dashboard selects on metric names only and never on phpfpm_pod, so it is unaffected; AGENTS.md claimed otherwise and is corrected.
The static half was covered, so nothing would have caught auto-tracking silently losing phpfpm_pod, which is the same wiring bug in reverse and would have hidden which pod a series came from.
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.
Problem
On VM deployments every series carried an empty pod label:
Only Kubernetes auto-tracking has a pod name to put there.
pm.Add(uri, "")on the static path left it empty, but the label was still in the descriptor, so it was emitted regardless.Prometheus states that "labels with an empty label value are considered equivalent to labels that do not exist", so the label conveyed nothing — it was noise in the exposition output for every non-Kubernetes deployment.
Fix
The label set now follows the discovery mode.
NewExportertakes options, andinternal/serverpassesphpfpm.WithPodLabel()only when--k8s.autotrackingis set.The choice has to be made per exporter, not per pool: client_golang rejects "descriptors reported by collector have inconsistent label names or help strings for the same fully-qualified name" (
registry.go:336), so one process cannot mix both label sets. That is fine, becauseserverruns in exactly one discovery mode.Impact
phpfpm_podis still emitted and populated.grafana/dashboard is unaffected. It selects on metric names only and never referencesphpfpm_pod— I checked all 11phpfpm_*references inkubernetes-php-fpm.json.AGENTS.mdimplied the dashboard keys off these labels; that is corrected.Why not rename it to something environment-agnostic
Considered and rejected. On a VM there is no value for such a label that is not already available: the host is Prometheus's own
instance, the pool ispool, and the socket isscrape_uri. A rename would produce a label that is redundant on VMs while remaining Kubernetes-specific in practice, and it would change the identity of every existing Kubernetes series.Dropping the label entirely was also rejected: with one exporter per namespace fanning out to many pods, Prometheus's scrape target is the exporter, so relabeling cannot recover pod identity. The exporter has to emit it.
Tests
phpfpm_podat all for static targetsprometheus.Registrygathers cleanly in both modes, which is what proves the descriptors and the metric values agree/metricsassertion over HTTP for the static casenewExportermaps the discovery mode to the label set in both directions, so auto-tracking silently losing the label is caught tooVerification
golangci-lint(v2.12.2, as CI pins): 5 issues, unchanged from master, none new. Label names became constants because two label-set families would otherwise repeat each string past thegoconstthreshold.Also confirmed by running the built binary before and after, which is how the bug was reproduced in the first place.