Conversation
|
Risk: No findings This increment adds Sentinel reviewed |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
🤖 CI report
|
| File | Comment lines | Added lines |
|---|---|---|
products/metrics/backend/metrics_overview_query_runner.py |
3 | 11 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Playwright — 1 flaky
🎭 Playwright report · View test results →
- create experiment via wizard, add metrics, and launch (chromium)
These issues are not necessarily caused by your changes.
Annoyed by this section? Help fix flakies and failures and it will go green!
|
[Medium risk] Adjusts metrics query to use hourly projection. The PR is not ready to merge because its active-series window disagrees with the window reported to users, and the new test must meet the repository’s testing requirement. Reviews (1) · Last reviewed commit: "perf(metrics): read the hourly projectio..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| toTimeZone(max(max(last_seen)) OVER (), 'UTC') AS total_last_seen_at | ||
| FROM posthog.metric_series | ||
| WHERE last_seen > now() - {lookback} | ||
| WHERE time_bucket >= toStartOfHour(toTimeZone(now() - {lookback}, 'UTC')) |
There was a problem hiding this comment.
last_seen cutoff alongside the bucket bound, or report the window the query actually uses.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/metrics/backend/metrics_overview_query_runner.py
Line: 128
Comment:
**Lookback includes stale series.** If a series was last seen before the lookback cutoff but within the same UTC hour, this bucket-only filter still includes it. The overview can therefore show nearly an extra hour of stale services and active series while labeling the counts “Last 24 hours.” Keep an exact `last_seen` cutoff alongside the bucket bound, or report the window the query actually uses.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
it really doesn't matter if it's 25hrs
There was a problem hiding this comment.
You're right — the 24–25 hour whole-UTC-hour window is explicitly intentional, so including rows from the cutoff hour is part of the desired behavior. My comment treated the lookback as an exact 24-hour cutoff; it isn't a valid finding for this change, and no adjustment is needed.
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
@greptile-apps[bot] still has a review in flight (👀) — not approving over an unfinished review. The review re-runs on the next push, or re-request one once the reviewer finishes.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 22L, 2F substantive, 47L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (47L, 3F, two-areas, perf) |
| stamphog 2.3.1 | .stamphog/policy.yml @ 06a9481 · reviewed head 06a9481 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/trunk merge |
|
⏳ Stack waiting to start tests on this stack - details. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe metrics overview query now filters metric series by the UTC hour bucket, uses Priority: ➖ Normal Merge Risk: 🔵 Low · up to The overview query’s intended UTC-hour window is supported, but its new regression test leaves the boundary unprotected. Add that coverage before merging if practical, and deploy only after the projection is ready as the PR specifies. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected query remains team-scoped and read-only, with no demonstrated increase in access or privileges. The main uncertainty is whether projection coverage is complete before rollout and whether mixed coverage preserves complete overview results. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
products/metrics/backend/tests/test_metrics_overview_query_runner.py (1)
73-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the hourly boundary in the forced-projection test.
The query filters from
toStartOfHour(now() - lookback), so the test should include one earlier point from the same series in another included UTC hour. Add a separate series in the bucket immediately before that boundary and assert that it is excluded. Use a distinct label set for the excluded series souniq(series_fingerprint)makes the assertion observable.Existing tests cover same-window aggregation and data several days outside the window. They do not reliably detect cross-hour aggregation or the exact whole-hour boundary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 8291365d-8be2-4d5b-8f6c-1acebfdd84f7
📒 Files selected for processing (4)
posthog/hogql/constants.pyposthog/hogql/database/schema/metrics.pyproducts/metrics/backend/metrics_overview_query_runner.pyproducts/metrics/backend/tests/test_metrics_overview_query_runner.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
The PR was retargeted to a different base branch, so the approved diff is no longer what was reviewed. Stamphog re-reviews automatically.
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
Two gates refused this pull request, so stamphog can't review it. The deny-list gate matched the migrations path: posthog/clickhouse/migrations/0345_metrics4_series_services_projection.py and max_migration.txt are changed, and migrations always need a human reviewer. The tier gate also classified the change as T2-never, because it spans two areas (ClickHouse schema and HogQL/query runner code) and is a performance change, across 19 files and 311 lines.
To move forward, ask a human reviewer, such as someone on the owning team, to review it. The description says this is already stacked on #110666, so you could also move the schema and migration pieces into that PR and leave only the query runner and HogQL changes here. That would give a smaller change in one area for a later pass.
- coderabbitai[bot] reviewed the current head.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: migrations |
| size | ✓ | 258L, 16F substantive, 311L/19F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (311L, 19F, two-areas, perf) |
| stamphog 2.3.1 | .stamphog/policy.yml @ unknown · reviewed head e683ee6 |
…er (#110771) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
The services query scanned every series-hour row of the last day. The services_by_hour projection on metrics4_series holds the same rollup per team, hour and service, but the query shape kept ClickHouse from using it. The query now filters on time_bucket, counts series with uniq, and aggregates the bare last_seen column with convertToProjectTimezone off, converting to UTC outside max(). metric_series exposes time_bucket for the filter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 3538a544-5668-418a-aebd-c3430d18864a
The overview test now runs the services query with force_optimize_projection, so ClickHouse rejects the query when the projection is not used. HogQL settings accept the new field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 3538a544-5668-418a-aebd-c3430d18864a
e683ee6 to
de574c6
Compare
There was a problem hiding this comment.
Approved.
Contained, reversible perf change to the metrics overview query, with a test that forces projection use. The author has strong familiarity with the code. The one reviewer concern (24 to 25 hour window) was answered as intentional, and the rollout-ordering warning is disclosed in the description.
- Author wrote 100% of the modified lines and has 38 merged PRs in these paths (familiarity STRONG).
- 👍 on the PR from greptile-apps[bot].
- Rollout ordering: the services_by_hour projection must be deployed and cover all parts in the last day before this ships, or the overview can show incomplete results. The author disclosed this.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 23L, 3F substantive, 42L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1b-small (42L, 4F, two-areas, perf) |
| stamphog 2.3.1 | .stamphog/policy.yml @ de574c6 · reviewed head de574c6 |
This branch has not been deployed
3 participants
Problem
The metrics overview page is slow for teams with many active series.
metrics4_series.services_by_hourprojection, which holds the same rollup per team, hour and service.Changes
uniq, so for large teams the number can differ slightly from an exact count.metric_seriesin HogQL gets atime_bucketcolumn, so the query can filter on it.convertToProjectTimezoneoff and converts to UTC outsidemax().force_optimize_projection. Only the test sets it.ClickHouse used the projection only for one shape of the query (
EXPLAINon the dev metrics cluster):timestampuniqExactmax(toTimeZone(timestamp))time_bucketuniqmax(toTimeZone(timestamp))timestampuniqmax(timestamp)time_bucketuniqtoTimeZone(max(timestamp))HogQL wraps every DateTime column in
toTimeZone, somax(last_seen)prints asmax(toTimeZone(timestamp, ...)). That expression does not match the projection'smax(timestamp), which is why the modifier is off for this query.Warning
Merge this only after #110666 is deployed and every part in the last day has the projection. In dev, a query that used the projection while only some parts had it returned incomplete results.
How did you test this code?
pytest products/metrics/backend/tests/test_metrics_overview_query_runner.pypasses locally.EXPLAINon the dev metrics cluster, for the SQL this query prints, showsAggregatingProjectionand a read fromservices_by_hour.Test rationale:
test_services_query_reads_the_hourly_projectionruns the services query withforce_optimize_projection, so ClickHouse rejects the query when it does not use a projection. It fails if the query shape drifts, for example a return touniqExactor a removed modifier. That drift would only show as a slow page. With the modifier removed, this test failed withPROJECTION_NOT_USEDand every other overview test passed.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Opus 5.5
metric_names, which has no projection, so a module-wide setting would fail that query./writing-tests,/writing-code-comments,/writing-pr-descriptions.🤖 Generated with Claude Code
Created with PostHog Desktop