Repository navigation
parse: return protobuf explicit-bucket histograms as NHCB - #42
nicolastakashi wants to merge 6 commits into
Conversation
ProtobufParser now converts a histogram without native buckets to one native histogram with custom buckets (schema -53), like Go's NewProtobufParser with convertClassicHistogramsToNHCB set. Before, these histograms were skipped. Histogram gets a custom_values field for the bucket bounds. The Go harness turns NHCB conversion on, prints custom values, and marshals hand-written NHCB cases from tests/go-harness/testdata/nhcb. Refs #37 Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
Replace the nested branches with small helpers and one ensure check. Keep Go's behaviour of appending the +Inf bucket without a cumulative check, and add a Go harness case for it. Use wrapping math for bucket deltas, like Go. Signed-off-by: Nicolas Takashi <nicolas.takashi@dash0.com>
|
|
||
| /// Go's `TempHistogram.Convert` returns an integer histogram when every count is a whole number. | ||
| fn is_integer_histogram(count: f64, buckets: &[(f64, f64)]) -> bool { | ||
| let is_integer = |value: f64| value == (value.round() as i64) as f64; |
There was a problem hiding this comment.
For 2^63, such as sample_count = 1 << 63, Rust's as i64 saturates to i64::MAX and casting back to f64 rounds right back up to 2^63, so is_integer returns true and convert_to_nhcb clamps the bucket count to i64::MAX while keeping count == 1 << 63. Guarding with value < i64::MAX as f64 here and adding a Rust unit test for 1 << 63 keeps bucket sums valid and matches Go on x86_64 returning a FloatHistogram.
| /// Each value minus the value before it, starting from zero. | ||
| fn differences<B: Bucket>(values: impl Iterator<Item = B>) -> impl Iterator<Item = B> { | ||
| values.scan(B::ZERO, |previous, value| { | ||
| let difference = value.add(previous.neg()); |
There was a problem hiding this comment.
When a classic bucket has cumulative_count_float: nan followed by a finite bucket, value.add(previous.neg()) flips the NaN sign bit before adding and produces 0xfff8... instead of matching Go's b.count - prevCount in convertnhcb.go#L222. Could we add sub to Bucket using wrapping_sub and -, and cover cumulative_count_float: nan in conversion.prototxt?
| ensure(cumulative_count >= 0.0 || cumulative_count.is_nan(), || { | ||
| format!("bucket count must be non-negative: le={upper_bound}, count={cumulative_count}") | ||
| })?; | ||
| let index = buckets.partition_point(|&(bound, _)| bound < upper_bound); |
There was a problem hiding this comment.
Looks like the refactor in 1dddac0 dropped the sorted-bucket append fast path from a4567ae, even though the PR description and benchmark table still describe it. Restoring the buckets.last() check before partition_point and updating the benchmark commit links in the PR body would get that happy path back.
| if err != nil { | ||
| return "", err | ||
| } | ||
| return protobufParseInput, writeDelimited(filepath.Join(outRoot, protobufParseInput), families) |
There was a problem hiding this comment.
Upstream also has TestProtobufParseWithNHCB in protobufparse_test.go#L4134-L4281 with the same testMetricFamilies slice layout, covering NHCB unit metadata and multiple timestamped bucket exemplars. Worth extracting that fixture here too so matches_go_protobufparse_output covers it?
| schema: 0 | ||
| zero_threshold: 0.001 | ||
| zero_count: 1 | ||
| > |
There was a problem hiding this comment.
Right now mixed only exercises int_histogram reuse across native and NHCB metrics, leaving float_histogram state reset like custom_values.clear() and negative_spans.clear() untested. Could we also include a float native to float NHCB to float native sequence here?
Refs #37
ProtobufParsernow returns a histogram without native buckets as one native histogram with custom buckets (NHCB, schema -53), like Go'sNewProtobufParserwithconvertClassicHistogramsToNHCBset. Before this change these histograms were skipped, so their data was lost.Histogramgets acustom_valuesfield for the bucket bounds.The Go harness now runs
NewProtobufParserwith NHCB conversion on, prints custom values, and marshals hand-written cases fromtests/go-harness/testdata/nhcb/: float and gauge histograms, unsorted and repeated bounds, runs of empty buckets, missing histograms, native and classic histograms in one family, and the six conversion errors.Follow-up for the rest of #37: port Go's
NHCBParserfor the text and OpenMetrics 1.0 parsers, and remove the classic histogram grouping insrc/arrow/family.rs.Differences from Go:
maxEmptyBucketsapart. An NHCB starts with one span and is only split at gaps longer than that, so the step never changes the result. https://github.com/prometheus/prometheus/blob/v0.315.1/model/histogram/generic.go#L452-L515Display, not Go's%g. https://github.com/prometheus/prometheus/blob/v0.315.1/util/convertnhcb/convertnhcb.go#L90-L110Tested: the AGENTS.md checks and
go generate ./... && go test ./...with notestdata/diff. Withsrc/parse/protobufparse.rsreverted tomain,matches_go_protobufparse_outputfails on the regenerated fixtures.Performance. After the first scrape, conversion does no heap allocations. I measured
convert_to_nhcbwith a throwaway release-mode test (arm64, 2M iterations, 3 runs each) at 167096b and a4567ae. The second commit makes two changes. It appends sorted buckets without a binary search, like Go's happy case inSetBucketCount(https://github.com/prometheus/prometheus/blob/v0.315.1/util/convertnhcb/convertnhcb.go#L84-L94). It also letscompact_bucketsreturn early when no bucket is empty. Before, that early return never fired for an NHCB, because its first span has offset 0.Not run:
benchstat, becauseProtobufParserispub(crate)and no benchmark target can reach it, and the OTAP compat suite, becausesrc/arrowis unchanged.