Repository navigation
fix(metrics): take the overview last datapoint from the newest sample - #116676
DanielVisca wants to merge 2 commits into
Conversation
metric_series only takes rows from labelled samples, and capture labels only the first sample of each label window, so its last_seen lags the newest sample. The overview strip could then show an old last datapoint or the stale banner while data still arrives. Read the newest sample time from metric_samples, bounded to the hours after the series last_seen, and keep the later of the two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 28e82888-d17f-491f-8aa4-4ba0138e92be
|
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 |
🤖 CI report
|
| File | Patch | Uncovered changed lines |
|---|---|---|
products/metrics/backend/metrics_overview_query_runner.py |
77.8% | 44–47, 52, 54 |
🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 105740348136830 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
metrics |
██████████████████░░ 90.7% |
8,439 / 9,309 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| series_last_seen = rollup.last_seen if rollup.services else self._run_freshness_fallback() | ||
| newest_samples = self._run_newest_samples(series_last_seen) if series_last_seen is not None else {} |
There was a problem hiding this comment.
P2 · Use each service's last seen time as its freshness lower bound
When service A has a labelled sample at 12:25 and service B has labels at 12:00 followed by an unlabelled sample at 12:20, _run_newest_samples starts at the global 12:25 maximum. It excludes B's newest sample, so B's returned last_seen remains 12:00 despite newer stored data. Use per-service lower bounds when fetching service freshness.
There was a problem hiding this comment.
Fixed in 39faa9e. Each listed service's read now starts at the hour of its own series last_seen, through transform(service_name, ...) in the time_bucket filter. New test case: service A is labelled at 12:25, and service B is labelled at 12:00 with an unlabelled sample at 12:20. B now reports 12:20. The test failed before the fix.
| ORDER BY newest_sample_at DESC | ||
| LIMIT {limit} |
There was a problem hiding this comment.
P2 · Align the freshness service limit with the returned services
For teams with more than 500 services, _run_services selects services by series count, but _run_newest_samples selects them by sample recency. A displayed service outside the 500 freshest services receives no sample timestamp and retains its stale labelled timestamp. Fetch freshness for the returned service set while preserving the separate overall maximum.
There was a problem hiding this comment.
Fixed in 39faa9e. The read now covers exactly the services that _run_services returned, with one row each, plus one row for all other services, so the overall maximum stays correct. It sets LIMIT len(names) + 1, because without an explicit limit HogQL adds LIMIT 100 and cuts teams with more listed services. New test case: MAX_SERVICES patched to 1, with the newest sample in an unlisted service. The test failed before the fix.
| toTimeZone(max(timestamp_max), 'UTC') AS newest_sample_at | ||
| FROM posthog.metric_samples | ||
| WHERE time_bucket >= {bucket_from} | ||
| AND timestamp_max >= {since} | ||
| GROUP BY service_name |
There was a problem hiding this comment.
P2 · Read sample freshness from a scalar rollup
For high-cardinality projects, every overview refresh scans timestamp arrays for all active series in the selected hours: timestamp_max is an ALIAS arrayMax(timestamp_arr), so it requires reading every timestamp in each matching row. The page polls every 30 seconds, and LIMIT 500 does not bound this scan. Store a scalar sample maximum or add an aggregate freshness projection.
There was a problem hiding this comment.
Confirmed: timestamp_max is ALIAS arrayMax(timestamp_arr) in every environment, and no existing table has a scalar newest time that includes unlabelled samples. metrics4_names, metrics4_series and metrics4_attributes take only labelled rows, and time_bucket is only hourly. So a real fix needs a ClickHouse schema change, which I kept out of this PR.
What this PR does to limit the cost: each service reads timestamp arrays only from the hour of its last labelled sample. Capture sends labels again every 30 minutes by default, so for a service that still sends data, the read covers about 2 hourly buckets. On seeded local data, the read with all services fresh costs the same as before the fix (same rows, same bytes).
Options for a follow-up, all with a migration:
- A. A small rollup table of team, service, hour and max timestamp, fed from
metrics4_inputwith nohas_labelsfilter. This is the cheapest read and exact. - B. A stored scalar max-timestamp column on
metrics4_samples, written by the samples view. - C. A projection on
metrics4_samples. It is awkward becauseservice_nameis an aggregate column there.
I suggest A. @PostHog/team-apm, can you confirm whether the current bounded read is acceptable for the 30-second overview poll until a follow-up adds A?
|
Risk: No findings The change rewrites the overview's newest-sample query so each listed service reads from its own series-last_seen hour via a Sentinel reviewed |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
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.
@PostHog's three substantive concerns remain unaddressed, so its current-head review supplies no concern-free assurance. Fix the lower-bound and service-selection issues and address the scan-cost concern with @PostHog/team-apm, with Frank Hamand as a suggested reviewer, before re-requesting.
- Author wrote 0% of the modified lines and has 16 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: 3 findings, posted on the PR.
- posthog[bot] reviewed the current head.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: The freshness query uses the global maximum labelled timestamp as its lower bound (lines 237–238), excluding newer unlabelled samples for services whose latest sample precedes that global maximum. The per-service response then retains its stale labelled timestamp.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: The displayed services are limited by series count (lines 189–190), while sample freshness is limited independently by recency (lines 124–125). With more than 500 services, displayed services can be omitted from the freshness results and retain stale timestamps.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: The new aggregation reads timestamp_max across matching sample rows without a scalar freshness rollup. posthog/clickhouse/metrics/metrics4.py:226 defines timestamp_max through arrayMax(timestamp_arr), and the overview refresh interval is 30 seconds; the result limit does not bound the aggregation scan. The current code does not address the reviewer's high-cardinality performance concern.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 95L, 1F substantive, 133L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (133L, 3F, single-area, fix) |
| stamphog 2.5.0 | .stamphog/policy.yml @ 1f654c2 · reviewed head 1f654c2 |
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.
@PostHog's three substantive concerns remain unresolved in the current code, so its current-head review does not provide concern-free assurance. Address those threads and request follow-up review from @PostHog/team-apm, with Frank Hamand as a suggested reviewer, before re-requesting.
- Author wrote 0% of the modified lines and has 16 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: 3 findings, posted on the PR.
- posthog[bot] reviewed the current head.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: The sample query uses the global latest labelled timestamp as its lower bound (lines 237–238), excluding newer unlabelled samples for services whose timestamps precede that global maximum. The per-service freshness concern remains unfixed.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: Services are selected by series count (lines 189–190), whereas sample freshness selects 500 services by recency (lines 124–125). Displayed services outside the latter set retain stale labelled timestamps; the selection mismatch remains unfixed.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: The additional aggregate reads timestamp_max across matching sample rows, and the underlying column is an arrayMax alias in posthog/clickhouse/metrics/metrics4.py:226. The frontend refresh interval is 30 seconds, and the result limit does not bound the aggregate scan. The requested scalar freshness rollup or projection is absent.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 95L, 1F substantive, 133L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (133L, 3F, single-area, fix) |
| stamphog 2.5.0 | .stamphog/policy.yml @ 1f654c2 · reviewed head 1f654c2 |
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.
@PostHog's three substantive concerns remain unresolved in the current code, so its current-head review does not provide concern-free assurance. Address those threads and request follow-up review from @PostHog/team-apm, with Frank Hamand as a suggested reviewer, before re-requesting.
- Author wrote 0% of the modified lines and has 16 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: 3 findings, posted on the PR.
- posthog[bot] reviewed the current head.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: The sample query uses the global latest labelled timestamp as its lower bound (lines 237–238), excluding newer unlabelled samples for services whose timestamps precede that global maximum. The per-service freshness concern remains unfixed.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: Services are selected by series count (lines 189–190), whereas sample freshness selects 500 services by recency (lines 124–125). Displayed services outside the latter set retain stale labelled timestamps; the selection mismatch remains unfixed.
- @PostHog, products/metrics/backend/metrics_overview_query_runner.py: The additional aggregate reads timestamp_max across matching sample rows, and the underlying column is an arrayMax alias in posthog/clickhouse/metrics/metrics4.py:226. The frontend refresh interval is 30 seconds, and the result limit does not bound the aggregate scan. The requested scalar freshness rollup or projection is absent.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 95L, 1F substantive, 133L/3F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (133L, 3F, single-area, fix) |
| stamphog 2.5.0 | .stamphog/policy.yml @ 1f654c2 · reviewed head 1f654c2 |
… start hour The newest-sample read started at the latest series last_seen across all services, so a service with older labelled samples kept its old time. It also ranked services by recency with LIMIT 500, so a listed service could miss its sample time. The read now looks only at the listed services, starts each one at the hour of its own last_seen, and groups the other services into one row so the overall maximum stays correct. It also sets an explicit LIMIT instead of the HogQL default of 100. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 28e82888-d17f-491f-8aa4-4ba0138e92be
This branch has not been deployed
1 participant
Problem
max(last_seen)frommetric_series.has_labels = false.metric_seriestakes rows only from labelled samples, so itslast_seencan lag the newest sample by up to the label window (30 minutes by default).Changes
max(timestamp_max)per service frommetric_samplesand keeps the later of that and the serieslast_seen.last_seenhour, so an active team reads about one hour of rows. It stays team-scoped and limited to 500 services.has_labelsargument, and the rollup keeps datetimes until the response is built.How did you test this code?
test_metrics_overview_query_runner.py. The new case failed before the fix, with the labelled sample time 20 minutes before the newest sample, and passes after.Test rationale: The new case seeds a labelled sample and a later unlabelled sample, plus a newer sample for another team. It catches a regression to the series
last_seenand a leak across teams. The existing overview tests seeded only labelled samples.Release status
Docs update
None.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Slack app (Claude Code), claude-opus-5-5
metric_names_query_runner.py. This PR fixes the overview, uses the same rule (newest sample wins), and leaves that file alone. Both add the same seeder argument, so the second to merge gets a small conflict.Created with PostHog from a Slack thread
🤖 Generated with Claude Code