Repository navigation
feat(error-tracking): overlay recent issue state - #87915
Conversation
|
😎 Stack merged successfully - details. |
🤖 CI report
|
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 3 should fix, 0 consider. Published 3 findings (view the review). Resolved comments: 2 fixed, 1 already settled |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
Prompt To Fix All With AI### Issue 1
products/error_tracking/backend/hogql_queries/error_tracking_query_builder.py:579-580
**Legacy overlay drops unmapped events**
When a mixed-filter query runs during the recent-state window and a matching exception fingerprint has not reached ClickHouse, this new fingerprint-state inner join discards the event before filtering and grouping, causing a valid issue to disappear from the results.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(error-tracking): keep issue-state wa..." | Re-trigger Greptile |
0c711d5 to
6abdd27
Compare
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
The bulk mutation endpoint does not cap how many issues one request changes, and every changed issue was loaded from Postgres and sent as an external-table row on each forced refresh for the whole overlay window. A large bulk resolve or assign could materialize an unbounded row list and ship it to ClickHouse repeatedly. Cap the loaded rows at MAX_RECENT_ISSUE_STATES. When a mutation touches more issues than the bound, skip the overlay and fall back to ClickHouse state instead of shipping the payload. The database read is limited to the bound plus one so the query itself stays cheap. Generated-By: PostHog Desktop Task-Id: a2355240-7fae-4452-b056-7d8eee6d040c
The issue-state overlay adds two primary-Postgres reads to every error tracking query. The watermark read runs on the cache-key path, before any cache lookup, so an OperationalError there fails a request whose result may already be cached. The recent-states read runs during calculation, so a failure there fails the whole query even though ClickHouse could serve a slightly stale result without the overlay. Wrap both reads in except OperationalError, mirroring AnalyticsQueryRunner._products_modifiers_for_cache: the watermark degrades to a stable "unavailable" cache marker, and the overlay falls back to an empty list so the query runs without it. Log each failure and count it on a new error_tracking_recent_issue_state_overlay_unavailable_total metric. Add a regression test for the cache-payload degradation. Generated-By: PostHog Desktop Task-Id: a1a0ffdb-a9ff-4893-b651-6d018645eb54
The cache watermark is derived as the maximum state_updated_at over live issue rows. A merge deletes the source rows without touching the target's timestamp, so a deleted source that held the team maximum drags the watermark backward. That can re-validate a pre-merge cache key and serve a snapshot from before the merge for the duration of the staleness window, showing a merged-away issue as still present. Stamp the merge target's state_updated_at inside merge(), so the watermark stays monotonic even when the deleted source held the maximum. This lives in the model's merge() so both the user mutation path and the Temporal auto-merge path are covered. Stamping is also semantically correct: a merge changes the target's composition. Add a regression test asserting the watermark does not move backward across a merge. Generated-By: PostHog Desktop Task-Id: a1a0ffdb-a9ff-4893-b651-6d018645eb54
6abdd27 to
232abb3
Compare
|
Reviews (2): Last reviewed commit: 6abdd27 | Re-trigger Greptile |
Problem
Error tracking users can briefly see stale issue status, severity, and assignments after a mutation.
ClickHouse receives issue state asynchronously. The current workaround sends client-generated rows for each fingerprint and caps them at 50.nnFollow-up: #87950 removes those client rows after production validation.
Before
flowchart LR A[Issue mutation] --> B[Frontend fetches fingerprints] B --> C[Pending row per fingerprint] C --> D[ClickHouse UNION ALL and argMax] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class A phYellow; class B,C phBlue; class D phRed;Changes
After
flowchart LR A[Issue mutation] --> B[Postgres state timestamp] B --> C[Query loads recent issue state] C --> D[ClickHouse external overlay by issue ID] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class A phYellow; class B,C phBlue; class D phRed;How did you test this code?
hogli ci:preflightbecause it requires Git metadata unavailable in this Jujutsu workspace.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Automatic notifications
Docs update
No docs update. This changes internal read-after-write consistency without changing an API or documented workflow.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Pi implemented the approved issue-state overlay design. The implementation uses the existing fingerprint overlay only for mapping changes.
Skills invoked:
/error-tracking,/writing-tests,/writing-code-comments,/writing-dataclasses,/running-ci-preflight,/writing-pr-descriptions, and/pr.The committed tests and identifiers use repository fixtures and invented values. No private customer material informed the change.