perf(dashboard): cap high-cardinality time-chart series with load-all escape hatches - #2802
Open
brandon-pereira wants to merge 12 commits into
Open
perf(dashboard): cap high-cardinality time-chart series with load-all escape hatches#2802brandon-pereira wants to merge 12 commits into
brandon-pereira wants to merge 12 commits into
Conversation
… escape hatches Time charts with many groups could render thousands of <Area>/<Bar> series at once, spiking memory and causing severe hover jank. Bound the number of series materialized and drawn per tile, and give users ways to see the rest: - A per-tile Series Limit control (0 = unlimited) in Display Settings, defaulting to a safe cap; series beyond the cap are dropped lowest-peak-first and surfaced via a hidden-series notice. - +N more affordances in the hover and pinned tooltips, and a load-all-series action that lifts the cap on demand. - Tooltips cap how many rows they render per frame so a wide bucket cannot mount thousands of popovers; the pinned tooltip scrolls the full set once loaded. - The external dashboards API round-trips the limit as a positive-only limit so a GET -> PUT of a tile with no limit is not rejected by the write schema.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 622731d 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 |
brandon-pereira
marked this pull request as ready for review
August 4, 2026 21:29
github-actions
Bot
added
the
review/tier-4
Contributor
🔴 Tier 4 — CriticalTouches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD. Why this tier:
Additional context: agent branch ( Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Contributor
E2E Test Results✅ All tests passed • 269 passed • 1 skipped • 1096s
Tests ran across 4 shards in parallel. |
Contributor
Greptile SummaryThe PR limits materialized high-cardinality time-chart series while retaining bounded, explicit load-all behavior and reducing chart transformation and rendering overhead.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported comparison-group ranking, sparse peak scanning, group-key collision, previous-only handling, and bounded load-all issues are addressed in the current code.
|
| Filename | Overview |
|---|---|
| packages/app/src/ChartUtils.tsx | Adds bounded logical-group materialization, sparse peak ranking, timestamp caching, and complete comparison/multi-value group retention. |
| packages/app/src/components/DBTimeChart.tsx | Introduces query-shape-scoped bounded load-all state and correctly disables actions once the cap cannot be raised. |
| packages/app/src/HDXMultiSeriesTimeChart.tsx | Reduces tooltip and hover rendering costs while preserving bounded expanded tooltip behavior. |
| packages/app/src/components/charts/ChartSeriesTooltip.tsx | Caps tooltip rows and exposes load-all only when an actionable handler is available. |
| packages/app/src/components/charts/HiddenSeriesIndicator.tsx | Provides clickable or passive hidden-series indicators according to whether loading additional series is possible. |
| packages/app/src/hooks/useChartConfig.tsx | Adds a first-chunk fast path that avoids an unnecessary full-array copy. |
| packages/common-utils/src/core/renderChartConfig.ts | Applies server-side series limiting only for positive limits, preserving zero as unlimited. |
| packages/common-utils/src/types.ts | Unifies nonnegative series-limit semantics in shared chart configuration schemas. |
| packages/api/src/routers/external-api/v2/utils/dashboards.ts | Omits nonpositive external limits so dashboard GET-to-PUT round trips satisfy positive-only write schemas. |
| packages/api/src/mcp/tools/dashboards/schemas.ts | Aligns MCP dashboard schemas and descriptions with shared three-state series-limit behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Config[Tile seriesLimit] --> Resolve[Resolve materialization cap]
Query[Current and previous query results] --> Format[Build logical series groups]
Resolve --> Rank[Rank groups by peak]
Format --> Rank
Rank --> Cap[Retain complete groups up to cap]
Cap --> Render[Render up to hard line limit]
Cap --> Hidden{Hidden groups remain?}
Hidden -->|Yes, cap can increase| Load[Load all up to bounded maximum]
Load --> Rank
Hidden -->|Yes, bound reached| Passive[Show passive hidden-series count]
Hidden -->|No| Render
Reviews (11): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Cap by logical series (grouped by currentPeriodKey) so comparison current/previous pairs are kept or dropped together instead of being orphaned by a flat entry-list slice. - Peak-rank scan iterates each bucket's populated cells only, so sparse high-cardinality results cost O(populated cells) not O(buckets*series). - HiddenSeriesIndicator goes passive (no onLoadAll) once showAllSeries is already on, so a result past MAX_LOADABLE_TIME_CHART_SERIES doesn't offer a no-op click; stopPropagation on the button so it can't start a tile drag. - resolveRenderedSeriesCap falls back to the default cap for NaN/negative/ non-integer values instead of disabling the guard. - queryShapeIdentity folds in the raw seriesLimit so a null<->0 edit resets the load-all opt-in. - Clarify the seriesLimit doc comment (fetch vs render cap, null/0/N).
PR #2772 (merged into this branch via main) added seriesLimit to the external Line/StackedBar tile schemas as positive-only, and the converter emitted `config.seriesLimit ?? undefined`. This branch relaxes the internal schema to allow 0 (unlimited), so a stored 0 would serialize verbatim and be rejected on a GET->PUT round-trip by the positive-only write schema. Gate both cases on hasPositiveSeriesLimit so 0/null map to absent, matching the existing Pie/Bar limit mappings. Adds unit coverage for 0/null/positive.
- Rewrite the external seriesLimit test to round-trip through JSON instead of `as any` casts, so it no longer adds no-unsafe-type-assertion warnings that pushed the api package over its --max-warnings budget. - Fold the line/stacked_bar 0-gating note into the single series-limit changeset (no separate entry).
Store the query shape the 'load all' opt-in was enabled at and derive showAllSeries from `shape === queryShapeIdentity` during render, rather than resetting a boolean in a queryShapeIdentity-keyed effect. This drops the opt-in in the same render the query changes (no one-commit window where a stale opt-in pairs with the new shape) and removes the react-hooks/set-state-in-effect warning. The pin-dismiss stays in its effect as a genuine side effect.
The main merge left two identical .chartTooltipContentClipped blocks in HDXLineChart.module.scss, which stylelint's no-duplicate-selectors rejected as an error — failing the app ci:lint (yarn lint:styles) step with a cryptic exit 2 that surfaced no eslint/tsc summary. Drop the duplicate.
…review findings External API (allow 0 = unlimited): - Relax external line/stacked_bar seriesLimit and pie/bar limit schemas from .positive() to .nonnegative(), so 0 (unlimited) is a valid, round-tripping value instead of being rejected/dropped on GET->PUT. - Converter emits 0 (?? undefined) across all four tile types; OpenAPI docs updated to minimum:0 with three-state description; openapi.json regenerated. Deep-review fixes: - Return a logical renderedSeriesCount from formatResponseForTimeChart so the hidden-series notice isn't double-counted on comparison charts. - Rank only current-period logical series when capping, so a previous-only group can't evict a current-period series the query explicitly selected. - HiddenSeriesIndicator uses the defined --color-text-warning token (not an undefined --color-warning) and an ActionIcon variant=subtle per convention. - Strengthen the load-all opt-in test to rerender with a genuinely shifted dateRange, guarding against dateRange re-entering the shape identity.
The previous-only-eviction fix ranked only current-period groups so a previous-only group couldn't evict a current-period series, but the keptGroups filter then dropped those previous-only groups entirely when over the cap, and they weren't counted in hiddenSeriesCount — so a dashed previous-period-only series vanished with no accounting. Add every previous-only group back into keptGroups unconditionally (the previous query's own CTE already bounds them), and add a regression test.
…w 0 in MCP
Addresses deep-review + Greptile findings on the series cap:
- Cap by GROUP identity (strip the value-column prefix from currentPeriodKey)
so a multi-value chart (e.g. avg+max) keeps all value columns of a surviving
group together instead of a large-magnitude column evicting a smaller one.
- Bound the TOTAL logical groups by maxSeries in comparison mode: current-period
groups take priority, then previous-only groups fill remaining slots. Fixes
both a previous-only group evicting a current one and disjoint result sets
exceeding the cap while underreporting hiddenSeriesCount.
- MCP seriesLimitSchema and pie/bar limit relaxed .positive() -> .nonnegative()
so a seriesLimit: 0 (unlimited) tile no longer rejects the whole MCP save;
descriptions corrected.
- Load-all uses Math.max(bound, tile cap) so it can only raise the rendered
count, never reduce it when a tile's seriesLimit already exceeds the bound.
- hasPositiveSeriesLimit now requires Number.isInteger, matching
resolveRenderedSeriesCap, so a non-integer stored value can't reach the CTE
and bind as { Int32: 0.5 }.
- Pie/bar external limit OpenAPI docs updated to minimum:0; openapi.json
regenerated.
Adds ChartUtils tests for multi-value grouping, current-period priority with a
bounded total, and previous-only retention under the cap.
- groupKeyOf stripped the '<valueColumn> · ' prefix even on single-value charts that never add it, so a group literally named 'value · x' collapsed into group 'x'. Only strip when the key builder added the prefix (multiple value columns AND group columns). Adds a collision regression test. - Load-all is offered only when it would actually raise the cap: when a tile's own seriesLimit already meets/exceeds MAX_LOADABLE_TIME_CHART_SERIES, both maxSeries branches were equal, so the click was a no-op that then hid the affordance permanently. Gated via loadAllHandler. - Assert renderedSeriesCount (logical group count) in the multi-value and comparison tests so a regression to lineData.length is caught.
Contributor
|
＜!-- deep-review --＞ Deep ReviewWarning This review is incomplete and diff-blind — read this before trusting the findings below. The runner environment is broken in a way that prevented the requested multi-agent review from completing:
Consequences: the diff was never obtained, so nothing below is attributed to specific added lines — these are defects in the code as it currently stands on the PR head, some of which may predate this PR. No test file could be located or read, so this review contains no test-coverage assessment. Eight persona reviewers were dispatched but none returned before output was required; the findings below come from my own direct reading of the source. Re-run this review in a working environment before treating it as a merge gate. ✅ No P0/P1 issues found — but read that as "none substantiated by the partial inspection that was possible", not as a clean bill of health. 🟡 P2 -- recommended
🔵 P3 nitpicks (3)
Reviewers (1): orchestrator direct source inspection. Eight persona reviewers (correctness ×2, performance, testing, adversarial, api-contract, maintainability, kieran-typescript) were dispatched but returned no results before output was required. Testing gaps:
Verified as non-issues: |
brandon-pereira
requested review from
a team and
pulpdrew
and removed request for
a team
August 5, 2026 18:40
pulpdrew
previously approved these changes
Aug 6, 2026
…tile-series-limit # Conflicts: # packages/app/src/HDXMultiSeriesTimeChart.tsx # packages/app/src/components/DBTimeChart.tsx # packages/app/src/components/charts/ChartSeriesTooltip.tsx # packages/app/src/components/charts/__tests__/ChartSeriesTooltip.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
High-cardinality time charts — especially raw SQL tiles — can return tens of thousands of series. The client held all of them in memory while only ~100 lines were ever drawn, and every hover re-serialized a huge chart SVG. On a heavy dashboard this pinned the browser tab at several GB of memory and made panning, hovering, and editing sluggish.
This PR caps what gets fetched and rendered, gives users explicit escape hatches to override those caps, and lands a batch of transform/render optimizations. Together they bring a heavy dashboard down from ~8 GB to ~2 GB in some scenarios and make the app noticeably more responsive (smoother hover, faster tile edits, less jank while scrolling).
What changed
Per-tile "Series Limit" control
seriesLimitnow applies to all time charts (builder and raw SQL) viaSharedChartSettingsSchema, with unified three-state semantics:null/undefined→ default materialization cap (MAX_RENDERED_TIME_CHART_SERIES= 250)N→ keep the top N series by peak value0→ unlimited (explicit opt-out)hasPositiveSeriesLimit()helper centralizes the!= null && > 0check that gates the SQL__hdx_series_limitCTE, the pie/barLIMIT, and the chunked-ranking window. Schema relaxed from.positive()→.nonnegative()so0is storable;renderChartConfigskips the CTE for0(avoids emittingLIMIT 0).min=0), with distinct copy for the raw-SQL render cap vs. the builder fetch cap."Load all series" escape hatches
+N moreline both become clickable, letting users render every series on demand.MAX_LOADABLE_TIME_CHART_SERIES(5000) rather thanInfinity, so a runaway result still can't exhaust memory; drawn lines remain capped byHARD_LINES_LIMIT.Render & transform optimizations (responsiveness)
rr-blockon the chart root — tells the rrweb session-replay recorder to capture each chart as a placeholder instead of serializing its large SVG DOM on every mutation (the dominant session-replay cost on high-cardinality dashboards).addResponseToFormattedDatapreviously rebuilt aLineDataon every result row (~250k allocations) instead of once per distinct series.new Date(...)parsing dominated; now cached per raw value.<Area>props (rebuilding every line each frame); now a scoped CSS class handles thicken/fade. The anomaly band is explicitly excluded so it stays shaded.+N more) for both hover and pinned tooltips (always keeping the cursor-nearest series); pinned tooltip lifts toMAX_EXPANDED_TOOLTIP_ROWS(500) and scrolls when "load all" is active.appendChunkfast path that reuses the chunk array for the first/only chunk instead of an O(rows) spread copy.External API v2 fix
limitas absent (not0/null), since the external field is positive-only viahasPositiveSeriesLimit()— keeps a GET→PUT round-trip from being rejected by the write-body schema.Impact
MAX_RENDERED_TIME_CHART_SERIES), per-tile overridableHARD_LINES_LIMIT)rr-block)Tests
hasPositiveSeriesLimitunit tests +renderChartConfig"no CTE whenseriesLimit=0"ChartUtilsmaxSeriescapping behaviorHiddenSeriesIndicatorandChartSeriesTooltipload-all affordance (passive vs. clickable+N more)DBTimeChartload-all escape-hatch behavior (appears/disappears;seriesLimit=0shows no affordance)useChartConfigappendChunkfast pathChartDisplaySettingsDrawercontrol visibilityNotes for reviewers
minorbump for@hyperdx/common-utils,@hyperdx/app,@hyperdx/api).