Repository navigation
fix(metrics): unblock histogram percentiles across bucket layouts - #100966
posthog[bot] wants to merge 7 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 1 should fix, 1 consider. Published 3 findings (view the review). Resolved comments: 3 fixed |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
products/metrics/backend/tests/test_metric_query_runner.py |
9 | 127 |
products/metrics/backend/metric_query_runner.py |
7 | 58 |
This check does not block merging. It updates on every push and clears when the share drops.
ℹ️ MCP UI apps size — 33 app(s), 17628.0 KB JS
Built size of each MCP UI app (main.js + styles.css).
| App | JS | CSS |
|---|---|---|
| debug | 597.8 KB | 196.2 KB |
| action | 454.1 KB | 196.2 KB |
| action-list | 564.1 KB | 196.2 KB |
| cohort | 453.1 KB | 196.2 KB |
| cohort-list | 563.1 KB | 196.2 KB |
| email-template | 452.9 KB | 196.2 KB |
| error-details | 469.1 KB | 196.2 KB |
| error-issue | 453.8 KB | 196.2 KB |
| error-issue-list | 564.0 KB | 196.2 KB |
| experiment | 561.2 KB | 196.2 KB |
| experiment-list | 564.9 KB | 196.2 KB |
| experiment-results | 566.2 KB | 196.2 KB |
| feature-flag | 566.7 KB | 196.2 KB |
| feature-flag-list | 570.5 KB | 196.2 KB |
| feature-flag-testing | 457.3 KB | 196.2 KB |
| inline-scan | 453.6 KB | 196.2 KB |
| insight-actors | 562.3 KB | 196.2 KB |
| invite-email-preview | 452.3 KB | 196.2 KB |
| llm-costs | 559.3 KB | 196.2 KB |
| session-recording | 454.9 KB | 196.2 KB |
| survey | 454.7 KB | 196.2 KB |
| survey-global-stats | 561.8 KB | 196.2 KB |
| survey-list | 564.8 KB | 196.2 KB |
| survey-stats | 561.8 KB | 196.2 KB |
| trace-span | 453.5 KB | 196.2 KB |
| trace-span-list | 564.0 KB | 196.2 KB |
| vision-observation-list | 563.2 KB | 196.2 KB |
| workflow | 453.4 KB | 196.2 KB |
| workflow-list | 563.5 KB | 196.2 KB |
| loops-review | 457.8 KB | 196.2 KB |
| query-results | 773.9 KB | 196.2 KB |
| render-ui | 856.4 KB | 196.2 KB |
| visual-review-snapshots | 457.9 KB | 196.2 KB |
✅ ClickHouse migration SQL — none
No ClickHouse migrations in the latest push.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
A well-scoped, well-tested backend query-logic fix to an internal metrics tool (histogram percentile computation) — not auth, billing, crypto, migrations, dependencies, CI, or event ingestion. My own reading of the index/SQL changes confirms they line up correctly, and the diff matches the PR's stated behavior with new tests covering each fixed scenario.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 53L, 3F substantive, 117L/5F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (117L, 5F, two-areas, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 5759aa6 · reviewed head 5759aa6 |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughHistogram quantile processing now validates bucket layouts per output point. Empty layouts and zero-count contributions do not cause conflicts. Mixed non-empty layouts within one point raise a detailed Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The histogram layout behavior and user guidance align, with no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
The posthog[bot] review has 3 unresolved threads; the most severe (cumulative counters subtracting across a layout change) does appear fixed by the new prev_bounds check in the diff, but two remain unaddressed — one where the mismatch error can show byte-identical text for two distinct layouts, and one where a histogram with empty explicit_bounds is silently excluded from the layout guard and could combine with another layout's counts into a wrong percentile. That second point is a real silent-corruption risk in a query path, not just cosmetic.
- Unresolved should_fix (posthog[bot], metric_query_runner.py): _render_bounds truncates to 6 significant digits, so two distinct bucket layouts can render identical text in the mismatch error, defeating the diagnostic's purpose.
- Unresolved consider (posthog[bot], metric_query_runner.py): the
if variantfilter drops empty-bounds layouts from the guard'slayoutsset, so an active OTel histogram with no explicit bounds can combine its counts with another layout undetected and return a false percentile — cited against real ingestion behavior and an existing 'no_bounds' test fixture.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 67L, 3F substantive, 165L/5F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (165L, 5F, two-areas, fix) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 4ca53c8 · reviewed head 4ca53c8 |
A histogram_quantile query failed whenever any two selected series used different bucket layouts, anywhere in the range. Each output point already computes its quantile from its own bounds, so layouts that differ between time buckets or between groups are harmless. Only a layout change inside one point makes the summed counts unusable. Series that contribute no counts no longer define or challenge the layout, so an idle or reset series cannot block a query. The error now names the point, its labels, and the layouts it mixes, and points at group_by, which separates layouts that a filter cannot. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 931ffce0-d0d8-4ee7-abd6-6e1a8126a591
The query-metrics prompt text is inlined into the generated tool definitions, so the histogram guidance change has to land there too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 931ffce0-d0d8-4ee7-abd6-6e1a8126a591
The per-point check leaves exactly one layout, so a separate anyIf column carried no information the variants column did not already hold. Dropping it removes an aggregate and one positional offset. The error no longer promises that an attribute always separates two layouts. When none does, it says where the problem actually lies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 931ffce0-d0d8-4ee7-abd6-6e1a8126a591
The cumulative path lagged the counts but not the bounds, and the series identity excludes the bucket layout. A layout change under one identity therefore subtracted counts from unrelated buckets: with an equal bucket count element by element, and otherwise by treating the whole cumulative vector as the increase. Both produced a percentile with no error. The per-point guard cannot see this, because a change that falls on a bucket boundary leaves each point holding one layout. The window now lags the bounds too. A sample whose layout differs from its predecessor contributes nothing, the same as a sample with no predecessor at all, so the point shows a gap and the next sample of the new layout resolves normally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 57b727df-0f85-4977-b5f0-209446feaa05
The message named the layouts it found, but two distinct layouts could render as the same text. Six shown bounds hid a change further along, and `:g` rounded away a difference past the sixth significant digit. The error then claimed two layouts and printed one string twice, which leaves an agent with the unactionable error this work removes. Bounds now print with round-trip formatting, and a layout that shares its head with another also names the bound that separates it. The length of the bounds is reported as boundaries, not buckets. Explicit buckets carry one more count than bounds and flattened exponential buckets carry one each, so the bounds alone cannot give a bucket count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 57b727df-0f85-4977-b5f0-209446feaa05
Ingestion accepts a histogram whose bounds are empty and whose single overflow count is positive. The guard dropped that empty layout before it compared layouts, so a point holding both an unbucketed series and a bucketed one raised nothing. The overflow count then landed in the first bucket of a layout it never belonged to, and the point returned a percentile built from both. The guard now counts an empty layout, so such a point reports the mismatch and names both layouts. A point where the unbucketed series is alone is unchanged. The test helper no longer folds empty bounds into the default layout, so a test can seed the unbucketed shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 57b727df-0f85-4977-b5f0-209446feaa05
Resolving the merge with master dropped the leading bounds column from the histogram query, but kept master's row[4 + group_count] for series_fingerprints. It is the third column after time, so read it at row[3 + group_count].
f68bcdc to
4510edb
Compare
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
1 participant
Problem
query-metricscannot get a histogram percentile at all: the query fails, and the recovery the tool documents does not recover it._run_histogram_quantilerejected the whole query when any two selected series used different bucket layouts, anywhere in the range.filters, named no series and no layouts, and filters cannot separate two layouts inside one point anyway.Origin
Changes
groupBythat puts each layout in its own series.groupUniqArrayIfskips them, so an idle series cannot block a query.groupBy, which separates layouts that a filter cannot, and says where the problem lies when no attribute separates them.histogram_quantileon a metric whose series do not all share one bucket layout:groupBy: service_name, one layout eachservices/mcp/schema/*.jsonregenerated, because the prompt text is inlined there.How did you test this code?
products/metrics/backend/tests/test_metric_query_runner.pyran against the sandbox ClickHouse.groupBy, a layout change between points, and an idle series on another layout.groupBy. It passed before against anyValueError, including an unactionable one.mypyon the changed module reports no issues. The repo-wide run did not finish in this sandbox, so CI is the check for reverse dependencies.Automatic notifications
Docs update
None. The MCP prompt is the only user-facing text, and it changes in this PR.
🤖 Agent context
Autonomy: Fully autonomous
Agent: PostHog Desktop (Claude Code), Claude Opus 5
/writing-pr-descriptions,/reviewing-with-coderabbit,/writing-simplified-technical-english.crnorcoderabbitis on PATH in this sandbox and flox is not installed, so the PR opens without a local pass.gh pr list --state open --searchover histogram and metrics keywords found no open PR touching this guard.arrayExists, notarraySum. HogQL casts the counts toNullable(Float64), and ClickHouse rejectsarraySumon that./simplifydropped a second aggregate the first draft kept alongsidebounds_variants, and flagged that the error copy promised a recovery that does not always exist. Both are in the final diff.Created with PostHog Desktop from this inbox report.
Summary by CodeRabbit
Bug Fixes
Documentation