Repository navigation
fix(mcp): make event_patterns drill-down snippets executable - #3426
brandon-pereira wants to merge 8 commits into
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: e261c8d 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:
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 |
| }; | ||
| } | ||
|
|
||
| const tableFilterSql = |
There was a problem hiding this comment.
🔵 minor — Re-implements the table-filter rule that buildSearchChartConfig already owns
buildSearchChartConfig in packages/common-utils/src/core/searchChartConfig.ts:134-137 already holds this rule: for a Log source with tableFilterExpression set, add [{ type: 'sql', condition }] to filters. Its doc says it exists so every path applies the filter the same way. Here, and in andSql(tableFilterSql, …), the rule is restated by hand with kind === SourceKind.Log && 'tableFilterExpression' in source. Pull a shared getSourceTableFilters(source) helper out of searchChartConfig.ts and use it in both places (or at least reuse isLogSource), so the two copies can't 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
PR Review
5 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 5 minor 1 posted as inline comment(s) on the changed lines. 4 listed below. Findings outside the changed lines4 minor
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.
 |
E2E Test Results✅ All tests passed • 344 passed • 1 skipped • 1359s
Tests ran across 3 shards in parallel. |
| }; | ||
| } | ||
|
|
||
| const tableFilterSql = |
There was a problem hiding this comment.
🔵 minor — Table-filter resolution duplicates buildSearchChartConfig's tableFilterExpression logic
packages/common-utils/src/core/searchChartConfig.ts:134 already turns a Log source's tableFilterExpression into [{ type: 'sql', condition }] behind the isLogSource guard. It is described as the single source of truth so that every path sees the same row set. Pull that logic into a shared exported helper, for example getSourceTableFilters(source), and call it here, instead of the hand-rolled source.kind === SourceKind.Log && 'tableFilterExpression' in source check. The two copies already differ slightly: one checks != null, the other checks truthiness. (REQUIRED DRY rule.)
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
…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.
| implicitColumnExpression?: string; | ||
| useTextIndexForImplicitColumn?: UseTextIndex; | ||
| }): Promise<string> { | ||
| if (!where.trim()) return ''; |
There was a problem hiding this comment.
🔵 minor — userWhereAsSql re-implements renderWhereExpressionStr, and the two can drift
renderWhereExpressionStr in packages/common-utils/src/core/renderChartConfig.ts:1277 already turns a Lucene where into SQL with CustomSchemaSQLSerializerV2 + SearchQueryBuilder. It also passes bodyExpression and applies the materialized-column rewrite. Mining goes through that function, but the drill-down goes through this copy, so the two SQL renderings of the same filter can differ. Export the common-utils function (or a thin wrapper around it) and call it here instead of building the serializer a second time.
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
Deep Review✅ No critical issues found. No P0/P1 ship-blockers. The injection surface (the main risk in this diff) was traced safe: 🟡 P2 — recommended
🔵 P3 nitpicks (7)
Reviewers (7): security, testing, maintainability, project-standards, performance, kieran-typescript, previous-comments. The correctness and adversarial reviewers did not return before synthesis; their surface was covered by direct verification (injection payload tracing, Drain-tokenizer consistency, Testing gaps:
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
| }; | ||
| } | ||
|
|
||
| const tableFilterSql = |
There was a problem hiding this comment.
🔵 minor — Rebuilds the table-filter rule that buildSearchChartConfig documents as its single source of truth
buildSearchChartConfig (packages/common-utils/src/core/searchChartConfig.ts:120-137) is documented as the one place that turns source.tableFilterExpression into a SQL Filter, so that every path sees the same rows. This PR writes a second copy, and it differs slightly: it uses a truthiness check where the original uses != null. Pull the table-filter construction out of searchChartConfig.ts into an exported helper (e.g. getSourceTableFilters(source)) and call it here, or build the sample and count configs with buildSearchChartConfig.
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.
| }; | ||
| } | ||
|
|
||
| const tableFilterSql = |
There was a problem hiding this comment.
🔵 minor — Table-filter assembly re-implements buildSearchChartConfig's tableFilterExpression logic
buildSearchChartConfig in packages/common-utils/src/core/searchChartConfig.ts:134-137 already turns a Log source's tableFilterExpression into the [{ type: 'sql', condition }] filter, and its doc comment calls itself the single source of truth so every path sees the same rows. This copy differs already: it uses || undefined where the original uses != null, so an empty string is handled differently. Move the tableFilter computation into an exported helper (e.g. getSourceTableFilters(source)) beside buildSearchChartConfig and call it from both places.
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 tableFilterSql = |
There was a problem hiding this comment.
🔵 minor — Table-filter rule copied by hand from buildSearchChartConfig, with a slightly different condition
buildSearchChartConfig (packages/common-utils/src/core/searchChartConfig.ts:134) already defines how tableFilterExpression becomes a SQL filter. Its check is != null, while this copy uses truthiness. Extract a shared getSourceTableFilters(source) from searchChartConfig.ts and use it here, so the deprecated filter is applied the same way on every path.
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
| * argument list (which may span lines, hence the `s` flag) for | ||
| * `normalizeBodyExpression` to split and validate argument by argument. | ||
| */ | ||
| const JSON_EXTRACT_STRING_CALL = /^JSONExtractString\((.*)\)$/s; |
There was a problem hiding this comment.
this stuff feels a bit complex - open to ideas
| 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.
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