feat: Implement quantile+sum for exponential histogram metrics - #2697
Conversation
🦋 Changeset detectedLatest commit: f2aabf1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
E2E Test Results✅ All tests passed • 243 passed • 1 skipped • 1009s
Tests ran across 4 shards in parallel. |
Greptile SummaryThis PR adds query-builder support for OTEL exponential histogram metrics. The main changes are:
Confidence Score: 4/5This is close, but the count aggregation issue should be fixed before merging.
packages/common-utils/src/core/histogram.ts Important Files Changed
Reviews (8): Last reviewed commit: "Merge branch 'main' into drew/exponentia..." | Re-trigger Greptile |
Deep ReviewMulti-agent review of the exponential-histogram quantile+sum implementation. Six reviewers ran against the net-new code in 🔴 P0/P1 -- must fix
🟡 P2 -- recommended
🔵 P3 nitpicks (9)
Reviewers (6): correctness, adversarial, performance, testing, maintainability, kieran-typescript. Testing gaps:
|
472934e to
f21f9b5
Compare
f21f9b5 to
9c1fe4b
Compare
9c1fe4b to
a103ec0
Compare
This is not true - ClickHouse promotes the subtraction to signed Int64. |
6d37e38 to
66e21d8
Compare
knudtty
left a comment
There was a problem hiding this comment.
Some strategy related comments
| { | ||
| name: 'source', | ||
| sql: chSql` | ||
| SELECT |
There was a problem hiding this comment.
First off, are some of these CTEs referenced multiple times throughout the query? ClickHouse does this really weird thing where it will recalculate the CTE for each time you reference the query. There was a MATERIALIZED modifier recently added https://clickhouse.com/docs/sql-reference/statements/select/with#materialized-common-table-expressions, perhaps that could help perf on several of these? It was added in 26.4, so we'd need some conditional logic to add that modifier
| count() OVER prev_row = 0 AS is_first_series_point, | ||
| toInt64(any(Count) OVER prev_row) AS previous_count, | ||
| any(StartTimeUnix) OVER prev_row AS previous_start_time, | ||
| CASE | ||
| WHEN AggregationTemporality = 1 THEN current_count | ||
| WHEN AggregationTemporality = 2 THEN | ||
| multiIf( | ||
| is_first_series_point OR StartTimeUnix = TimeUnix, 0, | ||
| StartTimeUnix != previous_start_time OR current_count < previous_count, current_count, | ||
| current_count - previous_count | ||
| ) | ||
| ELSE 0 | ||
| END AS delta |
There was a problem hiding this comment.
I see what you're saying about a lot of extra logic to support the different aggregation temporalities, I think it's likely worthwhile to ticket up an issue to create some prefetch query to determine the correct one to use for a series, cache it, and fire off the correct aggregation query. That would likely greatly help AggregationTemporality=1 queries.
| }): WithClauses => [ | ||
| // Filter for the relevant source data | ||
| { | ||
| name: 'filtered_series', |
There was a problem hiding this comment.
Looks like this one at least is used multiple times, the MATERIALIZED modifier would probably help here for ch-server >= 26.4
| series.PositiveOffset + length(series.PositiveBucketCounts) - 1 AS positive_last_index, | ||
| bitShiftRight(series.PositiveOffset, scale_shift) AS normalized_positive_offset, | ||
| normalized_negative_offset AS NegativeOffset, | ||
|
|
||
| series.NegativeOffset + length(series.NegativeBucketCounts) - 1 AS negative_last_index, | ||
| bitShiftRight(series.NegativeOffset, scale_shift) AS normalized_negative_offset, | ||
| normalized_positive_offset AS PositiveOffset, |
There was a problem hiding this comment.
Would prefetch queries help with these positive & negative offsets too?
| ) AS negative_bucket_indexes, | ||
| NegativeBucketCounts::Array(Int64) AS negative_bucket_counts | ||
| FROM series_with_normalized_scale | ||
| WHERE AggregationTemporality = 1 |
There was a problem hiding this comment.
Schema related: I don't think we have any index on this column?
There was a problem hiding this comment.
Although it's referencing a previous CTE, might not help in this branch
|
Great suggestions @knudtty - I am collecting potential optimizations in https://linear.app/clickhouse/issue/HDX-4865/investigate-potential-exponential-histogram-optimizations I have tested omitting cumulative handling and negative bucket handling from the query, and the gains are minimal on large scales of data right now. The AggregationTemporality index could be useful if we can push it down into filtered_series. Ideally the selection should be either temporality 1 or 2, so I suspect one of those selections should always be empty, in which case materialization may not have much of an impact. But all of this can be tested more formally! |
| PARTITION BY ${groupBy ? 'group, ' : ''}MetricName, attr_hash, AggregationTemporality | ||
| ORDER BY TimeUnix | ||
| ROWS BETWEEN 1 PRECEDING AND 1 PRECEDING |
There was a problem hiding this comment.
The new count query uses the same cumulative predecessor model as the quantile query, so each window predecessor must come from the same OTEL stream. This partition only separates rows by group, MetricName, attr_hash, and temporality. If two cumulative streams share those values but differ in stream identity fields outside the attribute hash, their rows can interleave and previous_count or previous_start_time can come from the other stream. That makes the count delta subtract the wrong baseline, so exponential histogram count charts can be over- or under-counted.
Knowledge Base Used: common-utils
Summary
This PR implements quantile and sum aggregations over OTEL Exponential Histogram metrics in the query builder.
max/min/sum aggregations are not supported, consistent with the histogram metrics implementation, because max/min/sum are optional in the OTEL Histogram data model. Similarly avg is not supported because it would rely on the unsupported sum aggregation.
Known limitation: Only timeseries display type is correct, inline with the existing histogram metric type. HDX-4858 will address this limitation.
Exponential Histogram Data Model
See the OTEL Docs: https://opentelemetry.io/docs/specs/otel/metrics/data-model/#exponentialhistogram
Important notes:
Query details
The quantile query is horrifying! It is broken into a few CTEs:
filtered_series- first step, apply filters so that later stages are only working with the relevant dataseries_with_normalized_scale- Convert all data points to the minimum Scale value, so that they can be aggregated later. Finer-grained buckets are combined to reduce Scale. Offsets are accounted for so that the buckets being combined are aligned correctly.normalized_deltas- This is a passthrough for metrics with delta temporality. For cumulative temporality, this converts cumulative counts in each data point to deltas, by comparing counts in each data point to the counts from the previous data point in the same series, via a window function. This is the most expensive part of the process, for cumulative metrics.summed_buckets- Sum bucket counts across timeseries by time bucket + custom group byselected_quantile_buckets- for each time bucket + custom group by, find the histogram bucket containing the requested quantile level. This requires combining negative, zero, and positive counts, in bucket order, then converting each bucket to a cumulative sum (sum of its own counts and counts in all lesser buckets).metrics- Within the bucket, estimate the quantile value by interpolating between the bucket's upper and lower bounds. Use log-linear interpolation to match Prometheus behavior.Screenshots or video
How to test locally
This can be tested locally by
NEXT_PUBLIC_ENABLE_EXPONENTIAL_HISTOGRAMS=truein .env.localComparing the data to identical metrics sent to prometheus + grafana is a good sanity check, and agents can set this up relatively well.
References