fix(prometheus): require word boundary when detecting HELP/TYPE comments - #304
Open
detail-app[bot] wants to merge 1 commit into
Open
detail-app[bot] wants to merge 1 commit into
detail-app[bot] wants to merge 1 commit into
Conversation
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a focused Prometheus parsing fix that preserves valid metadata handling while ensuring ordinary comments beginning with HELP or TYPE remain in the output. The production change is minimal and supported by parser and filtering regression tests. You can add or adjust custom eligibility rules. Learn more. |
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.
Detail bug report: View on Detail
Fixes ENG-705
Bug
parseCommentLineinsrc/signals/prometheus/line_parser.zigdispatched on a barestd.mem.startsWith(rest, "HELP"/"TYPE")with no word-boundary check, so a#-comment whose first token merely began with those letters (e.g.# HELPFUL info,# TYPES: examples) was misclassified as HELP/TYPE metadata. The Prometheus exposition spec tokenizesHELP/TYPEon whitespace, soHELPFUL/TYPES:are single tokens distinct fromHELP/TYPEand must be plain ignored comments.On the production path this violated
PolicyStreamingFilter's "comments preserved regardless of policy" contract: misclassified comments are stored as metadata rather than forwarded inline, so they were silently dropped from the proxy output when no later kept sample matched the bogus derived metric name (FUL/S:).Fix
Require a word boundary — a space, tab, or end-of-comment — immediately after the
HELP/TYPEtoken before dispatching toparseHelpLine/parseTypeLine. Lines such as# HELPFUL infonow parse as.commentand forward unchanged; well-formed# HELP <name> …/# TYPE <name> <type>(space- or tab-separated) still parse as metadata.Added two focused regression tests:
parseLineunit test covering prefix-colliding tokens, tab-separated metadata, and the bare# HELP/# TYPEparse-error casePolicyStreamingFiltercontract test asserting prefix-colliding comments survive a DROP policy, matching how the bug manifested in productionTesting
zig fmt --check,ziglint,zig build, and the full suite (zig build test --summary allandtask do, including ReleaseSafe) all pass — 521/522 tests passed, 0 failures (1 skipped).edge-prometheusbinary: a local/metricsupstream served the trigger comments (# HELPFUL …,# TYPES: …) interleaved with metrics, proxied under both a no-policy and a DROP^metric_config. After the fix both comments are preserved in their original position, and well-formed# HELP/# TYPEmetadata re-positions before its matching sample; under the DROP policymetric_*is dropped while the comments survive.task signoff:task doandtask build:safepass, buttask test:s3-e2erequires pulling theminio/minioimage, which this sandbox's container registry denies — docker is available andhello-worldpulls, butminio/miniois reproducibly denied. That step covers the unrelated S3-dump module and does not affect this fix, sogh signoffwas not reached.Automatic Fixes PRs can be configured here.