Fix invalid Prometheus metric names from iostat parse glitches - #4711
Open
sophoah wants to merge 1 commit into
Open
Fix invalid Prometheus metric names from iostat parse glitches#4711sophoah wants to merge 1 commit into
sophoah wants to merge 1 commit into
Conversation
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
Prometheus scraping a Nitro node's
/debug/metrics/prometheusendpoint failed entirely with:The node itself was emitting an illegal metric name, not a Prometheus config problem. The
iostat_*metrics come fromutil/iostat/iostat.go, which spawnsiostat -dNxy 1and parses its stdout to register per-device gauges (iostat_<device>_{await,readspersecond,writespersecond}). A parse glitch caused the literal string"99.99"to get captured as a device name. Real devices on the affected host are things likevg0-root,nvme0n1,loop0, never a bare decimal number. That producediostat_99.99_await, which is invalid per the Prometheus exposition format since metric names have to match[a-zA-Z_:][a-zA-Z0-9_:]*and dots aren't allowed.A single invalid metric name aborts the whole scrape, not just that one series. And go-ethereum's metrics registry never unregisters a metric once it's created, so this one bad row permanently broke metrics collection for the node until it was restarted.
There was already some sanitization in place (
strings.ReplaceAll(stat.DeviceName, "-", "_"), added in #2491 for LVM device names likevg0-root), but it only handled hyphens, never dots or anything else. Nothing validated that a parsed token actually looked like a plausible device name before it got registered as a metric.Fix
Two changes in
util/iostat/iostat.go:parseStreamnow skips a row if its parsed device name parses successfully as a float (e.g.99.99), since real device names never do. This stops the bad rows from reaching metric registration in the first place.RegisterAndPopulateMetricsnow sanitizes device names with the existing sharedmetricsutil.CanonicalizeMetricNamehelper (already used elsewhere in the codebase for this) instead of just replacing hyphens, so any other invalid character gets handled too.Testing
Added a
parseStreamtest case confirming a row with a numeric device name (99.99) gets dropped.Added
util/metricsutil/metricsutil_test.go, which didn't exist before, confirmingCanonicalizeMetricNamesanitizes real device names seen in production (vg0-swap,vg0-root,nvme0n1,loop0) the same way the old hyphen-only replacement did. So this change doesn't rename any existing legitimate metric, it only changes behavior for the previously-broken case.go test,go vet, andgofmtall pass forutil/iostatandutil/metricsutil.