Repository navigation
Conversation
A new instance of a summary or histogram was only detected once both _sum and _count of the previous instance had been seen. _sum is optional: summaries exported from Dropwizard timers, e.g. by the Jenkins Prometheus plugin, only have quantiles and _count. For a family with several label sets, the quantiles of the second instance were added to the first one with the _count sample type, and the decoding failed with "not enough samples for summary", dropping the whole payload. Also start a new instance when a quantile follows a _sum or _count line, and when a bucket follows the _sum or _count of an instance with buckets. Signed-off-by: Chris Burr <christopher.burr@cern.ch>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aee043c29c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (sample->type == CMT_DECODE_PROMETHEUS_CONTEXT_SAMPLE_TYPE_BUCKET) { | ||
| bucket_count++; |
There was a problem hiding this comment.
When a valid sum-less histogram emits _count before its buckets (an ordering already represented by tests/data/issue_6534.txt), this new bucket-only sizing accepts it but silently corrupts the implicit bucket. For h_count 2, h_bucket{le="1"} 1, h_bucket{le="+Inf"} 2, the COUNT branch first writes slot 0, the finite bucket overwrites it, and the skipped +Inf sample leaves the final slot at zero, so re-encoding reports _count 2 but +Inf 0. Store the parsed count in the final bucket after the scan rather than at the current bucket index.
AGENTS.md reference: AGENTS.md:L101-L104
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a pre-existing bug but I've fixed it in b5e2486.
| if (bucket_count < 2) { | ||
| return report_error(context, | ||
| CMT_DECODE_PROMETHEUS_SYNTAX_ERROR, | ||
| "not enough buckets for histogram"); | ||
| } |
There was a problem hiding this comment.
When _count is absent but the input has a finite bucket, +Inf, and _sum, this guard now passes because it checks only the number of bucket samples. The later loop skips the +Inf value, and with no COUNT sample both the implicit bucket and histogram count remain zero, so the decoder succeeds and silently changes the exposed total; the parent implementation rejected this three-sample case. Require count_found (or derive and validate count from +Inf) before constructing the histogram.
AGENTS.md reference: AGENTS.md:L101-L106
Useful? React with 👍 / 👎.
The number of buckets of a histogram was computed as the number of samples minus 3, assuming that the "+Inf" bucket, _sum and _count are all present. Without _sum, which is optional, it was one too low: the last finite bucket was taken as the "+Inf" one, and a histogram with a single finite bucket failed with "cmt_histogram_buckets_create_size failed". Count the bucket samples instead, as is already done for summary quantiles, and report a syntax error when there are less than two. A histogram without _count was only rejected because of that sample arithmetic. Reject it explicitly, otherwise it would now be decoded with a count of zero. Signed-off-by: Chris Burr <christopher.burr@cern.ch>
Decode a summary family with several instances that have quantiles and _count but no _sum, as exported for Dropwizard timers by the Jenkins Prometheus plugin, and the same for a histogram family. Check that a histogram without _count is still rejected. Signed-off-by: Chris Burr <christopher.burr@cern.ch>
The value of _count was written to the bucket slot at the current bucket index, assuming that it follows all the buckets of the instance. When _count comes before the buckets, as in the .NET exporter output of Fluent Bit issue 6534 (_sum, _count, then the buckets), that slot was overwritten by the first finite bucket, the "+Inf" sample was skipped, and the "+Inf" bucket was decoded as zero while _count kept its value. Store the value of the "+Inf" bucket sample in its own slot, and only use _count for the histogram count. Signed-off-by: Chris Burr <christopher.burr@cern.ch>
Decode histograms whose _count is written before the buckets, with and without _sum, and check the "+Inf" bucket values. The expected output of the fluent-bit issue 6534 test had every "+Inf" bucket at zero, which was the decoding error itself: they now match the input and _count. Signed-off-by: Chris Burr <christopher.burr@cern.ch>
aee043c to
20269b5
Compare
This PR fixes an issue I found when using Fluent Bit to scrape Prometheus metrics exposed in Jenkins. With:
Below is the LLM generated description for this fix based on the guidelines in the repo:
🤖
_sumis optional for summaries and histograms (OpenMetrics), and Prometheus accepts payloads without it. Summaries exported from Dropwizard timers, e.g. by the Jenkins Prometheus plugin (jenkins_node_builds{node="..."}), only have quantiles and_count:_sumand_counthad been seen. Without_sum, the quantiles of the second instance were added to the first one with the_countsample type, and decoding failed with "not enough samples for summary". That drops the whole scrape, as seen with Fluent Bit 5.1.3prometheus_scrape. A quantile or bucket that follows a_sumor_countnow starts a new instance._sumand_countare present. Without_sum, the last finite bucket was taken as+Inf, or bucket creation failed. Buckets are now counted, as security: harden decoders, encoders and cat against malformed input #308 did for summary quantiles. A histogram without_countwas only rejected because of that arithmetic; it is now rejected explicitly ("missing histogram _count") instead of being decoded with a count of zero._countwas written to the bucket slot at the current index, assuming it comes after all buckets. When_countcomes first (_sum,_count, then buckets, as in the .NET output of Prometheus Scrape not able to parse endpoint fluent-bit#6534), the first finite bucket overwrote it and the+Infbucket was decoded as0. The+Infsample's own value is now used, and_countonly sets the count. The expected output of the existingissue_fluent_bit_6534test had every+Infbucket at0while_countwas 321733, 759, 133, 8 and 618; those five lines now match the input.Regression tests cover summary and histogram families with several instances and no
_sum, a histogram without_count, and histograms with_countbefore the buckets (with and without_sum).Compatibility: a summary instance whose quantiles come after its own
_sum/_countis now split into two instances. Prometheus client libraries write the quantiles first, and the previous code already split there once both_sumand_counthad been seen. Instances without_sumare still encoded with_sum 0, as before. Histograms without_countstay rejected, as on master, but now with an explicit error.Validation:
scripts/agent-verify.shpasses (25/25). Valgrind and ASan are clean for the summary and histogram tests; UBSan reports a misaligned load in the bundled xxhash inlabel_limits, which also happens on master. A real Jenkins payload (2,395 samples) decodes with every sample identical. Commit b5e2486 alone failsissue_fluent_bit_6534until the next commit updates its expected+Infvalues, which keeps fixes and tests in separate commits.