feat(charts): make the series limit opt-in and consistent across chunks#2449
feat(charts): make the series limit opt-in and consistent across chunks#2449wrn14897 wants to merge 7 commits into
Conversation
Each time-window chunk's __hdx_series_limit CTE ranked the top N within its own window, so the union across chunks could exceed seriesLimit and adjacent windows disagreed on which series to keep. Chunked queries now carry the full chart range as seriesLimitDateRange and the CTE ranks over that pinned range, so every chunk keeps the identical top-N set. Follow-up to HDX-4499.
🦋 Changeset detectedLatest commit: e40772c 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🟡 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
|
Greptile SummaryThis PR fixes three issues with the
Confidence Score: 5/5Safe to merge. The chunked-query ranking fix is correct, well-tested (unit + integration), and the render layer change is isolated to the series-limit CTE path. All three fixes are targeted and well-tested: the CTE pinning has a ClickHouse integration test that directly verifies the newest-window winner wins in both chunks, hook tests cover serial, parallel, and unchunked paths, and the SQL preview fix is covered by a new unit test. No data paths outside the series-limit feature are touched. The only observation is a cosmetic UI glitch in the Disable button visibility when the server returns null for an unset field. packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx — isCustomValue logic may show the Disable button when the setting is already null/unset. Important Files Changed
Sequence DiagramsequenceDiagram
participant UC as useQueriedChartConfig
participant FDC as fetchDataInChunks
participant CH as ClickHouse
participant RCC as renderChartConfig
participant SLC as renderSeriesLimitCte
UC->>FDC: "config (seriesLimit=N, dateRange=full)"
Note over FDC: windows = getGranularityAlignedTimeWindows()<br/>rankingDateRange = windows[0].dateRange (newest)
loop Each window chunk
FDC->>FDC: "windowedConfig = { dateRange: chunkWindow, seriesLimitDateRange: rankingDateRange }"
FDC->>CH: queryChartConfig(windowedConfig)
CH->>RCC: renderChartConfig(windowedConfig)
RCC->>SLC: "renderSeriesLimitCte(chartConfig, {from, where, groupBy})"
Note over SLC: cteConfig = {dateRange: seriesLimitDateRange,<br/> dateRangeStartInclusive: true, dateRangeEndInclusive: true}<br/>cteWhere = renderWhere(cteConfig)<br/>cteGroupBy = renderGroupBy(cteConfig)
SLC-->>RCC: CTE identical across all chunks
RCC-->>CH: SQL with pinned CTE + windowed outer query
CH-->>FDC: chunk result (top-N from newest window)
end
FDC-->>UC: merged results (consistent N series)
Reviews (6): Last reviewed commit: "chore: update changeset for opt-in serie..." | Re-trigger Greptile |
E2E Test Results✅ All tests passed • 199 passed • 3 skipped • 1319s
Tests ran across 4 shards in parallel. |
buildChartConfigForExplanations called convertToTimeChartConfig without the team's seriesLimit, so the Generated SQL section always rendered the default LIMIT 100 even when the team setting was lower. Pass the same value DBTimeChart uses so the preview matches the executed query.
Pinning the __hdx_series_limit ranking to the full chart range made every chunk re-scan the whole range. Rank over the newest window instead: still one fixed range shared by all chunks (so the top-N set stays consistent), but the scan is bounded by the smallest window. Trade-off: series are picked by recent activity, so groups with no events in the newest window are dropped from the chart.
The team seriesLimit setting no longer falls back to 100: when unset, charts fetch every series and no __hdx_series_limit CTE is emitted. The team settings page shows the setting as "Disabled" by default and adds a Disable button to clear a configured value back to undefined.
wrn14897
changed the title
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
Change itself LGTM in isolation. I have some thoughts though:
- I think opt-in is a good idea, in the future I could see it being useful to opt-in at the chart level instead of team-wide, so that charts without too many series don't pay the performance penalty of this new approach, while charts that do need series limiting can opt-in individually. This could be another "display setting"
- Also I am wondering if you've considered a two-query solution: One query to fetch the top-N series over the whole time range; then the main query, without a CTE, just an additional
WHERE <series> IN (...)condition?- This would address "Series are selected by activity in the newest window, so a group with no events there is dropped from the chart even if it was active earlier in the range."
- I would imagine this could also make the query building a bit simpler too, but there are probably some complexities around making sure that the
tuples identifying the top N series are rendered correctly, so maybe not. - This could also be used to fix the complaint that the chart seems to change drastically as new chunks load. The "pre flight" query could be used to get the max series value that will be rendered, and the chart y-axis could be scaled according to that value, instead of being re-scaled as each new chunk is loaded in.
- Not related to this change but something I notice about original implementation which is still present: When using multiple series, or when using ratio mode, only the first series is aggregated to choose the "top" series, instead of the max of the series or the ratio value.
|
2 participants
Summary
Reworks the top-N series cap added in #2429: the team setting becomes opt-in, and when enabled, the cap now holds across chunked queries and is reported accurately in the SQL preview.
The series limit is now opt-in (default: disabled). The team
seriesLimitsetting no longer falls back to 100: when unset, charts fetch every series and no__hdx_series_limitCTE is emitted — the pre-#2429 behavior. The team settings page shows "Time Chart Series Limit" as Disabled by default, and a configured value can be cleared back to disabled via a new "Disable" button (the generic setting form gainedallowUnset/resetLabelprops for settings without a server-side default). No API changes were needed — the update schema already acceptednullto unset, and the mongoose field has no default.When a limit is set, chunked queries now respect it. Time charts fetch data in time-windowed chunks (
useQueriedChartConfig→fetchDataInChunks), and each chunk's CTE ranked the top N within its own window — so a 46-minute chart withseriesLimit: 3split into a 15m + 31m chunk could render 5 series, and alert previews returned inconsistent group sets between adjacent windows. Chunked queries now carry a shared ranking range as a runtime-onlyseriesLimitDateRangefield — the newest chunk window, which also bounds the ranking scan instead of re-scanning the full chart range in every chunk — andrenderSeriesLimitCterenders the ranking CTE's WHERE/GROUP BY against that pinned range (with normalized boundary inclusivity) while the outer query still fetches only its window. The CTE is byte-identical across chunks and itsORDER BY max(rank) DESC, grouptiebreak is deterministic, so every chunk keeps the exact same top-N set and the union equals N. Trade-off: series are selected by activity in the newest window, so a group with no events there is dropped from the chart even if it was active earlier in the range. Unchunked consumers (alert evaluation, API) never set the field and are unchanged.Generated SQL preview shows the real limit. The chart editor's "Generated SQL" section always rendered
LIMIT 100regardless of the team setting, becausebuildChartConfigForExplanationscalledconvertToTimeChartConfigwithout the team value. It now passes the sameme.team.seriesLimitthatDBTimeChartuses, so the preview matches the executed query — including no CTE at all when the limit is disabled.Verified by unit tests (no CTE / no
seriesLimitwhen the team setting is unset; CTE pinned and byte-identical across two windowed renders; explanation config reflects the team limit), hook tests (serial and parallel chunked fetches pin the ranking to the newest window; unchunked fetches don't), and a ClickHouse integration test where two windows with different local winners both return the newest window's top-1.How to test on Vercel preview
Preview routes: /team, /chart
Steps:
__hdx_series_limitCTE in the SQL showsLIMIT 5(notLIMIT 100).References