Recover panics from wrapped collectors (match Gather safeCollect)#2029
Open
greymoth-jp wants to merge 2 commits into
Open
Recover panics from wrapped collectors (match Gather safeCollect)#2029greymoth-jp wants to merge 2 commits into
greymoth-jp wants to merge 2 commits into
Conversation
greymoth-jp
force-pushed
the
fix/wrapped-collector-panic-recovery
branch
from
8e3d3d7 to
e4a9aa2
Compare
PR prometheus#1961 added safeCollect to recover panics raised by a collector's Collect method during Registry.Gather, turning a misbehaving collector (issue prometheus#1877) into a recoverable error instead of a process crash. However, a collector registered via WrapRegistererWith is wrapped in a wrappingCollector whose Collect (and Describe) runs the inner collector in a NEW goroutine. A recover only catches panics from its own goroutine, so safeCollect in the gather worker cannot catch a panic raised inside the wrappingCollector goroutine; it still crashes the whole process. WrapRegistererWith is the standard way to add constant labels/prefixes, so this leaves the common path unprotected. Recover the panic inside the wrappingCollector goroutines themselves and surface it as an invalid Metric (Collect) / invalid Desc (Describe), mirroring safeCollect's behaviour. Adds regression tests for both paths. Signed-off-by: greymoth-jp <m.hirakawa07@icloud.com>
Signed-off-by: greymoth-jp <m.hirakawa07@icloud.com>
greymoth-jp
force-pushed
the
fix/wrapped-collector-panic-recovery
branch
from
805301e to
5facb0d
Compare
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
1 participant
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.
wrappingCollector.Collect/.Describe(prometheus/wrap.go) run the inner collector in a child goroutine, so the panic recovery #1961 added toRegistry.Gather(safeCollect) can't catch it — recover is per-goroutine. A collector registered viaWrapRegistererWith(the standard const-label/prefix API) that panics still crashes the whole process.#1961 hardened the unwrapped path; the wrapped path was the symmetric leftover. The fix applies the same recovery to the wrapped collect/describe. Verified both ways: old panics crash the test binary (exit 1); fixed surfaces them as a Gather error. Full
./prometheus/suite passes, 0 regression.