Repository navigation
decode_prometheus: decode summary and histogram instances without _sum #316
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -433,6 +433,7 @@ static int add_metric_histogram(struct cmt_decode_prometheus_context *context) | |
| int ret = 0; | ||
| int i; | ||
| int has_le = CMT_FALSE; | ||
| int has_count = CMT_FALSE; | ||
| size_t bucket_count; | ||
| size_t bucket_index; | ||
| size_t label_count = 0; | ||
|
|
@@ -458,11 +459,23 @@ static int add_metric_histogram(struct cmt_decode_prometheus_context *context) | |
| "not enough samples for histogram"); | ||
| } | ||
|
|
||
| /* bucket_count = number of bucket samples without the "+Inf" one. Count | ||
| * them instead of assuming that sum and count are present, _sum is | ||
| * optional */ | ||
| bucket_count = 0; | ||
| cfl_list_foreach(head, &context->metric.samples) { | ||
| sample = cfl_list_entry(head, struct cmt_decode_prometheus_context_sample, _head); | ||
| if (sample->type == CMT_DECODE_PROMETHEUS_CONTEXT_SAMPLE_TYPE_BUCKET) { | ||
| bucket_count++; | ||
| } | ||
| } | ||
| if (bucket_count < 2) { | ||
| return report_error(context, | ||
| CMT_DECODE_PROMETHEUS_SYNTAX_ERROR, | ||
| "not enough buckets for histogram"); | ||
| } | ||
|
Comment on lines
+472
to
+476
There was a problem hiding this comment.
When AGENTS.md reference: AGENTS.md:L101-L106 Useful? React with 👍 / 👎.
Author
|
||
| bucket_count--; | ||
|
|
||
| if (context->opts.override_timestamp) { | ||
| timestamp = context->opts.override_timestamp; | ||
| } | ||
|
|
@@ -530,23 +543,26 @@ static int add_metric_histogram(struct cmt_decode_prometheus_context *context) | |
| sample = cfl_list_entry(head, struct cmt_decode_prometheus_context_sample, _head); | ||
| switch (sample->type) { | ||
| case CMT_DECODE_PROMETHEUS_CONTEXT_SAMPLE_TYPE_BUCKET: | ||
| if (bucket_index > bucket_count) { | ||
| break; | ||
| } | ||
| /* the last bucket is "+Inf": it has no boundary to parse and | ||
| * its value goes to the extra slot of bucket_defaults */ | ||
| if (bucket_index < bucket_count) { | ||
| if (!sample->label_values[le_label_index] || | ||
| sample->label_values[le_label_index][0] == '\0') { | ||
| ret = report_error(context, | ||
| CMT_DECODE_PROMETHEUS_SYNTAX_ERROR, | ||
| "missing histogram bucket \"le\" value"); | ||
| goto end; | ||
| } | ||
| if (parse_double(sample->label_values[le_label_index], | ||
| buckets + bucket_index)) { | ||
| ret = report_error(context, | ||
| CMT_DECODE_PROMETHEUS_CMT_CREATE_ERROR, | ||
| "failed to parse bucket"); | ||
| goto end; | ||
| } | ||
| } | ||
| if (parse_uint64(sample->value1, | ||
| bucket_defaults + bucket_index)) { | ||
|
|
@@ -604,7 +620,7 @@ static int add_metric_histogram(struct cmt_decode_prometheus_context *context) | |
| count = (uint64_t)count_dbl; | ||
| } | ||
| } | ||
| has_count = CMT_TRUE; | ||
|
|
||
| if (!timestamp) { | ||
| ret = parse_timestamp(context, sample->value2, ×tamp); | ||
|
|
@@ -618,6 +634,13 @@ static int add_metric_histogram(struct cmt_decode_prometheus_context *context) | |
| } | ||
| } | ||
|
|
||
| if (!has_count) { | ||
| ret = report_error(context, | ||
| CMT_DECODE_PROMETHEUS_SYNTAX_ERROR, | ||
| "missing histogram _count"); | ||
| goto end; | ||
| } | ||
|
|
||
| if (!timestamp) { | ||
| /* No timestamp was specified, use default value */ | ||
| timestamp = context->opts.default_timestamp; | ||
|
|
@@ -1043,9 +1066,11 @@ static int parse_histogram_summary_name( | |
| } | ||
|
|
||
| if (name_matched) { | ||
| if ((sum_found && count_found) || is_previous_sum_or_count) { | ||
| /* finish instance of the summary/histogram, the samples of the | ||
| * next instance using the base name are quantiles. _sum is | ||
| * optional (e.g. summaries of Dropwizard timers), so a quantile | ||
| * following a _sum or _count also starts a new instance */ | ||
| return finish_duplicate_histogram_summary_sum_count( | ||
| context, | ||
| metric_name, | ||
|
|
@@ -1060,8 +1085,9 @@ static int parse_histogram_summary_name( | |
|
|
||
| /* invalid histogram/summary suffix, treat it as a different metric */ | ||
| if (!strcmp(metric_name + parsed_name_len, "_bucket")) { | ||
| if (has_buckets && is_previous_sum_or_count) { | ||
| /* a bucket following the _sum or _count of an instance with | ||
| * buckets starts a new instance, _sum being optional */ | ||
| return finish_duplicate_histogram_summary_sum_count( | ||
| context, | ||
| metric_name, | ||
|
|
||
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a valid sum-less histogram emits
_countbefore its buckets (an ordering already represented bytests/data/issue_6534.txt), this new bucket-only sizing accepts it but silently corrupts the implicit bucket. Forh_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+Infsample leaves the final slot at zero, so re-encoding reports_count2 but+Inf0. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pre-existing bug but I've fixed it in b5e2486.