fix(api): fetch a grouped alert's example log lines once, not once per group - #3111
fix(api): fetch a grouped alert's example log lines once, not once per group#3111jordan-simonovski wants to merge 2 commits into
Conversation
The five sample rows quoted in a notification body come from a query carrying the saved search's filter and the evaluation window, but no group predicate. It sat in the per-group render, so a grouped alert re-ran the identical query for every group. Hoist it to the evaluation, keyed by window so backfilled buckets still fetch their own, and reuse the alias clauses the evaluation already resolved.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 1f8c991 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Greptile SummaryThis PR memoizes saved-search sample-line queries by evaluation-window start, allowing grouped notifications for the same window to share one ClickHouse query while preserving separate queries for backfilled windows.
Confidence Score: 5/5The PR appears safe to merge; no actionable correctness, security, or repository-rule issues remain. The per-window cache stores the in-flight promise before grouped renders can request another fetch, preserves distinct keys for backfilled windows, and shares both successful and failed results as intended. The latest changes only clarify the changeset wording.
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/index.ts | Adds a per-window promise cache and supplies the shared sample-line provider to grouped notification rendering. |
| packages/api/src/tasks/checkAlerts/template.ts | Extracts sample-line retrieval into a reusable helper while preserving inline fallback behavior and error handling. |
| packages/api/src/tasks/checkAlerts/tests/checkAlerts.int.test.ts | Verifies that two breaching groups in one evaluation window produce only one sample-row CSV query. |
| .changeset/alert-sample-rows-per-window.md | Documents the grouped-alert query reduction as an API patch release. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Evaluate saved-search window] --> B[Compute alias clauses]
B --> C[Evaluate grouped counts]
C --> D{Group sends notification?}
D -->|Yes| E[Request sample lines for window]
E --> F{Promise cached by window start?}
F -->|No| G[Query ClickHouse once]
F -->|Yes| H[Reuse cached promise]
G --> I[Render group notification]
H --> I
Reviews (2): Last reviewed commit: "docs: rewrite the sample-rows changeset ..." | Re-trigger Greptile
jordan-simonovski
changed the title
E2E Test Results✅ All tests passed • 357 passed • 1 skipped • 1379s
Tests ran across 4 shards in parallel. |
Deep Review✅ No critical issues found. This is a well-contained memoization refactor. The extracted 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers (7): correctness, adversarial, testing, performance, reliability, kieran-typescript, maintainability. Testing gaps: Multi-window backfill fetch-count is not covered; the new test asserts query count but not message-body content. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| try { | ||
| const withClauses = | ||
| aliasWith ?? |
There was a problem hiding this comment.
🔵 minor — aliasWith ?? compute(...) can't tell "no aliases" from "not supplied", so the advertised reuse doesn't happen for most saved searches
aliasMapToWithClauses returns undefined when the saved search's SELECT has no AS aliases (packages/common-utils/src/core/utils.ts:1357), which is the common case — e.g. the select: 'Body' fixture in this file's own test setup. aliasWithClauses in index.ts:1161 is then undefined, so fetchSampleLines re-runs computeAliasWithClauses (another full renderChartConfig of the saved search's SELECT) for every window, exactly the recomputation index.ts:1143-1144 says it is avoiding. Make the absence explicit — e.g. have computeAliasWithClauses return [] rather than undefined for "no aliases", or pass { aliasWith } as a present-but-undefined marker and switch to an 'aliasWith' in opts check.
| fetchSampleLines({ | ||
| aliasWith: aliasWithClauses, | ||
| clickhouseClient, | ||
| endTime: fns.addMinutes(startTime, windowSizeInMins), |
There was a problem hiding this comment.
🔵 minor — The sample window's end time is computed independently of the notification's end time
fns.addMinutes(startTime, windowSizeInMins) is now written twice — once for the sample query (index.ts:1310) and once for the message's own endTime (index.ts:1376) — so the rows quoted in the body and the "Time Range (UTC)" line printed above them are two hand-synced sources of truth. Compute the end time once per startTime (e.g. in trySendNotification, before the fireChannelEvent call) and pass the same value into both sampleLinesFor and fireChannelEvent.
|
|
||
| // The sample rows quoted in the message body carry no group predicate, so | ||
| // every group's notification used to re-run the identical query. | ||
| it('fetches the message body sample rows once for all groups in a window', async () => { |
There was a problem hiding this comment.
🔵 minor — New test never exercises the per-window keying, the one subtle part of the change
The test covers a single window with two groups, so a regression that drops the sampleLinesByWindow key entirely (one shared promise per evaluation) still issues exactly one CSV query and passes — while backfilled notifications would quote another window's rows. Add a backfill case (a missed tick, as the 22:16 multi-window test above at line 6458 does) asserting two CSV queries whose query_params carry distinct window bounds, and assert both groups' slack.postMessageToWebhook bodies actually contain the shared sample rows rather than only counting queries.
PR Review3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 3 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
2 participants
When a saved-search alert fires, the notification includes a handful of example log lines — the rows that actually tripped the threshold — so you can see what happened without opening the app. Fetching those lines means a second question to ClickHouse, separate from the one that counted the rows and decided to fire.
An alert can also be grouped. Grouped by service, one run sends a separate notification per breaching service: one for checkout, one for search, one for billing. Each message was built independently, and the example-lines query was being made while building each one — so ten breaching services meant asking ClickHouse ten times. It really was the same question every time: that query filters by the saved search and the evaluation window, and never by the group. Ten queries, ten identical answers, ten scans of the same data. Now it asks once and all the messages for that run share the answer.
The one wrinkle is that a run which is catching up on skipped ticks evaluates several windows at once, and each window genuinely needs its own example lines. So this is "once per window" rather than "once per run", which is why there's a small lookup keyed on the window start instead of a single saved value. A failed fetch is shared the same way a successful one is: the message falls back to carrying no example lines, exactly as it did before, rather than retrying a failing query once per group.
Two things not to read into this. An ungrouped alert is completely unaffected, because it only ever asked once — if you are looking at a slow single-group alert, this changes nothing for it. And the query stays the expensive part of sending a notification either way: it fetches raw rows in a sort order, so unlike the count that decides whether to fire, it can never be answered from a materialised view. This makes it happen fewer times, not faster.
One thing this deliberately leaves alone: because the query carries no group predicate, a grouped alert's message can quote lines belonging to a different group than the one it is reporting on. There is a standing
TODOabout that above the query. It is a correctness wart rather than a performance one, and fixing it would mean going back to a query per group — the opposite of this change — so it wants a deliberate decision rather than a quiet fix here.Implementation detail
The fetch moved out of
renderAlertTemplateinto an exportedfetchSampleLines, and the render now takes an optional provider that the evaluation supplies as a memoised closure keyed on the window. Callers that don't pass one still fetch inline, so the query itself has a single code path. The evaluation also hands over the aliasWITHclauses it had already resolved from the same saved search and source, which the render had been recomputing per group.The new integration test asserts that a two-group evaluation issues one
CSVquery, which is the only CSV query an evaluation makes. Without the fix it fails with two — identical SQL, identical parameters, identical window.