Fix alert sample events for filtered searches - #3091
Conversation
🦋 Changeset detectedLatest commit: a660bda The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
|
@ryux1 is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
external
needs-vouch
|
Hi @ryux1, thanks for the pull request! Before we review code from a first-time contributor we ask that a maintainer vouches for you, and you're not on our list yet. This PR stays open — it just isn't in the review queue until someone vouches. To get vouched, open an issue saying hello and what you're working on: https://github.com/hyperdxio/hyperdx/issues/new?template=introduce-yourself.md A maintainer will usually reply within a day or two, and then this PR gets picked up as normal. More detail in our contributing guide. |
Greptile SummaryThis PR aligns alert notification samples with saved-search filtering and the firing group while improving alert-response metadata handling.
Confidence Score: 5/5The PR appears safe to merge based on the current review state and has no accepted new findings. No new reportable issue remains. The matching prior empty-group-key finding was manually resolved, so it is not outstanding and is not reposted.
|
| Filename | Overview |
|---|---|
| packages/api/src/tasks/checkAlerts/index.ts | Derives response metadata, separates alert values from group projections, and passes discriminated grouping data into notifications. |
| packages/api/src/tasks/checkAlerts/template.ts | Builds filtered sample-event queries and visibly handles partial predicate degradation and query failures. |
| packages/common-utils/src/core/renderChartConfig.ts | Exposes the rendered builder response layout for alert metadata consumers. |
| packages/common-utils/src/core/searchChartConfig.ts | Centralizes alert-window bounds and search-order resolution used by app and server paths. |
| packages/common-utils/src/filters.ts | Adds shared rendering for typed equality predicates, including NULL values. |
| packages/api/src/tasks/checkAlerts/tests/checkAlerts.int.test.ts | Adds broad regression coverage for alert parsing, group keys, sample constraints, and event identity. |
| packages/api/src/tasks/checkAlerts/tests/renderAlertTemplate.int.test.ts | Tests saved-search filters, typed group predicates, fail-closed behavior, and independent sample-query failures. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Saved alert configuration] --> B[Shared search-config builder]
B --> C[Alert evaluation query]
C --> D[Response-layout metadata]
D --> E[Alert value and group fields]
E --> F[History and threshold evaluation]
E --> G[Supported group predicates]
B --> H[Notification sample query]
G --> H
H --> I[Filtered sample events]
G -->|No safe predicate| J[Samples unavailable marker]
Reviews (38): Last reviewed commit: "fix: keep grouped alert event IDs stable" | Re-trigger Greptile
Deep Review✅ No critical issues found. No P0/P1 defects: escaping in 🟡 P2 — recommended
🔵 P3 nitpicks (5)
Reviewers (11): correctness (self-verified), security, reliability, performance, testing, api-contract, maintainability, project-standards, agent-native, previous-comments, learnings-researcher. Testing gaps:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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. |
|
Addressed the latest deep-review follow-ups in fc3ff70: the explicit source-order test now uses |
|
Added the requested trace-source regression in 22378f0. It verifies saved-search filter propagation and source ordering on the trace sample path; the focused suite now passes 100 tests and 72 snapshots. |
|
Addressed the final review findings in d6ddd51. Group sample predicates now use shared equality-filter rendering, preserve numeric and NULL values through dispatch, and cover expression keys plus escaping. Resolver tests now live in common-utils, and the app delegates ordering entirely to the shared resolver. Focused suites, full lint/typecheck, and the full unit gate pass. |
|
Addressed the fresh review findings in 84e356b. Boolean and numeric group values now produce type-correct sample predicates, non-scalar values fail visibly, numeric group-by columns are separated from numeric alert values using response metadata, and sample-fetch failures emit a counter plus an explicit notification marker. The shared type, redundant branch, and app changeset were also cleaned up. All four new inline threads are resolved; focused common-utils tests, build, API typecheck, lint, and the isolated app rerun pass. |
|
Closed the remaining low-severity review items in f799802. Response-metadata coverage now pins multiple projected group columns including a numeric group, selectGroupBy=false is guarded, and non-stringified numeric equality filters remain numeric. The focused common-utils suites now pass 250 tests, with API typecheck and the repository ratchet also passing. |
|
Addressed the latest Claude edge cases in d1f35d6. Histogram value-last metadata now fails safe without disabling alert evaluation; non-scalar group filtering is inside the guarded sample path so notifications still render; group data has one raw source of truth; and the unused helper mode plus unreachable branch are removed. All five threads are resolved. Focused common-utils tests, build, API typecheck, formatting, and the quality ratchet pass. |
2a426c2 to
bcb72a9
Compare
|
Closed the latest review findings in bcb72a9. Alert response columns are now classified by configured group names instead of projection position, so numeric histogram groups cannot replace the actual metric value. Unsupported non-scalar group values are isolated per key with separate observability while valid group predicates still constrain samples. I also added a processAlert integration regression that verifies each firing group reaches its own sample query. All three new threads are resolved; lint, typecheck, 106 alert-template tests / 72 snapshots, and 11 focused metadata tests pass. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Completed all review follow-ups through All current review threads are resolved. The full lint/ratchet/OpenAPI gate passes; common build, API typecheck, 249 common tests, 108 alert-template tests / 72 snapshots, and 15 metadata tests pass. |
0fe8575 to
df6e105
Compare
|
Rebased this onto current On the rebased head, |
|
Reviewed the rebased deep-review findings. The grouped-resolution P2 does not apply: resolved alerts take the dedicated The migration-wording point was valid. |
| // cleaning this up next. fireChannelEvent guards against null values | ||
| // for these properties. | ||
| const grouping: AlertNotificationGrouping = | ||
| hasGroupBy && group !== '' |
There was a problem hiding this comment.
🔵 minor — isGroupedAlert narrowed to "non-empty group", silently changing the webhook eventId for zero-fill alerts
isGroupedAlert used to be hasGroupBy (diff: - isGroupedAlert: hasGroupBy); it is now hasGroupBy && group !== ''. template.ts:645-654 feeds view.isGroupedAlert into the objectHash that produces eventId, which transports/generic.ts:89 exposes as the notification idempotency/dedup key. A grouped alert that fires on the empty-group zero-fill path (index.ts:1691-1703, exercised by the new uses one eventId when an empty grouped bucket alerts and resolves test) hashed with isGrouped: true before this change and isGrouped: false after, so an incident opened pre-upgrade is never closed by the post-upgrade resolve. Either keep hashing hasGroupBy (pass it separately from the grouping union) or add this to the .changeset/bright-alert-samples.md transition note alongside the group-key migration.
|
|
||
| for (const checkData of checksData.data) { | ||
| const bucketStart = new Date(checkData[meta.timestampColumnName]); | ||
| const timestampValue = checkData[meta.timestampColumnName]; |
There was a problem hiding this comment.
🔵 minor — New NULL/boolean/array group handling is unreachable per the declared row type
parseAlertData/getResponseMetadata were widened to AlertQueryRow = Record<string, unknown> and the diff adds real behaviour for non-string|number values — IS NULL predicates (filters.ts:118), toString(IsError) IN ('true'), unsupportedKeys for arrays, and this timestampValue == null skip. But the only production feed, ClickhouseClient.queryChartConfig (packages/common-utils/src/clickhouse/index.ts:624), still declares Promise<ResponseJSON<Record<string, string | number>>> while internally returning resp.json<any>(), so TypeScript treats every one of those branches as dead and only the direct-call unit tests reach them. Widen queryChartConfig's return to ResponseJSON<Record<string, unknown>> so the type matches what ClickHouse actually returns and the new handling is type-visible to future readers.
| // Builder SELECT order ends with the rendered time bucket, after any group | ||
| // columns (which may themselves be Date/DateTime). Raw SQL keeps its | ||
| // established first-date-column convention because its shape is user-defined. | ||
| const timestampColumnName = isBuilderChartConfig(chartConfig) |
There was a problem hiding this comment.
🔵 minor — The raw-SQL half of the new first-vs-last date-column split is untested
timestampColumnName now diverges by config kind — dateColumns.at(-1) for builder configs, dateColumns[0] for raw SQL — and the value-column exclusion is likewise gated on isBuilderChartConfig. Every new getResponseMetadata test goes through makeMetadataChartConfig, which always produces a builder config, and the existing raw-SQL fixtures (RAW_SQL_ALERT_TEMPLATE, fixtures.ts:803) project exactly one date column, so nothing distinguishes first from last on the raw-SQL branch. Add a getResponseMetadata case with a raw-SQL config whose meta has two date columns (e.g. ts then min(Timestamp) AS firstSeen) asserting timestampColumnName === 'ts', plus one asserting a numeric group column is still treated as the value column, so the deliberately-preserved raw-SQL behaviour documented in the changeset is pinned.
Hi @ryux1 , is this change required in this PR? It seems unrelated to #3059 , and in general this PR seems to make a lot of changes beyond the scope of the issue. |
|
Superceded by #3106 |
2 participants
Summary
Alert notification sample events were built separately from the saved-search chart configuration, so filters used to calculate the alert count were omitted when fetching example events. This reuses the shared search-config builder for the sample query and carries saved-search filters through. Grouped alerts constrain examples to the firing group through shared equality-filter rendering, including expression/Dynamic fields and string, numeric, boolean, or NULL values. Unsupported values and response columns that cannot be mapped safely are omitted individually; valid predicates still apply, and partial degradation has its own metric, log message, and notification marker outside the sample-results code block. If no group predicate can be applied safely, the notification now says that samples are unavailable instead of showing unrelated cross-group events. A failed ClickHouse sample query remains separately observable and does not prevent the notification from firing.
For builder alerts, response-tail group identification separates projected values from group columns without assuming one response column per configured select. Numeric group columns therefore remain in the persisted group-history key instead of replacing the selected alert value, including for saved-search and tile alerts when ratios collapse multiple selects or formulas change the emitted value shape. The renderer now exposes the group-projection layout it uses, and the alert metadata consumer reads that shared contract for disabled, scalar, and packed-histogram projections. This corrects the prior mis-evaluation; existing numeric-group alerts will establish the corrected group-history key on their first evaluation after deployment. An already-firing alert may emit one resolution for its legacy key during that transition: the old key is empty for a numeric-only group or lacks the numeric dimension for a composite group. Alerts requiring multiple consecutive windows must then accumulate the corrected key for the remaining windows before firing again. Raw SQL keeps its existing last-numeric-column behavior because its result shape is user-defined.
A separate saved-search metadata mapping restores configured expressions only for notification sample predicates. Direct columns and ClickHouse-rewritten string expressions are mapped safely; Date/DateTime groups and ambiguous rewritten numeric expressions degrade visibly instead of silently producing empty or cross-group samples. Grouped-only metadata is represented as a discriminated view, so ungrouped notifications cannot accidentally carry sample-filter attributes. The sample window remains aligned with alert evaluation, and search ordering is resolved by one common utility shared by app and server paths.
Regression tests cover saved and source predicates, expression-based group keys, escaping and typed values, NULL handling, partial and wholly unsupported group filtering, unmapped numeric and DateTime groups, numeric-group threshold selection and history keys, collapsed ratio projections, date-typed group versus time-bucket selection, disabled and packed group projections, separate sample-query failure reporting, value-column name collisions, and the ungrouped path. Docker-backed
processAlertregressions exercise bracket-access grouping, numeric group projection, collapsed grouped ratios, and stable alert/resolve event IDs for a synthetic empty group through the real renderer and ClickHouse metadata.How to test on Vercel preview
N/A — non-UI change.
Testing
yarn workspace @hyperdx/common-utils jest --runInBand src/__tests__/filters.test.ts src/core/__tests__/searchChartConfig.test.ts(249 tests)yarn workspace @hyperdx/common-utils jest --runInBand src/__tests__/renderChartConfig.test.ts(190 tests, 54 snapshots)yarn workspace @hyperdx/common-utils ci:buildyarn workspace @hyperdx/api tsc --noEmitmake ci-lintyarn workspace @hyperdx/api jest --config jest.int.config.js --runInBand --ci --forceExit src/tasks/checkAlerts/__tests__/renderAlertTemplate.int.test.ts(109 tests, 72 snapshots)yarn workspace @hyperdx/api jest --config jest.int.config.js --runInBand --ci --forceExit src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts -t parseAlertData(19 tests)make ci-unit(all 5 project suites plus the release-notes and changeset-hash suites passed on the rebased head)The integration suites listed above passed before the base reconciliation. On the rebased head, the shared-library build, TypeScript validation, full unit suite, lint gates, and focused 439-test common-utils regression set all pass. After the final event-ID follow-up, API TypeScript validation, API lint (300 baseline warnings, no errors), Prettier, and
git diff --checkpass. The focused integration command cannot start on this host because ClickHouse is unavailable at127.0.0.1:18123; Docker is also unavailable, so hosted integration CI remains authoritative for that regression.References