Repository navigation
fix(mcp): make event_patterns drill-down snippets executable - #3426
Conversation
…icates whereSnippet is now a SQL match() predicate that keeps token order and wildcard positions and includes the caller filter and source table filter. Also: JSONExtractString bodyExpression support, sample coverage and trend bucket metadata, tableFilterExpression applied to pattern mining, and minePatterns reports each cluster's own template.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2340b5c 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 |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Additional context: touches the query rendering engine lightly (8 lines, under the 150-line bar for Tier 4) Review process: Full human review — logic, architecture, edge cases. Stats
|
Duplicate code analysis2 new clones introduced by this PR.
If one of these duplicates a helper that already exists, import it instead jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review
4 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 3 minor 4 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
 [Medium impact] The PR appears safe to merge, with a non-blocking drill-down issue for long templates.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 462 passed • 1 skipped • 1493s
Tests ran across 4 shards in parallel. |
…plate change denoise compared minePatterns templates with the template match() returns, which now differ when match() lands a member on another same-length cluster. Build the noisy set from the clusters match() returns for noisy members instead. Also accept doubled-quote JSON keys, count drill-down filter render failures, and type the event pattern int-test helpers.
Deep ReviewScope: PR #3426 — Intent: Turn the ✅ No critical issues found. The SQL-injection surface is the riskiest part of this change and it holds up: 🟡 P2 — recommended
🔵 P3 nitpicks (4)
Reviewers (10): correctness, security, adversarial, reliability, performance, api-contract, testing, maintainability, project-standards, previous-comments. Testing gaps: no test exercises the No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
… reporting after query errors Matching rows against the clusters that noisy members re-match removed whole rare clusters (greptile P1). A row is now noisy when its body fits a noisy template token by token. Also stop counting drill-down render failures caused by the client closing after the queries failed.
| } | ||
|
|
||
| // Drain tokenizes with trim + split(/\s+/). | ||
| function tokenize(text: string): string[] { |
There was a problem hiding this comment.
🔵 minor — The 'body fits template' logic now exists in three places, with no shared helper
The rule "same token count, literal tokens equal, any token at <*>" is now implemented three times: tokenize/buildNoisyBodyMatcher in denoise.ts, templateToRegex in patternDrilldown.ts, and the fits() helper in mine-patterns.test.ts. PATTERN_WILDCARD is also declared separately in denoise.ts:19 and patternDrilldown.ts:13. tokenize copies Drain.getContentAsTokens (packages/common-utils/src/drain/drain.ts:104), and '<*>' copies Drain's paramStr. Fix: export a templateFits(template, body) helper and the wildcard constant from @hyperdx/common-utils/dist/drain, built on Drain's tokenizer. Use it in denoise and the test, and take the constant in patternDrilldown, so the three cannot drift apart.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
A broad noisy template like '<*> <*>' removed every same-length row, including rows of rare specific clusters. Pick the fitting template with the fewest wildcards and drop the row only if it is noisy. Also test the drilldownUnavailable branch and the predicate length boundary.
| if (template.length > MAX_PREDICATE_TEMPLATE_LENGTH) return null; | ||
| return `match(${valueExpression}, '${escapeSqlString(templateToRegex(template))}')`; |
There was a problem hiding this comment.
The 4000-character limit checks the template, not the generated SQL. A template with 1500 short tokens can be about 3000 characters long, but repeating the whitespace expression between tokens makes its filter much larger. Once the response exceeds 50 KB, trimToolResponse can replace whereSnippet with an object. The pattern has no drilldownUnavailable warning, but passing its filter to clickstack_search fails.
Bound the generated filter size or preserve executable fields when trimming.
| /** AND together SQL conditions, skipping blanks. */ | ||
| export function andSql(...conditions: Array<string | undefined>): string { | ||
| const parts = conditions.filter( | ||
| (c): c is string => c != null && c.trim() !== '', |
There was a problem hiding this comment.
🔵 minor — andSql re-implements the existing joinConditions helper
packages/api/src/tasks/checkAlerts/template.ts:125 (joinConditions) already does this: trim, drop blanks, return a lone condition bare, and wrap two or more in parens joined by AND. Move that helper to packages/api/src/utils/ (or common-utils) and import it in both places. The REQUIRED DRY rule in the conventions forbids a second copy.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
jordan-simonovski
left a comment
jordan-simonovski
left a comment
There was a problem hiding this comment.
Approved with some minor maintainability suggestions
| 'add them again. Skip patterns that have drilldownUnavailable.', | ||
| }; | ||
|
|
||
| const { data: trimmedOutput, isTrimmed } = trimToolResponse(output); |
There was a problem hiding this comment.
OOC can a long pattern lose its whereSnippet when the response is trimmed?
Every token boundary adds about 30 characters of whitespace class to the snippet, so a template of around 150 tokens produces a snippet of several KB.
When the response is over 50k (long sample bodies do this quickly), trimToolResponse splits the budget per key. That means a pattern with big samples can come back with whereSnippet: { __hdx_trimmed: true }. Would trimming the sample bodies before this call keep the snippet intact? They're only there for display.
samples: p.samples.map(row => truncate(String(row.__hdx_pattern_body ?? ''), MAX_SAMPLE_CHARS)),There was a problem hiding this comment.
Good catch, yes: once the per-key budget kicks in, a long whereSnippet can come back as { __hdx_trimmed: true }. Rather than special-casing samples here, this is going to be fixed in trimToolResponse itself, which will shorten strings with a suffix and drop or shrink lower-priority paths like patterns[].samples first: #3542 (comment)
| * predicate in one clickstack_search `where`. Dashboard variables are | ||
| * expanded first because the search tool has no variable context. | ||
| */ | ||
| export async function userWhereAsSql({ |
There was a problem hiding this comment.
I wonder if userWhereAsSql could call the same Lucene to SQL step the mining query uses?
const expanded = variables ? substituteVariables(where, { variables, inputLanguage: whereLanguage }) : where;
return renderWhereExpressionStr({ condition: expanded, language: whereLanguage, metadata, from: { databaseName, tableName }, connectionId, implicitColumnExpression, useTextIndexForImplicitColumn });There was a problem hiding this comment.
Done in 735fa6c. renderWhereExpressionStr is now exported from common-utils and userWhereAsSql calls it after expanding variables, so the drill-down and the mining query render the filter the same way (including the materialized-column rewrite).
| }; | ||
| } | ||
|
|
||
| const tableFilterSql = |
There was a problem hiding this comment.
I feel like I'm piling on here, lmao.
Could the table filter come from the helper that buildSearchChartConfig uses?
searchChartConfig.ts exists so the search page, alert preview and alert task all apply tableFilterExpression the same way. A getSourceTableFilters(source) export next to buildSearchChartConfig would keep one copy.
export const getSourceTableFilters = (source: TSource): Filter[] =>
isLogSource(source) && source.tableFilterExpression ? [{ type: 'sql', condition: source.tableFilterExpression }] : [];There was a problem hiding this comment.
Done in 735fa6c. Added getSourceTableFilters(source) next to buildSearchChartConfig, which uses it too, and mining plus the drill-down take the table filter from it.
…ering userWhereAsSql now calls renderWhereExpressionStr, the same Lucene-to-SQL step the mining query uses, instead of building its own serializer. The source table filter comes from a new getSourceTableFilters helper that buildSearchChartConfig also uses.
…n-tweaks' into brandon/brandon-mcp-event-pattern-tweaks
| // Read the template from the row's own cluster: re-matching the body can | ||
| // land on a different same-length cluster whose template other members | ||
| // of this one do not fit. | ||
| const cluster = miner.drain.getCluster(clusterId); |
There was a problem hiding this comment.
🔵 minor — The template fix misses the CLI, which still re-matches the first sample to get its template
packages/cli/src/cli.tsx:2321-2322 still builds each group's template with miner.match(g.sample, 'fallback')?.getTemplate(). That is the bug this PR fixes in minePatterns: the CLI can still report a different same-length cluster's template. Have it use miner.drain.getCluster(clusterId)?.getTemplate(), or call minePatterns directly.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| // `\s` is ASCII-only. JS `\s` = RE2 `\s` + \v + \pZ (Zs/Zl/Zp) + BOM. `\pZ` | ||
| // keeps the snippet short; spelling out the code points repeats ~100 chars | ||
| // between every token. | ||
| const JS_WHITESPACE = '\\s\\x{0B}\\pZ\\x{FEFF}'; |
There was a problem hiding this comment.
🟠 major — Repeating the whitespace class between every token makes whereSnippet large enough that trimToolResponse drops patterns
Each separator is [\s\x{0B}\pZ\x{FEFF}]+. That is 22 characters, 26 after escapeSqlString and about 34 once JSON-encoded. Each <*> costs a similar amount, and every snippet also repeats the full user/table filter. A typical 30-token log line therefore produces a snippet of about 1.3 kB, where the old Lucene snippet was about 100 bytes. With the default topN of 20, plus samples and 24 trend buckets per pattern, the output passes the 50 kB budget in packages/api/src/utils/trimToolResponse.ts. trimObject then gives patterns about 16.6 kB, and trimArray cuts the array 20→14→10, so callers silently get half the patterns they asked for. Suggested fix: normalize the separators once in SQL and keep the per-token regex ASCII-only, e.g. match(replaceRegexpAll(expr, '[\x{0B}\pZ\x{FEFF}]', ' '), '^\s*Sending\s+\S+\s+rows\s*$'). Also put the shared drilldown.where in summary.drilldown once instead of repeating it in every pattern, or measure the snippet size against the trim budget.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| /** AND together SQL conditions, skipping blanks. */ | ||
| export function andSql(...conditions: Array<string | undefined>): string { | ||
| const parts = conditions.filter( | ||
| (c): c is string => c != null && c.trim() !== '', |
There was a problem hiding this comment.
🔵 minor — andSql re-implements the existing joinConditions helper
joinConditions in packages/api/src/tasks/checkAlerts/template.ts:125 already does this: trim, skip blanks, return a lone condition unwrapped, and wrap each part in parentheses when ANDing two or more. Move it into packages/api/src/utils/ (or common-utils), export it, and use it here instead of adding a second copy. The repo's DRY rule (REQUIRED) calls for this.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
|
||
| import { resolveBodyExpression } from './helpers'; | ||
|
|
||
| const PATTERN_WILDCARD = '<*>'; |
There was a problem hiding this comment.
🔵 minor — Two separate implementations now define when a body fits a template, each with its own wildcard constant and tokenizer
PATTERN_WILDCARD is declared in both denoise.ts and patternDrilldown.ts:10. Drain already has the default paramStr: '<*>' in packages/common-utils/src/drain/drain.ts:26. denoise's tokenize restates Drain.getContentAsTokens (drain.ts:104), and templateToRegex splits the template a third way. Since buildNoisyBodyMatcher (JS) and templateToRegex (RE2) must agree on what 'fits' means, export the wildcard constant and a tokenize/fitsTemplate helper from common-utils/src/drain and build both on them, so the two cannot drift apart.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
|
|
||
| const filters = getSourceTableFilters(source); | ||
| const tableFilterSql = andSql( | ||
| ...filters.map(f => ('condition' in f ? f.condition : undefined)), |
There was a problem hiding this comment.
🔵 minor — The 'condition' in f guard can never be false
getSourceTableFilters (packages/common-utils/src/core/searchChartConfig.ts) only ever builds { type: 'sql', condition } filters, so the undefined branch here can never run. The guard exists only because the return type is the wide Filter[]. Give getSourceTableFilters a narrower return type (e.g. Extract<Filter, { type: 'sql' }>[]) and map f => f.condition directly.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Take ClickHouse#3426's drill-down predicate in runEventPatterns.ts; drop this PR's buildWhereSnippet and its test, which ClickHouse#3426 supersedes.
…n the drill-down regex ClickHouse#3426's predicate matches a whole-token <*> as exactly one word and escapes anything else literally, so a masked timestamp that spans spaces (2026-11-04 06:12:12,345) or a mask inside a token ([<*>], trace_id=<*>) matched nothing. Named masks keep Drain's clustering as it is (a mask is one token that is equal across lines), and the regex maps <TS> to any text and <ID> to one word, also inside a token. Whole-token <*> is unchanged.
Summary
clickstack_event_patternsreturned awhereSnippetper pattern that agents were told to pass toclickstack_search, but it often matched nothing. A template likeSending <*> rowsbecameBody:"Sending rows"(a phrase that never occurs), and map-field body expressions produced Lucene that failed to parse. Agents hit dead ends on the main drill-down path.Changes:
whereSnippetis now a SQLmatch()predicate (whereLanguage: "sql") built from the template: literal tokens in order, one non-whitespace token per<*>, whitespace matching Drain's JS\stokenization. It already ANDs in the caller'swhere(rendered to SQL, dashboard variables expanded) and the source's table filter. If the caller filter can't be rendered as SQL,summary.drilldown.includesFiltersisfalsewith a warning. Templates over 4000 chars getdrilldownUnavailableinstead of an empty snippet that would match everything.event_patterns,emerging_signals, dashboard pattern tiles) now applies the log source'stableFilterExpression. Counts can drop for sources that set one.bodyExpressionalso acceptsJSONExtractString(<column or map ref>, '<key>', ...). It is rebuilt from validated args, so the injection allowlist still holds.sampleCoverage, andsummary.trendgives the real bucket interval and boundaries (granularity rounding means fewer or wider buckets than requested). Tool description and usage text explain that counts are extrapolated and a 0 bucket doesn't mean no events.minePatternstemplate bug. Each cluster now reports its own template. Before, it re-matched the first member's body and could pick up a different same-length cluster's template that other members didn't fit, so the drill-down missed some of the pattern's own events.How to reproduce
Starting point:
yarn dev, MCP client connected to the local API, a log source with data.On
main:clickstack_event_patternson a log source.<*>aswheretoclickstack_search. It returns no rows, or a parse error whenbodyExpressionis a map field.On this branch:
whereSnippetandwhereLanguage: "sql".clickstack_searchwithwhereLanguage: "sql". It returns that pattern's events.Covered by new unit tests (
patternDrilldown.test.ts,mine-patterns.test.ts,query.test.ts) and integration tests inqueryTool.int.test.ts/queryTile.int.test.tsthat run each snippet through search.References
HDX-5748