Repository navigation
feat: add native NetFlow monitoring with Lucene search and Sankey - #3347
valerypetrov wants to merge 11 commits into
Conversation
🦋 Changeset detectedLatest commit: 219da28 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 |
|
@valerypetrov 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 @valerypetrov, 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. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review
3 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. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. No P0/P1 defects surfaced from the completed reviewers or from direct inspection of the highest-risk paths. SQL injection is not present — filter values pass through 🟡 P2 -- recommended
🔵 P3 nitpicks (3)
Reviewers (5): api-contract, performance, previous-comments, learnings-researcher, orchestrator deep-inspection (security/correctness/SQL-escaping/Sankey/local-mode spot-checks). Testing gaps:
Coverage note: The correctness, security, adversarial, reliability, testing, maintainability, project-standards, kieran-typescript, and agent-native reviewers had not returned results at synthesis time; their lanes were partially covered by direct orchestrator inspection but a full pass on those dimensions did not complete. Treat the "no critical issues" verdict as grounded in the completed reviewers and the highest-risk spot-checks, not an exhaustive multi-persona sign-off. 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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed this review round in 9053fcb:
Validation: all app/API/common-utils unit suites passed, plus 58 MCP integration tests, ClickHouse query checks, and browser workflows covering source changes, search, filters, Sankey, axis layout, and time-range recovery. One app suite needed a rerun after a concurrent shared build temporarily removed its dependency; it passed. Repository lint/type/style checks, lint-staged, and Knip passed. For the remaining suggestions, following the repository's review scope rules:
SQL escaping already has unit and real ClickHouse coverage. Added explicit partial-credential routing cases, and documented why NetFlow uses an empty known-column set while restoring exact SQL expressions during serialization. |
|
Addressed the second review round in dee8414:
Validation: 110 backend integration tests and 72 snapshots passed, including real local ClickHouse/Mongo alert evaluation; 43 alert unit tests and 9 focused app tests passed. Both local browser workflows passed, covering drafts, source creation/switching, invalid ranges, search, autocomplete, records, and click filters. Repository lint/type/style/OpenAPI checks, lint-staged, and Knip passed. Historical log-alias fixtures required removing their expired TTL only in the isolated test ClickHouse instance. Remaining review suggestions:
Maintainers: please review the remaining scope and scale-validation items. Per AGENTS.md, “After two rounds of bot findings, stop.” This is the second round; further automated suggestions need maintainer triage. |
|
Addressed the remaining minor findings in a476fa9:
Validation: all 8,597 unit tests passed (4,603 app, 1,157 API, 2,837 common), plus 75 API integration tests and 14 local Playwright tests. The browser suite verifies a single summary request, distinct metric values, source creation/switching, autocomplete, Search, drafts, filters, Sankey, and real axis bounds. Geometry unit coverage uses real Recharts context and bars. Repository lint/type/style/OpenAPI checks, lint-staged, and Knip passed. Local Playwright used a temporary configuration that bypassed only the unrelated global PromQL seeder, which is incompatible with local ClickHouse 26.10; these NetFlow specs seeded their own real data. The committed runner and CI configuration remain unchanged. A separate two-million-row, five-dimension, 30-day Sankey check completed in 0.71 seconds with a 512 MiB memory cap and a 64 MiB spill threshold. The documentation explains that LIMIT bounds returned paths rather than aggregation memory and records the test's scope. |
Deep Review✅ No critical issues found. Direct inspection of the highest-risk path — ClickHouse SQL construction for NetFlow ( 🟡 P2 -- recommended
🔵 P3 nitpicks (1)
Reviewers (14 dispatched): correctness, security, adversarial, testing, maintainability, project-standards, performance, api-contract, reliability, kieran-typescript, julik-frontend-races, previous-comments, agent-native, learnings-researcher. Coverage note: This synthesis is grounded in direct orchestrator inspection of Testing gaps: NetFlow browser/ingestion fixtures and the 2M-row Sankey scale check remain local/manual rather than CI-enforced, per the author's documented scope decisions. 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 |
|
Addressed the latest review findings in bbd3e30:
Validation: 2,842 common-unit tests, 280 focused app tests, 68 API unit tests, 112 API integration tests, and all 15 NetFlow Playwright tests passed. Repository lint/type/style/OpenAPI checks, lint-staged, and Knip passed. Local Playwright again used the temporary global-setup bypass for the unrelated PromQL seeder incompatibility with ClickHouse 26.10; NetFlow fixtures seeded and cleaned up their own real ClickHouse data. For the repeated items in Deep Review, a476fa9 already includes the sampling multiplier/default-one description, SQL-metacharacter execution coverage in The two Vercel statuses require deployment authorization. Follow-up adc1e10 consolidates the 23 NetFlow query/hydration regressions into the existing test file. Both review jobs had stopped before code review because their |
| (async () => { | ||
| try { | ||
| if (watchedTableName !== prevTableNameRef.current) { | ||
| if ( |
There was a problem hiding this comment.
🟠 major — Schema inference now overwrites saved mappings on any source when the connection, database or kind changes
Inference used to run only when the table name changed. It now also runs on a kind, connection, database or metadata change, for every source kind. It calls resetField(..., { defaultValue }) on every inferred field that isn't dirty, and loaded saved values never count as dirty. So editing an existing Log or Trace source and switching it to another connection (for example a replica with the same table) silently swaps custom implicitColumnExpression, bodyExpression and similar values for the inferred defaults, and Save persists them. Fix: only re-infer on these new triggers when isNew is true or the watched kind is SourceKind.Netflow, or skip fields whose current value differs from their pristine value.
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
There was a problem hiding this comment.
Fixed in 7ebbb8d. Context changes now trigger inference only for new sources; saved Log, Trace, and NetFlow mappings survive connection/database/metadata changes. Explicit table/kind changes still infer, and stale responses remain cancelled. Regression tests reproduced the overwrite for all three kinds before the fix; all 19 source-form tests pass.
| ], | ||
| where: '', | ||
| timestampValueExpression: source.timestampValueExpression, | ||
| ...(source.kind === SourceKind.Netflow && { |
There was a problem hiding this comment.
🔵 minor — Bare-term implicit column is limited to NetFlow in the external charts API
getSourceImplicitColumnExpression already returns the right value for every kind (Log/Trace configured expression, NetFlow derived, undefined otherwise). Gating it on source.kind === SourceKind.Netflow adds a special case and leaves Log/Trace series with bare Lucene terms in where without their configured implicit column. Set implicitColumnExpression: getSourceImplicitColumnExpression(source) unconditionally, as the other call sites in this diff do.
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
There was a problem hiding this comment.
Fixed in 7ebbb8d by applying getSourceImplicitColumnExpression unconditionally. The Log/Trace omission predates this feature, but the already-edited builder now handles all three searchable kinds consistently. Real ClickHouse tests through POST /api/v2/charts/series verify bare-term plus numeric Lucene filtering for Log, Trace, and NetFlow (6 query integration tests pass).
| icon: <IconSitemap size={16} />, | ||
| isBeta: true, | ||
| }, | ||
| { |
There was a problem hiding this comment.
🔵 minor — NetFlow nav, spotlight and preset entries are shown to every user unconditionally
The nav link, the Spotlight preset (packages/app/src/Spotlights.tsx:93) and the dashboards-list preset all appear for every deployment, even with no NetFlow source. The neighbouring niche preset (Kubernetes) sits behind IS_K8S_DASHBOARD_ENABLED. Put these behind a flag, or hide them when useSources() has no SourceKind.Netflow source, or at least mark the nav entry isBeta like Service Map.
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
There was a problem hiding this comment.
Marked the NetFlow nav entry isBeta in 7ebbb8d using the existing Service Map convention. Kept the entry discoverable when no source exists so users can reach its source-setup flow.
|
Addressed the current Claude, Greptile, and Deep Review findings in 7ebbb8d:
Validation: 173 affected app unit tests, 13 real ClickHouse/Mongo API integration tests, and all 16 NetFlow Playwright tests passed. Integration coverage now includes Log/Trace/NetFlow bare terms through the v2 series endpoint, saved-alert OK/no-match and grouped results, NULL/zero/negative sampling, URL range removal, and source-switch draft cleanup. Repository lint/type/style/OpenAPI checks, lint-staged, and Knip passed. The reported partial-bucket test gap was already covered by the real ClickHouse test in adc1e10 (61 buckets at the expected constant rate, plus exact-range totals). I retained the existing default-SELECT type guards: they already include Log, Trace, and NetFlow, while isSearchableSource returns a boolean rather than narrowing the source union; replacing them is a future refactor, not a current behavior fix. Removed the previous unnecessary Services-dashboard helper edit because that dashboard restricts sources to Trace. The PR remains at 100 files, keeping the existing review jobs' file-list check working. Local Playwright again used isolated NetFlow fixtures and the documented bypass for the unrelated global PromQL seeder on ClickHouse 26.10. Vercel deployment still requires authorization. |
| ] | ||
| .map(expression => expression?.trim()) | ||
| .filter(Boolean); | ||
| return `concatWithSeparator(' ', ${expressions.map(expression => `ifNull(toString(${expression}), '')`).join(', ')})`; |
There was a problem hiding this comment.
🔵 minor — Bare-term protocol search uses raw numbers, but the UI shows names like TCP
The default implicit expression wraps the raw protocolExpression (toString(Proto) gives '6'). The charts, records and pills all show transform(...) names from getNetflowDimensions (packages/app/src/netflow.ts), so a bare TCP becomes ILIKE '%TCP%' and matches nothing. That contradicts docs/netflow.md, which says bare terms search the protocol. It also re-lists the mapped fields separately from getNetflowDimensions. Fix: move the dimension builder (protocol name transform, ::ffff: stripping) into common-utils and build the implicit expression from it, so there is one list of searchable flow dimensions.
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
| isTraceSource(source) || isLogSource(source) | ||
| ? source.implicitColumnExpression | ||
| : undefined, | ||
| implicitColumnExpression: getSourceImplicitColumnExpression(source), |
There was a problem hiding this comment.
🔵 minor — NetFlow dashboard filters are unreachable from the UI
QUERY_EXPRESSION_FILTER_SOURCE_KINDS (packages/common-utils/src/filters.ts:872) still lists only Log/Trace/Session/Metric. QueryExpressionFilterEditForm and DBDashboardImportPage use it as allowedSourceKinds, so users can never pick a NetFlow source for a dashboard filter. The new hook path and its test only run for hand-crafted or API-created filters. Fix: add SourceKind.Netflow to that list if the changeset's dashboard-filter support is intended, or drop that claim.
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 { useDefaultTimeRange, useNewTimeQuery } from '@/timeQuery'; | ||
| import { parseAsJsonEncoded } from '@/utils/queryParsers'; | ||
|
|
||
| const queryParsers = { |
There was a problem hiding this comment.
🔵 minor — Two separate filter systems for the same flow fields
Exporter, protocol and source/destination IP can each be filtered two ways: the quick-filter text inputs (exporter/protocol/srcAddr/dstAddr URL params, rendered by buildNetflowWhere as raw-column SQL that wants '6') and click include/exclude filters (the filters param, keyed on the canonical dimension expression that wants 'TCP' and stripped IPv4 addresses). They show up separately (inputs vs pills) and use different value formats, and both URL formats now have to be supported. Fix: route the quick inputs through useNetflowFilterState/applyFilter so each field has one persisted representation.
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
| isNew={mode === 'new'} | ||
| sourceId={mode === 'edit' ? sourceId : undefined} | ||
| defaultName="NetFlow" | ||
| defaultKind={SourceKind.Netflow} |
There was a problem hiding this comment.
🔵 minor — 'Add NetFlow source' modal can create a non-NetFlow source
TableSourceForm always renders the full kind radio group, so in this modal the user can switch to Logs or Traces and save. NetflowPage's onCreate then calls clearFilters(created.id) with an id that is not in netflowSources, and the page shows 'NetFlow source unavailable'. Fix: add a prop to TableSourceForm that locks or hides the kind selector when defaultKind is forced, and pass it from this modal.
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
| label={SOURCE_KIND_LABELS[SourceKind.Promql]} | ||
| /> | ||
| )} | ||
| <Radio |
There was a problem hiding this comment.
🔵 minor — Beta NetFlow kind is shown without a feature flag
Every other optional source kind is gated (IS_METRICS_ENABLED, IS_SESSIONS_ENABLED, IS_PROMQL_ENABLED), but the NetFlow radio, nav entry (marked isBeta), spotlight action and preset dashboard card are always shown to every deployment. Fix: add an IS_NETFLOW_ENABLED flag in config.ts and gate these entry points with it, as PromQL does.
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
| <ActiveFilterPills | ||
| searchFilters={state} | ||
| chartConfig={chartConfig} | ||
| enableValueEditing={false} |
There was a problem hiding this comment.
NetFlow now uses ActiveFilterPills, whose range removal calls clearFilter and deletes all selections for that field. For a shared URL containing Bytes BETWEEN 100 AND 200 and Bytes NOT IN ('150'), removing only the range also removes the exclusion. The page then shows more traffic than the user intended.
Remove just range and keep the field’s included and excluded values.
Bare Lucene terms searched the raw protocol number, so "TCP" matched no flows while charts, pills and records all show protocol names. The default full-text expression now uses the protocol-name transform from the NetFlow dimensions, moved to common-utils so both share one list. The source modal can save a non-NetFlow source. The page now switches only to a created NetFlow source instead of showing "NetFlow source unavailable". Flow records keyed rows by index behind an eslint-disable, which fails the escape-hatch ratchet. Rows are now keyed by content plus a repeat count. The changeset no longer claims dashboard-filter support, because dashboard filters cannot select NetFlow sources. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous commit replaced the protocol number with its name in the default full-text expression, so a bare "17" stopped matching UDP flows. Search both the number and the name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| severityText: source.severityTextExpression, | ||
| traceId: source.traceIdExpression, | ||
| }; | ||
| } else if (source.kind === SourceKind.Netflow) { |
There was a problem hiding this comment.
🔵 minor — NetFlow keyColumns were added to describe_source but not to clickstack_list_sources
packages/api/src/mcp/tools/sources/listSources.ts:274-308 still builds meta.keyColumns only for Trace, Log and Metric, so agents that list sources get no flow mappings for NetFlow sources while describe_source returns them. Add the same SourceKind.Netflow branch there, ideally through one shared helper used by both tools so the two lists can't drift.
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
Charts, records and the Sankey show protocols by name, but the Protocol filter only matched the raw number, so typing "TCP" returned no flows. Known names now map to their protocol number before filtering. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| label={SOURCE_KIND_LABELS[SourceKind.Promql]} | ||
| /> | ||
| )} | ||
| <Radio |
There was a problem hiding this comment.
🔵 minor — NetFlow source kind is always shown; Metric, Session and PromQL are each behind a feature flag
Gate the NetFlow radio behind its own flag, the way the sibling kinds use IS_METRICS_ENABLED, IS_SESSIONS_ENABLED and IS_PROMQL_ENABLED. Apply the same flag to the other entry points this PR adds without one: the /netflow nav link (AppNav.tsx), the Spotlight preset (Spotlights.tsx) and the dashboards-list preset (DashboardsListPage.tsx). As written, every deployment gets a beta source kind and page with no way to turn them off.
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
| @@ -2653,7 +2692,9 @@ export function isPromqlSource(source: TSource): source is TPromqlSource { | |||
| return source.kind === SourceKind.Promql; | |||
| } | |||
| export function isSearchableSource(source: TSource): boolean { | |||
There was a problem hiding this comment.
🔵 minor — Log/Trace/NetFlow kind check is copied by hand to ~6 sites because isSearchableSource is not a type guard
Make isSearchableSource a type guard (source is TLogSource | TTraceSource | TNetflowSource) and use it in resolveSelect (searchChartConfig.ts), AlertPreviewChart.tsx, computeAliasWithClauses (checkAlerts/index.ts), the select fallback in DBSearchPage.tsx, and both ChartEditorControls.tsx checks. Each of those now re-spells the same three-way kind check, so adding the next searchable kind means editing all of them again.
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
| tabIndex?: number; | ||
| }; | ||
| }; | ||
| type Props = { |
There was a problem hiding this comment.
🔵 minor — memo on NetflowFilterMenu has no effect because its callback props are recreated on every render
onFilter from useNetflowFilterState (NetflowFilterPills.tsx) is a new function on every NetflowPage render. It is passed down through NetflowCharts and NetflowRecords to every cell. NetflowSankeyTable and NetflowSankeyNode also pass inline onSelect arrows. So all 500×4 record cells re-render anyway, despite the comment about thousands of cells. Fix: wrap onFilter/onDimensionFilter in useCallback (with a ref for expressions/dimensions) and pass stable dimension/value props instead of inline closures; otherwise drop the memo.
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 has not been deployed
1 participant
Summary
Add native NetFlow monitoring for Akvorado tables through HyperDX connections, source configuration, Search, and charts. Integrate shared schemas, persistence, and the external API with configurable mappings and schema detection.
The page includes sampling-aware rates, top talkers, exporter/interface breakdowns, flow details, and a Sankey with two to five ordered dimensions. Lucene supports column suggestions; clickable values provide include/exclude filters. Searches, filters, and visualization settings persist in the URL. Sankey links represent selected top paths with bitrate and byte totals. Fix clipped rate-axis labels and direct ClickHouse requests in local mode.
Validation: unit suites, TypeScript, lint, ClickHouse query checks, and browser workflows passed. Screenshots use 14,400 synthetic records; setup instructions are in
docs/netflow.md.Screenshots
How to test on Vercel preview
Preview routes: /netflow
Proto:6, select Sankey dimensions, and include/exclude a node.Reference
Akvorado provides the schema and visualization reference.