Skip to content

[HDX-5080] Chart editor UI for metric formulas - #2909

Open
wrn14897 wants to merge 2 commits into
mainfrom
warren/HDX-5080-render-metric-formula-UI
Open

[HDX-5080] Chart editor UI for metric formulas#2909
wrn14897 wants to merge 2 commits into
mainfrom
warren/HDX-5080-render-metric-formula-UI

Conversation

@wrn14897

@wrn14897 wrn14897 commented Aug 14, 2026

Copy link
Copy Markdown
Member

Summary

Exposes metric formulas (HDX-5078's formulas config, rendered by HDX-5079) in the chart editor for metric sources, so a derived series like A / (A + B + C) * 100 can be built, validated, saved, and reloaded from the UI.

Rebased onto main now that #2908 (HDX-5079 rendering) has merged.

Editor

  • Formula rows (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".
  • Inline validation with the structured validator from HDX-5078 (validateFormula): malformed expressions, unknown series refs, constant-only expressions, etc. surface live under the input; validateChartForm blocks save/run with the same messages so an invalid expression can never reach ClickHouse.
  • Letter badges (A, B, C, ...) on metric series rows so formula refs are discoverable.
  • "Show input series" toggle drives 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.
  • Mutual exclusion with ratio: the "As Ratio" switch is hidden while a formula exists, and "Add Formula" is hidden while ratio mode is on (formulas supersede ratio in the renderer).
  • The Number-tile series cap (1, or 2 for ratio) is lifted when formulas exist, so operand-only series like A / (A + B + C) can be built.
  • normalizeChartConfig strips formulas/showOperandSeries on save for non-metric sources and for display types the composed metric query does not render (pie/bar/heatmap/search/patterns), mirroring the existing metricName/having stripping. 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.
  • New getBuilderValueColumnCount helper (formula/ratio-aware) used by DBTableChart for group-by column inference; per-column color mapping skips hidden-operand formula configs.
  • DBTimeChart drill-down skips the value-range filter when operands are hidden (formula columns don't map onto select expressions).
  • Legend/tooltip naming needs no changes — formula columns arrive as named result columns (alias || expression).

Persistence needs no API changes: tiles validate against SavedChartConfigSchema, which already carries formulas/showOperandSeries, and builderToRawSql already 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 — getChartConfigFromAlert rebuilds it from an explicit field list that dropped formulas/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.1 for a byte-valued operand), regardless of the tile's "Show input series" toggle. Fixed here:

  • formulas is passed through, and operand columns are always dropped from the alert query (showOperandSeries: false) so the formula is the value column parseAlertData picks — the alert evaluates exactly what the tile displays.
  • Drive-by with the same omission shape: ratioMode is now passed through, so grouped share_of_total ratio tile alerts no longer silently evaluate as per_group.
  • New integration tests (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, and share_of_total honored (asserting the exact share value a per_group fallback couldn't produce).

Testing

  • make ci-lint, make ci-unit pass.
  • New unit tests:
    • 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, showOperandSeries toggle, non-metric sources show no formula controls.
    • ChartEditor/utils.test.ts: validateChartForm formula rules (including the Number-tile cap lift) and normalization stripping/round-trip.
    • source.test.ts: useChartNumberFormats formula-column mapping (operands shown/hidden, ratio precedence, chart-format fallbacks) and getBuilderValueColumnCount.
  • New dashboard E2E (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

  1. Open a dashboard → Add tile → select a metrics source.
  2. Add two series (note the A/B badges), click Add Formula, enter A / (A + B) * 100.
  3. Try A / C to see the inline error; toggle Show input series; save, reload, and confirm the tile renders the formula series.

References

Screenshots

image

@changeset-bot

changeset-bot Bot commented Aug 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 6c9d2db

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@hyperdx/api Minor
@hyperdx/app Minor
@hyperdx/otel-collector Minor

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

@vercel

vercel Bot commented Aug 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview Aug 18, 2026 2:38am
hyperdx-storybook Ready Ready Preview Aug 18, 2026 2:38am

Request Review

@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR exposes metric formulas in the chart editor and carries their positional value-column contract through rendering, formatting, tables, drill-down behavior, and tile alerts.

  • Adds formula rows, validation, aliases, number formatting, operand visibility controls, and series-reference badges.
  • Normalizes formula configuration based on metric source and supported display type.
  • Updates chart consumers to account for formula and hidden-operand columns.
  • Preserves formulas and ratio modes when rebuilding metric tile-alert queries.
  • Adds unit, integration, and dashboard end-to-end coverage.

Confidence Score: 4/5

The PR is not yet safe to merge because series mutations can still silently change which metrics existing formula letters reference.

Formula references resolve by series position, while the current move, duplicate, and remove handlers mutate only the series array; displaying updated badges does not preserve operand identity or rewrite affected expressions.

Files Needing Attention: packages/app/src/components/DBEditTimeChartForm/ChartSeriesEditor.tsx and packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx

Important Files Changed

Filename Overview
packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx Adds formula field-array controls, operand visibility, ratio exclusion, and formula-aware Number-series limits.
packages/app/src/components/DBEditTimeChartForm/ChartFormulaEditor.tsx Introduces the formula expression, alias, validation, removal, and number-format editing UI.
packages/app/src/components/ChartEditor/utils.ts Adds formula-capable display normalization and save-time structured formula validation.
packages/app/src/source.ts Maps number formats and value-column counts across operand, formula, ratio, and hidden-operand query shapes.
packages/app/src/components/DBTableChart.tsx Uses formula-aware value-column counts for group-by and per-column rendering behavior.
packages/app/src/components/DBTimeChart.tsx Avoids deriving operand range filters from formula-only result columns.
packages/api/src/tasks/checkAlerts/index.ts Preserves formula and ratio configuration while rebuilding metric tile queries for alert evaluation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Editor[Chart editor series and formulas] --> Validation[Formula validation]
  Validation --> Saved[Saved chart config]
  Saved --> Query[Composed metric query]
  Query --> Columns[Operand and formula columns]
  Columns --> Charts[Time, table, and number charts]
  Saved --> Alerts[Tile alert config reconstruction]
  Alerts --> AlertQuery[Formula-only alert query]
  AlertQuery --> Threshold[Threshold evaluation]
Loading

Reviews (5): Last reviewed commit: "fix: evaluate metric formulas in tile al..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found.

The diff was traced against the renderer's actual projection contract in packages/common-utils/src/core/renderChartConfig.ts (operand columns in select order → formula columns → group-by passthrough; render-time validateFormula throws a structured error on malformed configs). The positional-mapping consumers, the tri-state showOperandSeries, the Number-tile cap lift, and the alert propagation all line up with that contract. No data-loss, injection, auth, or guaranteed-crash path was found in this change. The items below are hardening and coverage recommendations only.

🟡 P2 — recommended

  • packages/app/src/source.ts:637 — the "operands → formulas → group-by" value-column ordering is re-encoded independently in getBuilderValueColumnCount, useChartNumberFormats, DBTableChart group-by inference, DBTimeChart operand-hidden detection, and checkAlerts/index.ts, so a future renderer projection change can silently desync number formats, group-by slicing, color mapping, and the alert value column at once.
    • Fix: Route every consumer through one shared helper (extend getBuilderValueColumnCount/getFormulaConfig) so the projection order is asserted in a single place.
  • packages/api/src/tasks/checkAlerts/index.ts:719 — a tile alert with more than one formula evaluates only the last formula column (parseAlertData keeps the last value column), and no integration test exercises the multi-formula alert case, so a regression in projection order would pass CI.
    • Fix: Add a checkAlerts.int.test.ts case with two formulas asserting the last formula column drives the alert value.
🔵 P3 nitpicks (3)
  • packages/app/src/source.ts:607getFormulaConfig gates on config.metricTables != null, but the sibling formula checks in DBTableChart.tsx (colorByColumn skip) and DBTimeChart.tsx (operandsHidden) test only config.formulas?.length && config.showOperandSeries === false, so the same render uses two slightly different definitions of "has formulas."
    • Fix: Reuse getFormulaConfig in DBTableChart and DBTimeChart so all three checks share one predicate.
  • packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx:135handleRemoveFormula calls onSubmit(true) while handleAddFormula never calls onSubmit, an asymmetry that reads as an oversight even though the empty appended row is intentionally not persisted.
    • Fix: Add a short comment on handleAddFormula noting the append is deliberately not submitted until an expression is entered.
  • packages/app/src/components/ChartEditor/utils.ts:167isFormulaDisplayType duplicates the display-type membership already implied by the "Add Formula" gating in ChartEditorControls; the two lists must be kept in sync manually.
    • Fix: Derive the editor gate from isFormulaDisplayType rather than restating the display-type set.

Reviewers (6): correctness, adversarial, kieran-typescript, testing, maintainability, reliability.

Testing gaps:

  • No test covers a multi-formula tile alert (parseAlertData last-column semantics).
  • getBuilderValueColumnCount is not exercised with group-by columns present alongside hidden operands.
  • No round-trip test for normalizeChartConfig stripping and restoring formulas across a metric → non-metric → metric source switch.

@wrn14897
wrn14897 force-pushed the warren/HDX-5079-render-formulas branch from 8d9e37e to 41b2354 Compare August 17, 2026 19:03
@wrn14897
wrn14897 force-pushed the warren/HDX-5080-render-metric-formula-UI branch from 2067484 to 226e1a9 Compare August 17, 2026 23:08
@wrn14897
wrn14897 changed the base branch from warren/HDX-5079-render-formulas to main August 17, 2026 23:09
@wrn14897
wrn14897 force-pushed the warren/HDX-5080-render-metric-formula-UI branch from 226e1a9 to 1010ee4 Compare August 17, 2026 23:23
@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Aug 17, 2026
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Diff size: 508 production lines changed (Tier 2 max: < 250)
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api)

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.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 8
  • Production lines changed: 508 (+ 1057 in test files, excluded from tier calculation)
  • Branch: warren/HDX-5080-render-metric-formula-UI
  • Author: wrn14897

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 225 passed • 1 skipped • 829s

Status Count
✅ Passed 225
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 1

Tests ran across 3 shards in parallel.

View full report →

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
@pulpdrew

pulpdrew commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

One thing I noticed while testing: HAVING is applied within each CTE instead of to the final join, which is (to me at least) unexpected/unclear, and makes it hard to know what aliases/series/formulas I can reference.

Same with order by - I would expect to be able to reference any of the columns which are being outputted in the table in ORDER BY or HAVING.

Screenshot 2026-08-18 at 9 05 06 AM

@pulpdrew

pulpdrew commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Also, for number charts I suspect we need to make sure that (a) only one formula is allowed (b) show input series is hardcoded to false and (c) the chart renders the formula column and not any series columns.

I see we do (b) already when adding a formula, but not when a formula already exists and the display type is changed to number

Screenshot 2026-08-18 at 9 10 25 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants