Fix: Group By on ratio charts by jordan-simonovski · Pull Request #2538 · hyperdxio/hyperdx

jordan-simonovski · 2026-06-29T10:51:24Z

Problem

Ratio charts (As Ratio) couldn't be meaningfully grouped:

Adding a Group By collapsed the chart to a single line — all groups in a time bucket overwrote each other in the multi-series merge.
On metric sources, the Group By field offered no attribute autocomplete, so you couldn't discover keys like app.tenant_id or http.route while typing.

Root cause

Ratio runs the numerator and denominator as two separate queries and merges them. Two bugs:

The merge keyed rows by time bucket only, so different groups at the same bucket clobbered one another.
computeResultSetRatio dropped every non-value column, discarding the group dimension even if it had survived the merge.

Separately, metric sources have no single from.tableName (they fan out to per-type tables),input had no table to fetch attribute keys from.

Changes

packages/common-utils/src/clickhouse/index.ts

Merge now keys by (time bucket + group dimensions) instead of timestamp alone, so grouped series don't collapse. Ungrouped charts are unaffected (key falls back to the timestamp/fixed key).
computeResultSetRatio now identifies the two operands by numeric type and carries every other column through (timestamp + group dimensions), emitting one series per group.
Grouped ratios use share-of-total semantics: each group's denominator is the total of thel groups in that time bucket, so the grouped lines are each group's contribution to theoverall ratio and sum to the ungrouped value (e.g. each tenant's share of the overall error rate). Ungrouped ratios are unchanged (one row/bucket → bucket total is that row's denominator).
A group absent from the (filtered) numerator — e.g. a tenant with zero errors — contribut

packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx

The chart-level Group By input now builds a metric-aware tableConnection (resolving the selected metric's table via the shared getMetricTableName helper + metricName), so attribute autocomplete works while
typing on metric ratio/multi-series charts — matching the per-series inputs. Non-metric sou

Why share-of-total

"Group by" applied to a ratio has two plausible meanings: each group's own rate (per-group)n to the overall rate (share-of-total). We chose share-of-total so a grouped error-rate
decomposes the blended rate — the lines add up to the number you see ungrouped — which matcdown the 22% by tenant" chart. (Per-group rates are still available by not using ratio mode,or via a future toggle if needed.)

Testing

New unit tests in clickhouse.test.ts:
- grouped ratio = each group's share of the per-bucket total, and a bucket's contribution
- zero-error group → 0% (not N/A);
- existing non-grouped ratio tests unchanged (with/without timestamp; insufficient-column
Full common-utils unit suite green (1367); editor form tests green; lint + tsc clean.
Changeset added (@hyperdx/common-utils patch).

Scope

Only affects metric ratio charts — the only config path that splits into the merge/ratio coatio or non-metric charts.

changeset-bot · 2026-06-29T10:51:29Z

🦋 Changeset detected

Latest commit: 8043b34

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

This PR includes changesets to release 4 packages

Name	Type
@hyperdx/common-utils	Patch
@hyperdx/app	Patch
@hyperdx/api	Patch
@hyperdx/otel-collector	Patch

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 · 2026-06-29T10:51:31Z

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

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview, Comment	Jul 14, 2026 9:54pm
hyperdx-storybook	Ready	Preview, Comment	Jul 14, 2026 9:54pm

github-actions · 2026-06-29T10:52:04Z

🟡 Tier 3 — Standard

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

Why this tier:

Diff size: 480 production lines changed (Tier 2 max: < 250)
Cross-layer change: touches frontend (packages/app) + shared utils (packages/common-utils)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats

Production files changed: 7
Production lines changed: 480 (+ 678 in test files, excluded from tier calculation)
Branch: jordansimonovski/bugfix-ratio-grouping
Author: jordan-simonovski

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

greptile-apps · 2026-06-29T10:57:29Z

Greptile Summary

This PR fixes grouped ratio charts and metric Group By autocomplete. The main changes are:

Grouped metric ratio rows now merge by bucket and group dimensions.
Ratio calculation now carries group columns through the result.
Ratio mode support was added for per-group and share-of-total behavior.
Metric Group By autocomplete now uses fields valid across the selected metric series.
Unit tests cover grouped ratios, missing numerator or denominator rows, alias collisions, and field intersection.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the changed code.

Important Files Changed

Filename	Overview
packages/common-utils/src/clickhouse/index.ts	Updates grouped ratio merging, operand tracking, ratio mode handling, and denominator bucket totals.
packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx	Adds metric-aware Group By autocomplete props and a grouped-ratio mode toggle.
packages/app/src/components/DBEditTimeChartForm/utils.ts	Adds Group By connection selection for metric series field intersection.
packages/app/src/hooks/useMetadata.tsx	Adds intersection support for multi-table field metadata.
packages/common-utils/src/types.ts	Adds the ratio mode config schema.

_{Reviews (12): Last reviewed commit: "Merge branch 'main' into jordansimonovsk..." | Re-trigger Greptile}

github-actions · 2026-06-29T11:01:47Z

E2E Test Results

✅ All tests passed • 238 passed • 3 skipped • 1558s

Status	Count
✅ Passed	238
❌ Failed	0
⚠️ Flaky	0
⏭️ Skipped	3

Tests ran across 4 shards in parallel.

View full report →

github-actions · 2026-07-01T23:36:05Z

Deep Review

✅ No critical issues found.

No P0/P1 ship-blockers. Ungrouped ratios and the merge/dedup path are well-tested, and the grouped-ratio fix is sound for the common (time-series, string group dimension) case. The items below are recommended fixes and nits — the flagship P2 is a silently-wrong-numbers edge case reachable only through the new opt-in share-of-total mode.

🟡 P2 -- recommended

packages/common-utils/src/clickhouse/index.ts:468 -- In share_of_total mode, resolveBucketColumn falls back to inferTimestampColumn when no FIXED_TIME_BUCKET_EXPR_ALIAS exists, so a Number/Table ratio grouped by a Date-typed dimension buckets denominator totals per-date instead of by the intended grand total, producing silently wrong shares; the toggle is gated on metric-source + hasGroupBy but not on displayType, so this combination is reachable.
- Fix: When the fixed time-bucket alias is absent, use the __all__ grand-total bucket instead of falling back to the first Date-typed column, or thread an explicit hasTimeBucket flag from mergeResultSets.
- _{adversarial, correctness}
packages/common-utils/src/clickhouse/index.ts:437 -- numeratorValue = row[numerator.name] ?? 0 applies to every row, so an ungrouped time-series ratio whose numerator query omits a bucket now renders 0 where it previously produced a NaN gap, changing a path the change claims is unaffected.
- Fix: Gate the ?? 0 fallback on the presence of group columns (or mode === 'share_of_total') so ungrouped ratios keep undefined → NaN.
- _correctness
packages/common-utils/src/__tests__/clickhouse.test.ts:1147 -- No mergeResultSets test covers asymmetric group membership across the numerator/denominator splits (a group present in only one split), which is the exact zero-error-group scenario the fix targets; only computeResultSetRatio on hand-built merged rows is covered, not the merge step that must produce those single-operand rows.
- Fix: Add a mergeResultSets ratio test where numerator groups and denominator groups differ, asserting rows stay distinct and missing-operand rows resolve to 0/NaN without poisoning the bucket total.
- _{testing, adversarial, maintainability}

🔵 P3 nitpicks (9)

packages/common-utils/src/clickhouse/index.ts:521 -- Operand and merge-key value column are selected by inferNumericColumn(meta)?.[0] ("first numeric column"), a latent trap left un-hardened even though the sibling resolveBucketColumn was just added to defend the timestamp path against the analogous hazard; a future projection emitting a numeric group-by dimension before the value would mis-select the operand.
- Fix: Resolve the value column by the query builder's value alias (symmetric with FIXED_TIME_BUCKET_EXPR_ALIAS), or add an explicit invariant comment plus a regression test with a numeric group dimension ordered ahead of the value.
packages/common-utils/src/clickhouse/index.ts:557 -- The merge loop recomputes inferNumericColumn to pick the strip column, which must silently agree with the operandNames derived in the rename loop; divergence would corrupt the join.
- Fix: Carry the resolved value-column name alongside renamedResultSets and consume it in the merge loop rather than re-deriving it.
packages/common-utils/src/clickhouse/index.ts:600 -- Exported mergeResultSets reads operands positionally via const [numeratorName, denominatorName] = operandNames with a load-bearing '' sentinel and does not enforce isRatio ⇒ exactly two splits; a 3-split ratio would silently divide the first two.
- Fix: Assert exactly two non-empty operand names in the ratio branch, or model operands as a named { numerator, denominator } pair.
packages/app/src/components/DBEditTimeChartForm/utils.ts:287 -- buildGroupByConnectionProps types series as { metricType?: string; metricName?: string }[], widening the real MetricsDataType enum to string and decoupling the helper from the form's select-element type.
- Fix: Type the param from the real select-element type so metricType keeps its enum.
packages/common-utils/src/clickhouse/index.ts:412 -- denominatorLabel = denominator.name.replace(/__\d+$/, '') cosmetically strips a suffix from any legitimate operand alias that happens to end in __<digits>, mislabeling the rendered ratio column (computation unaffected).
- Fix: Only strip the suffix that was actually appended during collision renaming rather than any trailing __<digits>.
packages/common-utils/src/types.ts:1249 -- ratioMode has no .default('per_group'); the default is expressed only in the function parameter and the UI's undefined handling, so parsed configs carry undefined rather than the documented default.
- Fix: Add .default('per_group') to the schema, or document on the field that the default is applied downstream.
packages/common-utils/src/clickhouse/index.ts:455 -- resolveBucketColumn, buildBucketTotalDenominator, and the metaSet map each re-declare { name: string; type: string } inline despite the module already exporting ColumnMetaType.
- Fix: Reuse ColumnMetaType so the column-meta shape has one definition.
packages/common-utils/src/clickhouse/index.ts:573 -- The four-arm nested mergeKey ternary has two structurally identical objectHash branches, and tsBucketMap is now keyed by bucket-plus-group-dimensions rather than a timestamp, so the name misleads.
- Fix: Extract a named computeMergeKey helper with early returns and rename tsBucketMap to rowsByMergeKey.
.changeset/ratio-group-by.md:22 -- The changelog states grouped ratios "use share-of-total semantics ... rather than each group's in-group rate," but the shipped default is per_group (share-of-total is an opt-in toggle), and the patch bump accompanies a new persisted ratioMode field plus a required-argument change to exported computeResultSetRatio.
- Fix: Reword the changeset to state per-group is the default with share-of-total opt-in, and reconsider a minor bump for the new capability and exported-signature change.

Reviewers (8): correctness, adversarial, kieran-typescript, testing, maintainability, performance, api-contract, project-standards.

Testing gaps:

No mergeResultSets coverage for asymmetric group membership across numerator/denominator splits (the zero-error-group scenario).
No coverage for a numeric group-by dimension ordered ahead of the value column, nor for share_of_total on a Number/Table tile grouped by a Date-typed dimension.
No component test for the Share of total toggle gating (metric-source-only, hasGroupBy trim predicate, ratio, two series).

pulpdrew · 2026-07-06T15:11:45Z

Closes HDX-2555

pulpdrew · 2026-07-06T15:14:20Z

-  const _meta = resultSet.meta;
+export const computeResultSetRatio = (
+  resultSet: ResponseJSON<any>,
+  operands?: { numeratorName: string; denominatorName: string },


Do we need the optional here if the only caller is always passing the operator? Or is there an expected use-case in the near future which needs the inference behavior?

pulpdrew · 2026-07-06T15:21:42Z

+  // Share-of-total semantics: each group's denominator is the total of the
+  // denominator column across ALL groups in the same time bucket. So a grouped
+  // ratio shows each group's contribution to the overall ratio (the lines sum
+  // to the ungrouped value) rather than each group's own in-group rate. With no
+  // grouping there's one row per bucket, so the bucket total equals that row's
+  // denominator and the result is unchanged.


I'm curious why these semantics were chosen. My initial instinct would have been to apply the ratio within-group, for use-cases where for example you want a per-service error %. What is the motivation for this behavior instead?

For some of the use cases I was testing, I kind of wanted to determine a share of total, though you make a good point about something like error rate per service. That's an oversight by me! I'm going to toggle this behaviour

Added a ratioMode config field with two values:

per_group (default): each group divided by its own denominator. A per-service error %, which is what you'd expect. service: errors / total

share_of_total (opt-in): each group's numerator over the per-bucket total across all groups, so the lines decompose the blended rate and sum to the ungrouped value. I find this particularly useful to see how much of a total error rate would be comprised by which service.

The default is per_group, so we're not imposing the share of total interpretation. In the editor it surfaces as a "Share of total" switch that only appears when you have a ratio and a Group By.

pulpdrew · 2026-07-07T12:20:45Z

+              {fields.length === 2 &&
+                seriesReturnType === 'ratio' &&
+                hasGroupBy && (
+                  <Switch


Having the option is nice, but this seems to have no effect for non-metric type sources (since for those sources, we calculate the ratio within-group only, in the database, see renderSelectList in renderChartConfig)

the toggle is now gated on tableSource?.kind === SourceKind.Metric (in addition to seriesReturnType === 'ratio' and a Group By being set), so it only shows for the metric sources that fan out to per-series queries and get merged client-side. Left a comment in the code documenting why.

…verage

… ratios Grouped ratios had a single baked-in interpretation (share-of-total). Add a `ratioMode` config field so users pick the semantic: - per_group (default): each group divided by its own denominator (per-service error %), the intuitive "group by a ratio" meaning - share_of_total: each group's numerator over the per-bucket total across all groups, so the lines decompose the blended rate and sum to the ungrouped value Surfaces as a "Share of total" switch in the chart editor, shown only for a grouped ratio (no-op otherwise). Ungrouped ratios are identical under both modes, so no behavior change for existing charts. Also make `computeResultSetRatio`'s `operands` required and drop the numeric- column inference fallback — the only caller always passes operands explicitly; tests updated to match.

Only used within its own module; knip flags it as an unused export and the pre-commit hook (knip --no-config-hints) fails on it. Pre-existing on main; fixing here so commits on this branch can pass the hook.

ratioMode is consumed only in mergeResultSets, reached only when splitChartConfigs fans a config out to multiple queries — which happens solely for metric sources. Non-metric sources compute the ratio within-group in the DB (renderSelectList), so the toggle was a silent no-op there. Gate it on SourceKind.Metric.

…rence Address PR review on the grouped-ratio merge: - share_of_total bucketed rows via inferTimestampColumn (first Date-typed column), so a Date/DateTime group-by dimension ordered ahead of the real time bucket was mistaken for it and the per-bucket denominator was summed over the wrong column. Resolve the bucket by the query builder's FIXED_TIME_BUCKET_EXPR_ALIAS, falling back to inference only when absent. - operandNames.filter(Boolean) compacted a positional array, so a split with no inferable numeric column shifted the surviving operand into the numerator and left the denominator undefined, throwing and failing the whole chart response. Read operands positionally and return the merged rows undivided when either is missing. Adds tests for both paths plus zero-bucket-total, missing-own-denominator, and the collision rename combined with a carried-through group dimension.

pulpdrew

LGTM

github-actions Bot added the review/tier-3 Standard — full human review required label Jun 29, 2026

vercel Bot deployed to Preview – hyperdx-storybook June 29, 2026 10:53 View deployment

vercel Bot deployed to Preview – hyperdx-oss June 29, 2026 10:55 View deployment

greptile-apps Bot reviewed Jun 29, 2026

View reviewed changes

Comment thread packages/common-utils/src/clickhouse/index.ts Outdated

Comment thread packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx Outdated

vercel Bot deployed to Preview – hyperdx-storybook June 29, 2026 11:50 View deployment

vercel Bot deployed to Preview – hyperdx-oss June 29, 2026 11:51 View deployment

vercel Bot deployed to Preview – hyperdx-storybook June 29, 2026 21:01 View deployment

vercel Bot deployed to Preview – hyperdx-oss June 29, 2026 21:03 View deployment

vercel Bot deployed to Preview – hyperdx-storybook June 30, 2026 23:46 View deployment

vercel Bot deployed to Preview – hyperdx-oss June 30, 2026 23:48 View deployment

vercel Bot deployed to Preview – hyperdx-storybook July 1, 2026 23:28 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 1, 2026 23:29 View deployment

greptile-apps Bot reviewed Jul 1, 2026

View reviewed changes

Comment thread packages/app/src/components/DBEditTimeChartForm/ChartEditorControls.tsx Outdated

vercel Bot deployed to Preview – hyperdx-storybook July 2, 2026 22:25 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 2, 2026 22:25 View deployment

vercel Bot deployed to Preview – hyperdx-storybook July 3, 2026 01:34 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 3, 2026 01:35 View deployment

pulpdrew reviewed Jul 6, 2026

View reviewed changes

Comment thread packages/common-utils/src/clickhouse/index.ts

vercel Bot deployed to Preview – hyperdx-storybook July 7, 2026 03:23 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 7, 2026 03:24 View deployment

pulpdrew reviewed Jul 7, 2026

View reviewed changes

vercel Bot deployed to Preview – hyperdx-storybook July 7, 2026 20:03 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 7, 2026 20:05 View deployment

jordan-simonovski added 6 commits July 13, 2026 21:37

fix: Fixed up group by logic on ratios

caa88bd

Fixed PR comments

f4043b4

fix: Additional fixes for ratio calculations and added better test co…

7ae0df0

…verage

chore(api): drop redundant export on getAlertWindowStart

976b486

Only used within its own module; knip flags it as an unused export and the pre-commit hook (knip --no-config-hints) fails on it. Pre-existing on main; fixing here so commits on this branch can pass the hook.

vercel Bot deployed to Preview – hyperdx-storybook July 13, 2026 22:53 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 13, 2026 22:54 View deployment

jordan-simonovski force-pushed the jordansimonovski/bugfix-ratio-grouping branch from 63fd5ec to 46bbb9b Compare July 14, 2026 05:32

github-actions Bot added review/tier-4 Critical — deep review + domain expert sign-off and removed review/tier-3 Standard — full human review required labels Jul 14, 2026

vercel Bot deployed to Preview – hyperdx-storybook July 14, 2026 05:35 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 14, 2026 05:36 View deployment

vercel Bot deployed to Preview – hyperdx-storybook July 14, 2026 06:40 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 14, 2026 06:42 View deployment

pulpdrew approved these changes Jul 14, 2026

View reviewed changes

Merge branch 'main' into jordansimonovski/bugfix-ratio-grouping

8043b34

github-actions Bot added review/tier-3 Standard — full human review required and removed review/tier-4 Critical — deep review + domain expert sign-off labels Jul 14, 2026

vercel Bot deployed to Preview – hyperdx-storybook July 14, 2026 21:52 View deployment

vercel Bot deployed to Preview – hyperdx-oss July 14, 2026 21:54 View deployment

jordan-simonovski merged commit 7accfd2 into main Jul 15, 2026
23 checks passed

Uh oh!

Conversation

jordan-simonovski commented Jun 29, 2026

Problem

Root cause

Changes

Why share-of-total

Testing

Scope

Uh oh!

changeset-bot Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

vercel Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟡 Tier 3 — Standard

Uh oh!

greptile-apps Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E Test Results

Uh oh!

Uh oh!

github-actions Bot commented Jul 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deep Review

🟡 P2 -- recommended

Uh oh!

pulpdrew commented Jul 6, 2026

Uh oh!

pulpdrew Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

pulpdrew Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

jordan-simonovski Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

jordan-simonovski Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pulpdrew Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

jordan-simonovski Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

pulpdrew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Jun 29, 2026 •

edited

Loading

vercel Bot commented Jun 29, 2026 •

edited

Loading

github-actions Bot commented Jun 29, 2026 •

edited

Loading

greptile-apps Bot commented Jun 29, 2026 •

edited

Loading

github-actions Bot commented Jun 29, 2026 •

edited

Loading

github-actions Bot commented Jul 1, 2026 •

edited

Loading