Skip to content

feat(common-utils): share the Event Patterns drill-down predicate - #3662

Open
alex-fedotyev wants to merge 3 commits into
mainfrom
alex/FR-385-shared-pattern-predicate
Open

alex-fedotyev wants to merge 3 commits into
mainfrom
alex/FR-385-shared-pattern-predicate

Conversation

@alex-fedotyev

@alex-fedotyev alex-fedotyev commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

Adds the Event Patterns drill-down predicate that the MCP server builds for clickstack_event_patterns to @hyperdx/common-utils/dist/drain, so the app can build the same filter. Part of FR-385: filter for or exclude a pattern in one click from Event Patterns.

Stack

Part 1 of 4. Each PR is based on the one before it, so its diff shows only its own step. Review in order:

  1. feat(common-utils): share the Event Patterns drill-down predicate #3662 Share the pattern predicate in common-utils (this PR)
  2. refactor(mcp): use the shared pattern predicate for drill-downs #3663 MCP uses the shared predicate
  3. feat(common-utils): add a compact form of the pattern predicate #3664 Compact form of the predicate, short enough for a URL
  4. feat(app): filter for or exclude a pattern from Event Patterns #3665 Filter for or exclude a pattern from Event Patterns

Please merge in order: once a PR merges, the next one is retargeted to main. CI and the tier label only run on PRs whose base is main, so PRs 2 to 4 get them at that point. Until then the test plan lists what I ran locally.

Summary

The app mines patterns in the browser with Drain3 and all ASCII punctuation as extra delimiters. MCP mines with the TypeScript port and splits on whitespace only, so its regex does not fit browser templates: a browser template looks like GET api v1 users <*> <*> <*> for events like GET /api/v1/users/123 200 15ms.

  • templateToRegex and buildPatternPredicate take an optional extraDelimiters. Each delimiter joins the separator class, since Drain turns each one into a space before splitting. A delimiter longer than one character throws, because Drain splits on it as a whole string and a character class can't.
  • whitespace: 'python' matches templates from Python Drain3, whose str.split() also splits on \x1c to \x1f and NEL but not on BOM; JS \s is the other way round.
  • When every ASCII punctuation character is a delimiter, the class uses RE2's [:punct:] instead of repeating 32 characters between every token. ASCII_PUNCTUATION is exported so the app builds its delimiter list from it.
  • parsePatternPredicate reads a predicate back into its value expression and template. It only accepts what these functions built, so a hand-written match() is not taken for a pattern. The app uses it to show pattern filters as pills (feat(app): filter for or exclude a pattern from Event Patterns #3665).
  • MAX_PREDICATE_TEMPLATE_LENGTH comes along unchanged.

Without extraDelimiters the output is byte-identical to the MCP copy, which #3663 deletes.

#3551 also edits templateToRegex in the MCP copy (date masks). Whichever of the two lands second carries that change into packages/common-utils/src/drain/pattern-predicate.ts.

Test plan

  • unit: pattern-predicate.test.ts pins the MCP output format, checks punctuation delimiters (including ], \, ^ and -, which are special inside a character class), Python whitespace, multi-character delimiters and the length limit, checks that each message mined with the same delimiters matches its own cluster's template and no other, and round-trips predicates through parsePatternPredicate with and without options (and rejects them under the other options)
  • yarn ci:lint and yarn ci:unit in packages/common-utils (47 suites, 3,158 tests)
  • knip, yarn dupes (no new duplicate code)

templateToRegex and buildPatternPredicate build the whereSnippet that the
MCP clickstack_event_patterns tool returns for each pattern. They live in
the MCP server today. This adds them to @hyperdx/common-utils/dist/drain so
the app can build the same filter.

The app mines patterns with all ASCII punctuation as extra delimiters, so
the separator class gets an extraDelimiters option. When every ASCII
punctuation character is a delimiter, the class uses RE2's [:punct:]
instead of listing 32 characters between every token.
parsePatternPredicate reads a predicate back into its value expression and
template. It only accepts predicates these functions built, so a
hand-written match() is not mistaken for one.

Without extraDelimiters the output is byte-identical to the MCP copy. The
next change switches MCP over and deletes its copy.

Part of FR-385.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hyperdx-oss Ready Ready Preview Nov 13, 2026 11:44pm UTC
hyperdx-storybook Ready Ready Preview Nov 13, 2026 11:44pm UTC

Request Review

@changeset-bot

changeset-bot Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c2fd023

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@hyperdx/common-utils Minor
@hyperdx/api Patch
@hyperdx/app Patch
@hyperdx/otel-collector Patch

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

Copy link
Copy Markdown
Contributor

Duplicate code analysis

No new duplicate code introduced by this PR.


jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 108 in total (1.18%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

@github-actions github-actions Bot added the review/tier-2 Low risk — AI review + quick human skim label Nov 13, 2026
@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

🔵 Tier 2 — Low Risk

Small, isolated change with no API route or data model modifications.

Why this tier:

  • Standard feature/fix — introduces new logic or modifies core functionality

Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns.
SLA: Resolve within 4 business hours.

Stats
  • Production files changed: 2
  • Production lines changed: 144 (+ 182 in test files, excluded from tier calculation)
  • Branch: alex/FR-385-shared-pattern-predicate
  • Author: alex-fedotyev

To override this classification, remove the review/tier-2 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

PR Review

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

  • 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
  • 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
  • Anything about surrounding code, or asking you to widen the change (hoist a helper,
    dedupe with another file, fix other call sites): never fix here, whatever the
    severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor

1 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor
  • 🔵 packages/common-utils/src/drain/pattern-predicate.ts:342 — parsePatternPredicate accepts compound conditions: the greedy (.+) value expression swallows a leading match(...) AND → Take match(Body, 'timeout') AND match(Body, '<regex buildPatternPredicate built>'). The .+ group backtracks to the last , ', so the function returns valueExpression = "Body, 'timeout') AND match(Body" and the round-trip check passes, because it only checks the regex half. The app would then show this hand-written compound filter as a single pattern pill, which breaks the docstring's claim that only what this module built is accepted. Fix: reject a valueExpression whose parentheses or quotes are unbalanced, or that contains a top-level , ' (for example, scan for depth-0 commas outside string literals). Add this compound case to the 'rejects other conditions' test.

Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.

@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

Deep Review

Scope: packages/common-utils/src/drain/pattern-predicate.ts (new), packages/common-utils/src/drain/index.ts (exports), packages/common-utils/src/__tests__/pattern-predicate.test.ts (new), .changeset/shared-pattern-predicate.md. Pure TypeScript utility that builds and parses the Event Patterns drill-down match() predicate; moved from the MCP server to @hyperdx/common-utils.

✅ No critical issues found.

The regex construction, the escapeSqlString → parsePatternPredicate unescape round-trip, the [:punct:] separator-class optimization, and the js/python whitespace split all hold up under adversarial tracing. parsePatternPredicate is self-validating — it re-runs templateToRegex(template, options) and compares against the input regex, so a mis-split from the greedy (.+) value-expression capture or an imperfect unescape degrades to a null (false negative), never a false-positive match or a malformed predicate. Delimiter validation, the length limit, and option cross-rejection are all covered by tests, and all prior-review fixes claimed in 04c8a768a are present in the diff.

🟡 P2 -- recommended

  • packages/common-utils/src/__tests__/pattern-predicate.test.ts:16 -- every templateToRegex assertion runs the output through the hand-written toJsRegex translator and tests against JS RegExp, never against RE2 or ClickHouse, so a divergence between the generated RE2 string and ClickHouse's actual match() semantics (e.g. [:punct:] membership or \pZ/\x{85} handling) would pass the suite undetected.
    • Fix: add at least one assertion that pins the exact RE2 string output of templateToRegex for a punctuation-delimited template, independent of the toJsRegex proxy, and/or an integration test exercising the predicate against ClickHouse.
🔵 P3 nitpicks (2)
  • packages/common-utils/src/drain/pattern-predicate.ts:98 -- separatorClass is recomputed independently in templateToRegex and parsePatternPredicate; the two call sites must stay in lockstep for the self-check to pass, making it a latent consistency hazard (currently masked because parse re-derives via templateToRegex).
    • Fix: have parsePatternPredicate derive its separator/prefix/suffix from the same computed value templateToRegex uses, or add a comment noting the shared-derivation contract.
  • packages/common-utils/src/drain/pattern-predicate.ts:84 -- valueExpression is interpolated into the generated SQL unescaped while template is escaped via escapeSqlString; this is correct by design (the value expression is a trusted SQL column expression, not a string literal) and is carried over unchanged from the MCP copy, but the asymmetry is non-obvious.
    • Fix: add a one-line comment documenting that valueExpression is a trusted SQL expression and intentionally not escaped.

Reviewers (8): correctness, adversarial, security, testing, api-contract, maintainability, project-standards, previous-comments.

Testing gaps: Unit tests validate generated regexes only through the toJsRegex RE2→JS translation shim, not against RE2/ClickHouse directly; the translator itself is untested and is a single point of correctness risk for the whole suite.

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

@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown
Contributor

E2E Test Results

✅ All tests passed • 465 passed • 1 skipped • 1056s

Status Count
✅ Passed 465
❌ Failed 0
⚠️ Flaky 3
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

- whitespace: 'python' matches templates mined by Drain3, whose
  str.split() also splits on \x1C-\x1F and NEL but not on BOM.
- A delimiter longer than one character throws: Drain splits on it as a
  whole string, which a character class can't express.
- Export ASCII_PUNCTUATION so the app builds its delimiter list from it.
- Tests for the length limit, a round trip without options, and options
  that don't match in either direction.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alex-fedotyev

Copy link
Copy Markdown
Contributor Author

Review findings outside the inline threads, fixed in 04c8a76:

  • Multi-character delimiters: an entry longer than one character now throws, since a character class can't split on it the way Drain does.
  • Length limit: tested at and just over MAX_PREDICATE_TEMPLATE_LENGTH.
  • Options: a predicate built without options round-trips, and a predicate is rejected under the other options in both directions.

The new exports get their callers in #3663 and #3665.

With whitespace: 'python' the template was still split on JS \s, which also
splits on BOM. Drain3 keeps a BOM inside a token, so a template mined from a
body that starts with one gave a regex its own events failed. Also test a
round trip with quotes, backslashes and regex syntax in tokens.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alex-fedotyev

Copy link
Copy Markdown
Contributor Author

Second-round findings outside the inline threads:

That's the second round of automated findings, so per AGENTS.md I'm leaving any further ones for review.

'^[\\s\\x{0B}\\pZ\\x{FEFF}[:punct:]]*a[\\s\\x{0B}\\pZ\\x{FEFF}[:punct:]]*$',
);
const partial = { extraDelimiters: ['/', ':', '-', ']', '\\', '^'] };
const regex = toJsRegex(templateToRegex('a <*> c d e f g', partial));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 minor — New RE2 syntax is never run by RE2 or ClickHouse; toJsRegex rewrites it before testing

Every assertion runs a JS translation of the regex. The translation replaces [:punct:] with literal ranges, \x{..} with \u{..} and \pZ with \p{Z}. So a ClickHouse/RE2 problem with the new constructs would still pass: [:punct:] nested inside a larger bracket class, the \x{1C}-\x{1F} range, or \-/\[/\^ escapes in a class. Add a ClickHouse integration test (the package already has *.int.test.ts suites, e.g. src/__tests__/queryChartConfig.int.test.ts). It should run SELECT match(...) on a few bodies, with and without punctuation and Python whitespace.

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

This branch was successfully deployed

2 active deployments
Preview – hyperdx-oss — c2fd0232 Deployed Nov 13, 2026 by vercel[bot]
Preview – hyperdx-storybook — c2fd0232 Deployed Nov 13, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-2 Low risk — AI review + quick human skim

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant