Repository navigation
Conversation
- Mine the source's body column instead of the last table column. Adding an attribute column from the side panel switched denoise to that column, or broke the sample query when the column carried an alias. - Strip ANSI codes before matching, as sampling already did, so colored rows of a noisy pattern are removed. - Replace drain3's match() with the template-fit matcher the MCP denoise uses, moved to common-utils and shared by both. - Say in the panel when results can't be denoised. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 1cad23c 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 analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Mirrors Drain's tokenizer: delimiters become spaces, then split(/\s+/). | ||
| // Templates are already space-joined tokens, and splitting one on a | ||
| // delimiter would break up `<*>`. | ||
| function tokenize(text: string, extraDelimiters: string[]): string[] { |
There was a problem hiding this comment.
🔵 minor — tokenize copies Drain.getContentAsTokens (prior art in the same package)
Drain.getContentAsTokens in packages/common-utils/src/drain/drain.ts:104 already does this: trim, replace each extra delimiter with a space, then split(/\s+/). Pull that logic into one exported helper and have both Drain and buildNoisyBodyMatcher call it. That follows the REQUIRED DRY rule and keeps the matcher's tokens in step with the miner's tokens, which is the point of the extraDelimiters option.
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
| @@ -335,6 +335,7 @@ export function useGroupedPatterns({ | |||
| isLoading, | |||
| error, | |||
| miner: results?.miner, | |||
There was a problem hiding this comment.
🔵 minor — miner is returned but nothing reads it any more
The only consumer was matchLogs in DBRowTable, which this PR removes. Grep finds no other use of .miner in packages/app/src. Remove miner from the usePatterns result (line 206) and from the useGroupedPatterns return. Then the react-query cache stops holding a Pyodide miner object for every query.
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 { useGroupedPatterns } from '@/hooks/usePatterns'; | ||
|
|
||
| jest.mock('@/hooks/usePatterns', () => ({ | ||
| DRAIN_EXTRA_DELIMITERS: '!"#$%&\'()*+,-./:;<=>?@[\\]^_`{|}~'.split(''), |
There was a problem hiding this comment.
🔵 minor — Test mock pastes its own copy of DRAIN_EXTRA_DELIMITERS
The mock re-declares the delimiter list instead of using the real export. If the miner's delimiters change in usePatterns.tsx, this test still passes, even though matching would then disagree with the templates. Use jest.mock('@/hooks/usePatterns', () => ({ ...jest.requireActual('@/hooks/usePatterns'), useGroupedPatterns: jest.fn() })) instead, or move the constant to a module that doesn't need mocking.
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
| ) { | ||
| switch (skipped) { | ||
| case 'body_not_in_results': | ||
| return `Add ${bodyExpression} to the table columns to denoise results.`; |
There was a problem hiding this comment.
🔵 minor — Column name in the skip message is plain text, not Mantine <Code>
The code-snippet convention (REQUIRED) says short tokens in prose, such as a column name, use Mantine <Code>. Have skipMessage return a ReactNode, e.g. <>Add <Code>{bodyExpression}</Code> to the table columns to denoise results.</>.
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
3 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 2 minor 1 posted as inline comment(s) on the changed lines. 2 listed below. Findings outside the changed lines
1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep ReviewScope: PR #3621 — "Denoise results" reliability fix across ✅ No critical (P0/P1) issues found. 🟡 P2 — recommended
🔵 P3 nitpicks (3)
Reviewers (6): correctness, adversarial, testing, maintainability, kieran-typescript, performance. 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 |
E2E Test Results✅ All tests passed • 466 passed • 1 skipped • 1204s
Tests ran across 4 shards in parallel. |
Drain splits one logical pattern across clusters by token count and first token, so on the demo logs no cluster passed 10% and denoise removed nothing. Take the most frequent patterns until they cover half the sample, ignoring patterns under 1%. Applies to the web app and the MCP denoise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teeohhem
changed the title
|
|
||
| // Python's string.punctuation. Shared with the denoise matcher, which must | ||
| // split bodies into the same tokens the miner built its templates from. | ||
| export const DRAIN_EXTRA_DELIMITERS = |
There was a problem hiding this comment.
🔵 minor — DRAIN_EXTRA_DELIMITERS lives in the app hook, so the test mock has to copy it
The constant belongs next to buildNoisyBodyMatcher in common-utils/src/drain, since the matcher depends on it. Right now useDenoisedRows.test.tsx:12 retypes the whole punctuation string inside jest.mock, so the test checks a copy that can drift from the real value. Move it to common-utils and import it from there in usePatterns, useDenoisedRows and the test.
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
- Never hide a pattern whose sampled lines include an error or warning,
so a flood of errors during an incident stays visible.
- Replace the raw template list with a one-line summary ("Hiding N
common patterns, about X% of events") and a collapsed list. Each
pattern is shortened to one line, with runs of wildcards collapsed to
an ellipsis and its share of events.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| @@ -335,6 +335,7 @@ export function useGroupedPatterns({ | |||
| isLoading, | |||
| error, | |||
| miner: results?.miner, | |||
There was a problem hiding this comment.
🔵 minor — miner is still kept and returned, but nothing uses it now that matchLogs is gone
No caller reads useGroupedPatterns().miner any more (grep finds only the test mock). Drop miner from mineEventPatterns, the query result and the hook's return value. Also del the m_<id> Python global after getTemplates(), so each search stops leaving a 10k-log TemplateMiner behind in the pyodide globals.
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
Summary
"Denoise results" on the search page often left noisy rows in the table. Three bugs caused it:
patternColumn = columns[columns.length - 1]. Adding a column from the side panel appends it, so denoise switched to that attribute. An added map column carries an alias (LogAttributes['k'] AS "k"), and the alias was then used as a SQL expression in the sample query. Denoise now uses the source's body expression (getEventBody), finds its result column by position in the select list, and says so in the panel when the body isn't in the table.match(). That needs an exact template fit and can put a row in a different cluster from the one it was counted in. The MCP denoise already replaced this withbuildNoisyBodyMatcher. That matcher moves tocommon-utils/src/drain/noisy-matcher.ts, gains anextraDelimitersoption so it tokenizes like the web miner, and is now used by both.The denoise logic moves out of
DBRowTable.tsxintohooks/useDenoisedRows.tsandcomponents/DenoiseSummary.tsx. If the pattern query fails, the table now shows all rows instead of an empty table.Noise selection. A pattern used to count as noisy only if its cluster held more than 10% of the 10k-row sample. Drain splits one logical pattern across clusters by token count and first token, so on the demo logs no cluster passed 10% (the top one was 9.2%) and denoise removed nothing.
selectNoisyPatterns(common-utils) now takes the most frequent patterns until they cover half the sample, ignoring any under 1%. On the demo data's 6-hour window, that removes 9 patterns covering 53% of the sample. The MCPdenoiseoption uses the same rule, and its tool description is updated. Its integration test still passes: the seeded health-check pattern is 94% of the sample and is the only pattern removed.Errors and warnings are never hidden. Frequency isn't importance: during an incident a flood of errors becomes the most common pattern. The app skips any pattern whose sampled lines include an error or warning (
getLogLevelClass). The MCP denoise doesn't apply this yet, because its sample query doesn't select the level.Readable panel. The raw template list is replaced by a one-line summary ("Hiding 8 common patterns, about 53% of events. Errors and warnings are never hidden.") with a "Show patterns" toggle. Each pattern is one truncated line with its share of events, and runs of wildcards collapse to
…(simplifyPattern). Hover shows the full template.Screenshots or video
Screenshots need to be attached by the human author. The "after" view on the preview shows the summary line, "Show patterns" expanded to 8 one-line patterns with percentages, and error rows ("Failed to place order", "Visa cache full") still in the table.
How to reproduce
Starting point:
yarn app:dev:localconnected to the demo server (onboarding → "Connect to Demo Server").On
main:On this branch:
Bodycolumn from the select. The panel reads "Add Body to the table columns to denoise results." and all rows are shown.How to test on Vercel preview
Preview routes: /search
Steps:
Timestamp, ServiceNameand run the search.References
buildNoisyBodyMatcher)🤖 Generated with Claude Code