Repository navigation
Conversation
wrapDesc passed the Desc through V2.NewDesc even if it carried an error. A Desc created by NewInvalidDesc has a nil variableLabels, and with a non-empty prefix the name passes validation, so NewDesc dereferenced it. As a result, a collector signalling failure with NewInvalidDesc or NewInvalidMetric(NewInvalidDesc(err), err), both documented patterns, crashed the process in Register (inside the Describe goroutine, so it cannot be recovered) or panicked in Gather when registered via WrapRegistererWithPrefix. Return the error of an invalid Desc directly instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Manuel Rüger <manuel@rueg.eu>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The localized fix preserves valid-descriptor behavior and adds regression coverage for both reported failure paths.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes invalid-descriptor handling in Prometheus collector wrappers so prefixed wrappers return errors instead of panicking.
Changes:
- Propagates existing descriptor errors without rebuilding invalid descriptors.
- Adds regression tests for registration and gathering with labels, prefixes, and both.
| File | Description |
|---|---|
| prometheus/wrap.go | Handles invalid descriptors before constructor validation. |
| prometheus/wrap_test.go | Tests error propagation through wrapped registration and gathering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Summary
wrapDescpassed every Desc throughV2.NewDesc, even one that already carried an error. A Desc created byNewInvalidDeschas a nilvariableLabels. Without a prefix, the empty name fails validation early, but with a non-empty prefix the name is valid andNewDescdereferences the nilvariableLabels.As a result, a collector registered via
WrapRegistererWithPrefixthat signals failure with one of the documented patterns crashes:DescribesendingNewInvalidDesc(err):Registerpanics inside theDescribegoroutine, which cannot be recovered, so the process crashes.CollectsendingNewInvalidMetric(NewInvalidDesc(err), err)from an unchecked collector:Gatherpanics instead of returningerr.This change returns the error of an invalid Desc directly instead of building a new Desc from it. The earlier precedence rule ("earlier errors get precedence") is unchanged.
This also matters for #2029, which recovers panics in wrapped collectors by sending
NewInvalidDesc/NewInvalidMetric(NewInvalidDesc(err), err)through the wrapper, i.e. exactly the input that crashes here when a prefix is used.Testing
Added the table-driven
TestWrapInvalidDesc, covering labels, prefix, and both, forRegisterandGather. It crashes with a nil pointer dereference before the fix and passes after it.go test -race ./prometheus/...passes.🤖 Generated with Claude Code