[HDX-5080] Chart editor UI for metric formulas - #2909
Conversation
🦋 Changeset detectedLatest commit: f50b9bf 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.
|
Greptile SummaryThis PR exposes metric formulas in the chart editor and updates rendering metadata, number-chart normalization, and alert evaluation to consume formula columns correctly.
Confidence Score: 4/5The PR is not yet safe to merge because series mutations can silently rebind positional formula operands or leave formulas unsavable. Moving or duplicating a series changes the indices represented by formula letters without rewriting the expression, and removing a referenced series makes validation fail; the existing series controls remain enabled and unsynchronized with the formula field array. Files Needing Attention: packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx, packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx
|
| Filename | Overview |
|---|---|
| packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx | Adds formula field-array controls and operand visibility behavior, but series mutations remain unsynchronized with positional formula references. |
| packages/app/src/components/DBEditTimeChartForm/ChartFormulaEditor.tsx | Adds formula expression, alias, formatting, and live validation UI. |
| packages/app/src/components/ChartEditor/utils.ts | Adds formula-aware normalization and save-time validation, including number-chart constraints. |
| packages/api/src/tasks/checkAlerts/index.ts | Preserves formulas and ratio mode when rebuilding metric tile alert configurations and hides formula operands for evaluation. |
| packages/app/src/source.ts | Adds formula-aware value-column counting and number-format mapping. |
| packages/common-utils/src/core/utils.ts | Normalizes formula number-chart configurations to expose the formula as the displayed value. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Editor[Metric chart editor] --> Series[Operand series A, B, C]
Editor --> Formula[Validated formula expression]
Series --> Config[Saved chart config]
Formula --> Config
Config --> Query[Composed metric query]
Query --> Columns[Operand and formula columns]
Columns --> Charts[Table, time, or number chart]
Config --> Alert[Alert configuration rebuild]
Alert --> FormulaOnly[Formula-only alert query]
FormulaOnly --> Threshold[Threshold evaluation]
Reviews (8): Last reviewed commit: "Merge branch 'main' into warren/HDX-5080..." | Re-trigger Greptile
Deep Review✅ No critical issues found. The formula expression path is safe: 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers (2): ce-agent-native-reviewer, ce-learnings-researcher, plus orchestrator-level diff verification (formula grammar, alert column contract, positional column mapping, AST compilation). Coverage note: The eight dispatched persona reviewers (correctness, testing, maintainability, project-standards, kieran-typescript, reliability, adversarial, security) had not returned results at synthesis time; the findings above were confirmed directly against the diff and surrounding code, and the P0/P1-clean verdict reflects that independent verification rather than their input. ce-learnings-researcher found no relevant prior learnings in Testing gaps: New unit/integration/E2E coverage is substantial; the untested edge is the grouped-formula-alert value-selection path (P2 above) — add an integration case with a numeric group-by key. |
8d9e37e to
41b2354
Compare
2067484 to
226e1a9
Compare
226e1a9 to
1010ee4
Compare
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Additional context: touches background tasks or the delivery pipeline lightly (11 lines, under the 30-line bar for Tier 4) Review process: Full human review — logic, architecture, edge cases. Stats
|
E2E Test Results✅ All tests passed • 306 passed • 1 skipped • 1003s
Tests ran across 4 shards in parallel. |
1010ee4 to
862a3cf
Compare
Expose metric formulas (HDX-5078 schema, HDX-5079 rendering) in the chart editor for metric sources: - Formula rows in the builder (Add Formula / Remove Formula) with a letter-ref expression input, alias, and per-formula number format - Inline validation via the structured validator (malformed expressions, unknown series refs) plus save-time validation in validateChartForm - Letter badges (A, B, C) on metric series rows so refs are discoverable - Show input series toggle (showOperandSeries) for formula-only output; Number tiles default to hidden operands since they render the first value column - Formulas and the As Ratio toggle are mutually exclusive in the editor - normalizeChartConfig strips formulas for non-metric sources and display types the composed metric query does not render - useChartNumberFormats / DBTableChart / DBTimeChart updated for the formula-aware value-column projection (operands first unless hidden, then formula columns)
getChartConfigFromAlert rebuilds a builder tile's chart config from an explicit field list and dropped formulas/showOperandSeries, so an alert on a formula tile queried only the raw operand series and compared the threshold against the last operand's value (e.g. raw bytes) instead of the formula result — regardless of the tile's "Show input series" toggle. - Pass formulas through and always drop operand columns from the alert query (showOperandSeries: false) so the formula is the value column parseAlertData picks - Also pass ratioMode through: grouped share_of_total ratio tile alerts previously evaluated as per_group (the default) - Integration tests: formula value drives the alert (value distinct from both operands), toggle-independence, NULL formula (zero denominator) skipped, and share_of_total ratioMode honored
6c9d2db to
43cd185
Compare
Number charts display the first value column of the result, so a formula Number tile must project exactly one column: the formula. - convertToNumberChartConfig (common-utils) forces showOperandSeries: false for formula configs — the choke point every Number render passes through (dashboard tiles, editor preview, standalone charts, builderToRawSql), covering stale saved configs and display-type switches (formula defined on Line, then switched to Number) - normalizeChartConfig persists showOperandSeries: false on saved Number formula tiles so the stored config is self-describing; other display types keep the tile's own toggle value - validateChartForm rejects multiple formulas on Number charts; the editor hides Add Formula once a Number tile has one, and hides the Show input series toggle on Number (it would be a no-op) - The add-time setValue special case is removed in favor of the systematic enforcement above, so switching Number -> Line restores the user's operand-visibility choice - useSingleSeriesNumberFormat resolves the displayed format from formulas[0].numberFormat for formula configs instead of an operand that isn't rendered
|
Addressed in f5b76b1 — all three, plus the display-type-switch gap you called out: (a) Single formula on Number — (b)
(c) Renders the formula column — follows from (a)+(b): the Number query projects exactly one column, the formula. Also fixed a related gap found while implementing: Unit tests added at each enforcement point (common-utils |
…ot per-series branches (#2946) Fixes [HDX-5126](https://linear.app/clickhouse/issue/HDX-5126/apply-having-order-by-limit-to-the-composed-metric-join-not-per-series) — follow-up to [review feedback on #2909](#2909): "HAVING is applied within each CTE instead of to the final join … Same with order by." ## Why The composed multi-series metric query (HDX-5077) builds each per-series branch as `{ ...chartConfig, select: [...] }`, so `having`, `orderBy` and `limit` leaked into every UNION branch: - **HAVING** ran in a scope where the user-facing output names don't exist (the value column is renamed `__hdx_value`), and filtered each series independently before the join — making it impossible to know what aliases/series/formulas can be referenced. - **ORDER BY** ran per branch and was then discarded by the outer join — the final row order was nondeterministic. This also made table header-click sorting and `convertToTableChartConfig`'s default group ordering silent no-ops on composed metric tables. - **LIMIT/OFFSET** truncated each series to its own arbitrary group window before the join → mismatched group sets across series (spurious NULL cells) and incoherent table pagination (`useOffsetPaginatedQuery` injects page offsets into `limit`). ## What All three clauses now render once, on the final joined SELECT, where they reference the **output columns**: operand aliases (when shown), formula names/aliases, the ratio column, group-by passthroughs and the time bucket. Time charts stay bucket-ordered first, with the user sort as a tiebreaker. Row-level filters (`where`, `filters`, per-series `aggCondition`) stay per-branch, where they belong. ### Reference contract (pinned by integration tests on CH 26.5) Quoted output names or user aliases — e.g. `HAVING "err rate" > 0.5`, `ORDER BY "avg(metric.total)" DESC`, or a group-by `alias`. Raw expressions over source columns (e.g. `ResourceAttributes['service.name']`) do **not** resolve in the outer scope — verified `UNKNOWN_IDENTIFIER` under the new analyzer — so an expression group-by is referenced via its quoted ClickHouse-derived name (`"arrayElement(ResourceAttributes, 'service.name')"`) or, better, a group-by alias. Hidden operand series (`showOperandSeries: false`) are not referenceable: the contract is "reference what the result outputs". ## Tests - **Unit/snapshot** (`renderChartConfig.test.ts`): clauses render exactly once, on the outer statement, in HAVING → ORDER BY → LIMIT order; bucket ordering stays first with the user sort appended; HAVING can reference a formula output column. Existing composed snapshots are byte-identical (no clause fields → no change). - **Integration** (`queryChartConfig.int.test.ts`, 8 new): HAVING on an operand output column (including dropping a group whose *other* series fails the predicate — inexpressible per-branch) and on a formula alias; ORDER BY on a plain group column, an output value column, a quoted derived expression-group-by name, and a group-by alias; bucket-first ordering on time series; ratio-column HAVING/ORDER BY; LIMIT/OFFSET pages are disjoint windows of one consistent joined ordering. ## Out of scope (noted in HDX-5126) - `seriesLimit` top-N ranking still runs per-branch. - App-side: `DBTableChart` header-click sorting only quotes explicit user aliases — auto-aliased metric columns and formula columns produce unquoted `orderBy` expressions that don't resolve (broken before this change too, since branch scopes couldn't resolve them either). Editor autocomplete for output names in the Having/Order By inputs is a separate follow-up.
Plumb metric formula configs through every surface that persists or accepts chart configs: - External dashboards API v2: formulas/showOperandSeries on line, stacked_bar, table and number builder tile schemas, round-tripped through both converters, with write-time expression validation and a metric-source-only gate. OpenAPI spec regenerated with the new Formula component. - Shared validation: validateChartConfigFormulas in common-utils dashboardValidation mirrors the chart editor's save-time rules; isFormulaDisplayType moves to common-utils core/utils so the editor and server gates cannot drift. - MCP: save_dashboard / patch_dashboard tile schemas accept formulas, query_tile computes formula columns, and the dashboards query-guide prompt documents the feature. - CLI: number/table tile config transforms delegate to the shared formula-aware common-utils implementations; AGENTS.md pipeline notes updated. Alert-on-formula-tile support and its checkAlerts integration tests landed with HDX-5080 (#2909); verified green on this branch.
Plumb metric formula configs through every surface that persists or accepts chart configs: - External dashboards API v2: formulas/showOperandSeries on line, stacked_bar, table and number builder tile schemas, round-tripped through both converters, with write-time expression validation and a metric-source-only gate. OpenAPI spec regenerated with the new Formula component. - Shared validation: validateChartConfigFormulas in common-utils dashboardValidation mirrors the chart editor's save-time rules; isFormulaDisplayType moves to common-utils core/utils so the editor and server gates cannot drift. - MCP: save_dashboard / patch_dashboard tile schemas accept formulas, query_tile computes formula columns, and the dashboards query-guide prompt documents the feature. - CLI: number/table tile config transforms delegate to the shared formula-aware common-utils implementations; AGENTS.md pipeline notes updated. Alert-on-formula-tile support and its checkAlerts integration tests landed with HDX-5080 (#2909); verified green on this branch.
Plumb metric formula configs through every surface that persists or accepts chart configs: - External dashboards API v2: formulas/showOperandSeries on line, stacked_bar, table and number builder tile schemas, round-tripped through both converters, with write-time expression validation and a metric-source-only gate. OpenAPI spec regenerated with the new Formula component. - Shared validation: validateChartConfigFormulas in common-utils dashboardValidation mirrors the chart editor's save-time rules; isFormulaDisplayType moves to common-utils core/utils so the editor and server gates cannot drift. - MCP: save_dashboard / patch_dashboard tile schemas accept formulas, query_tile computes formula columns, and the dashboards query-guide prompt documents the feature. - CLI: number/table tile config transforms delegate to the shared formula-aware common-utils implementations; AGENTS.md pipeline notes updated. Alert-on-formula-tile support and its checkAlerts integration tests landed with HDX-5080 (#2909); verified green on this branch.
Plumb metric formula configs through every surface that persists or accepts chart configs: - External dashboards API v2: formulas/showOperandSeries on line, stacked_bar, table and number builder tile schemas, round-tripped through both converters, with write-time expression validation and a metric-source-only gate. OpenAPI spec regenerated with the new Formula component. - Shared validation: validateChartConfigFormulas in common-utils dashboardValidation mirrors the chart editor's save-time rules; isFormulaDisplayType moves to common-utils core/utils so the editor and server gates cannot drift. - MCP: save_dashboard / patch_dashboard tile schemas accept formulas, query_tile computes formula columns, and the dashboards query-guide prompt documents the feature. - CLI: number/table tile config transforms delegate to the shared formula-aware common-utils implementations; AGENTS.md pipeline notes updated. Alert-on-formula-tile support and its checkAlerts integration tests landed with HDX-5080 (#2909); verified green on this branch.
Plumb metric formula configs through every surface that persists or accepts chart configs: - External dashboards API v2: formulas/showOperandSeries on line, stacked_bar, table and number builder tile schemas, round-tripped through both converters, with write-time expression validation and a metric-source-only gate. OpenAPI spec regenerated with the new Formula component. - Shared validation: validateChartConfigFormulas in common-utils dashboardValidation mirrors the chart editor's save-time rules; isFormulaDisplayType moves to common-utils core/utils so the editor and server gates cannot drift. - MCP: save_dashboard / patch_dashboard tile schemas accept formulas, query_tile computes formula columns, and the dashboards query-guide prompt documents the feature. - CLI: number/table tile config transforms delegate to the shared formula-aware common-utils implementations; AGENTS.md pipeline notes updated. Alert-on-formula-tile support and its checkAlerts integration tests landed with HDX-5080 (#2909); verified green on this branch.
Plumb metric formula configs through every surface that persists or accepts chart configs: - External dashboards API v2: formulas/showOperandSeries on line, stacked_bar, table and number builder tile schemas, round-tripped through both converters, with write-time expression validation and a metric-source-only gate. OpenAPI spec regenerated with the new Formula component. - Shared validation: validateChartConfigFormulas in common-utils dashboardValidation mirrors the chart editor's save-time rules; isFormulaDisplayType moves to common-utils core/utils so the editor and server gates cannot drift. - MCP: save_dashboard / patch_dashboard tile schemas accept formulas, query_tile computes formula columns, and the dashboards query-guide prompt documents the feature. - CLI: number/table tile config transforms delegate to the shared formula-aware common-utils implementations; AGENTS.md pipeline notes updated. Alert-on-formula-tile support and its checkAlerts integration tests landed with HDX-5080 (#2909); verified green on this branch.



Summary
Exposes metric formulas (HDX-5078's
formulasconfig, rendered by HDX-5079) in the chart editor for metric sources, so a derived series likeA / (A + B + C) * 100can be built, validated, saved, and reloaded from the UI.Rebased onto main now that #2908 (HDX-5079 rendering) has merged.
Editor
ChartFormulaEditor) on metric-source builder charts (time series / table / number): "Add Formula" appends a row with a monospace letter-ref expression input, an alias, a per-formula number format (reuses the per-series format drawer), and "Remove Formula".validateFormula): malformed expressions, unknown series refs, constant-only expressions, etc. surface live under the input;validateChartFormblocks save/run with the same messages so an invalid expression can never reach ClickHouse.A,B,C, ...) on metric series rows so formula refs are discoverable.showOperandSeries(formula + raw operand series vs formula column(s) only). Adding a formula on a Number tile defaults operands to hidden, since Number tiles render the first value column.A / (A + B + C)can be built.normalizeChartConfigstripsformulas/showOperandSerieson save for non-metric sources and for display types the composed metric query does not render (pie/bar/heatmap/search/patterns), mirroring the existingmetricName/havingstripping. The form state keeps them, so switching back restores the rows.Rendering consumers (positional value-column contract)
The composed metric query projects operand columns (unless hidden) then formula columns, ahead of group-by passthrough columns. Updated the consumers that map columns positionally:
useChartNumberFormats: operand columns →select[i].numberFormat, formula columns →formulas[j].numberFormat, both falling back to the chart-wide format; chart-wide axis format prefers formula formats when operands are hidden.getBuilderValueColumnCounthelper (formula/ratio-aware) used byDBTableChartfor group-by column inference; per-column color mapping skips hidden-operand formula configs.DBTimeChartdrill-down skips the value-range filter when operands are hidden (formula columns don't map ontoselectexpressions).alias || expression).Persistence needs no API changes: tiles validate against
SavedChartConfigSchema, which already carriesformulas/showOperandSeries, andbuilderToRawSqlalready rejects formula configs with a clear message on the Builder → SQL switch.Alerts on formula tiles (
packages/api)Contrary to #2908's "alerts work with no changes" claim, the alert task does not run the tile config as-is —
getChartConfigFromAlertrebuilds it from an explicit field list that droppedformulas/showOperandSeries. An alert on a formula tile therefore queried only the raw operand series and compared the threshold against the last operand's value (e.g.740442112.0 meets or exceeds 0.1for a byte-valued operand), regardless of the tile's "Show input series" toggle. Fixed here:formulasis passed through, and operand columns are always dropped from the alert query (showOperandSeries: false) so the formula is the value columnparseAlertDatapicks — the alert evaluates exactly what the tile displays.ratioModeis now passed through, so groupedshare_of_totalratio tile alerts no longer silently evaluate asper_group.make dev-int FILE=checkAlerts, 168 passing): formula value drives the alert (fixture chosen so the formula result differs from both operands), toggle-independence, NULL formula (zero denominator) skipped without NaN history, andshare_of_totalhonored (asserting the exact share value aper_groupfallback couldn't produce).Testing
make ci-lint,make ci-unitpass.DBEditTimeChartForm.test.tsx: Add/Remove Formula, inline validation (malformed / unknown ref / clears when fixed), save round-trip, save blocked on invalid expression, ratio mutual exclusion,showOperandSeriestoggle, non-metric sources show no formula controls.ChartEditor/utils.test.ts:validateChartFormformula rules (including the Number-tile cap lift) and normalization stripping/round-trip.source.test.ts:useChartNumberFormatsformula-column mapping (operands shown/hidden, ratio precedence, chart-format fallbacks) andgetBuilderValueColumnCount.make dev-e2e FILE=dashboard GREP="Metric formulas", passing): creates a metric table tile with two gauge series +A / (A + B) * 100, asserts the inline error for an invalid ref, hides operands, saves, reloads the page, verifies the formula column renders with a finite value, and reopens the editor to verify the round-trip.How to test on Vercel preview
A/Bbadges), click Add Formula, enterA / (A + B) * 100.A / Cto see the inline error; toggle Show input series; save, reload, and confirm the tile renders the formula series.References
Screenshots