fix: change metrics PK for better memory usage

[knudtty]

Summary

Change the default schema and PK for metrics to reduce memory usage and introduce a lightweight minmax for better time filtering.

References

• Linear Issue: Closes HDX-4695

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview, Comment	Jul 2, 2026 2:11pm
hyperdx-storybook	Ready	Preview, Comment	Jul 2, 2026 2:11pm

[changeset-bot]

🦋 Changeset detected

Latest commit: 4e5a5f1

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	Minor
@hyperdx/otel-collector	Minor
@hyperdx/api	Minor
@hyperdx/app	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

[github-actions]

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

• Critical-path files (1):
  • docker/otel-collector/schema/seed/00003_otel_metrics.sql

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats

• Production files changed: 2
• Production lines changed: 83 (+ 11 in test files, excluded from tier calculation)
• Branch: aaron/metrics-reduce-pk-memory
• Author: knudtty

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

[greptile-apps]

Greptile Summary

This PR optimises the ClickHouse metrics schema to reduce primary-key memory usage and improve time-range pruning, paired with a query-generation fix that correctly applies toStartOfHour wrapping when includedDataInterval and a toStartOf* PK expression are both present.

• Schema (00003_otel_metrics.sql): All five metrics tables are updated — DateTime64(9) is downgraded to DateTime (second precision), MetricName gains LowCardinality, the Attributes Map is replaced in the ORDER BY with its cityHash64 hash (smaller PK footprint), toStartOfHour(TimeUnix) is added ahead of the hash for hour-bucket pruning, and a minmax skip index on TimeUnix is introduced across all tables.
• Query generation (renderChartConfig.ts): Refactors startTimeCond/endTimeCond into a two-step rawBound → toStartOf(rawBound) pattern so that interval-expanded bounds are also wrapped in the PK function, preventing false-negative granule skips when the expanded start falls in an earlier hour bucket.

Confidence Score: 5/5

Safe to merge — schema changes apply to fresh installations only and the query-generation fix is well-covered by the new test case.

All five metrics tables receive the same consistent treatment. The Delta codec without an explicit byte-width correctly auto-sizes to 4 bytes for DateTime columns. The renderChartConfig refactor only changes behavior for the previously-unhandled case where both includedDataInterval and a toStartOf* PK expression are active, and that case is now exercised by the new test.

No files require special attention.

Important Files Changed

Filename	Overview
docker/otel-collector/schema/seed/00003_otel_metrics.sql	Schema updated across all five metrics tables: DateTime64(9)→DateTime, LowCardinality(MetricName), cityHash64(Attributes) in ORDER BY, toStartOfHour(TimeUnix) sort prefix, and minmax skip index added. Delta codec correctly drops explicit byte-width (auto-sizes to 4 for DateTime). No inline ALTER TABLE migration — CREATE TABLE IF NOT EXISTS means existing installations will not pick up these changes automatically.
packages/common-utils/src/core/renderChartConfig.ts	Refactors time bound computation into rawStartBound/rawEndBound, then conditionally wraps with toStartOf. The only behavioral change is the previously-unhandled case where both includedDataInterval and a toStartOf* PK expression are set — the raw interval-expanded bound is now correctly wrapped, which is proven by the new test case.
packages/common-utils/src/tests/renderChartConfig.test.ts	Adds one targeted test case covering includedDataInterval + toStartOfHour(timestamp) combination; expected SQL is correct and demonstrates the fix.
.changeset/proud-shrimps-clean.md	Changeset entry marking both packages as minor bumps; consistent with the schema and query-generation changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[timeFilterExpr called] --> B{includedDataInterval set?}
    B -- Yes --> C["rawBound = toStartOfInterval(fromUnixTimestamp64Milli(t), INTERVAL X) ± INTERVAL X"]
    B -- No --> D["rawBound = fromUnixTimestamp64Milli(t)"]
    C --> E{toStartOf* in PK expr?}
    D --> E
    E -- Yes --> F["timeCond = toStartOfHour(rawBound)\n(wraps interval-expanded bound for PK pruning)"]
    E -- No --> G["timeCond = rawBound"]
    F --> H{isDateType?}
    G --> H
    H -- Yes --> I["WHERE col >= toDate(timeCond)\nAND col <= toDate(timeCond)"]
    H -- No --> J["WHERE col >= timeCond\nAND col <= timeCond"]
    I --> K[AND all sub-expressions together]
    J --> K

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[timeFilterExpr called] --> B{includedDataInterval set?}
    B -- Yes --> C["rawBound = toStartOfInterval(fromUnixTimestamp64Milli(t), INTERVAL X) ± INTERVAL X"]
    B -- No --> D["rawBound = fromUnixTimestamp64Milli(t)"]
    C --> E{toStartOf* in PK expr?}
    D --> E
    E -- Yes --> F["timeCond = toStartOfHour(rawBound)\n(wraps interval-expanded bound for PK pruning)"]
    E -- No --> G["timeCond = rawBound"]
    F --> H{isDateType?}
    G --> H
    H -- Yes --> I["WHERE col >= toDate(timeCond)\nAND col <= toDate(timeCond)"]
    H -- No --> J["WHERE col >= timeCond\nAND col <= timeCond"]
    I --> K[AND all sub-expressions together]
    J --> K

Loading

_{Reviews (6): Last reviewed commit: "Merge branch 'main' into aaron/metrics-r..." | Re-trigger Greptile}

[github-actions]

E2E Test Results

✅ All tests passed • 221 passed • 3 skipped • 1447s

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

Tests ran across 4 shards in parallel.

View full report →

[knudtty]

Changing to LowCardinality reduces PK memory usage by 2-3x

[iShiBin]

Seems to be a good change. I wonder why not apply it to the MetricName to all other metrics tables?

[knudtty]

Good callout. That was a mistake.

[iShiBin]

Glad it was addressed. thanks.

[knudtty]

Second precision is an appropriate default and reduces the storage unit from 8 bytes -> 4 bytes

[knudtty]

minmax is an extremely lightweight index and helps efficiently prune granules, even if the PK is not fully qualified for a query to take advantage of TimeUnix

[knudtty]

Time bucketing before the high cardinality Attributes column groups data more efficiently speeding up I/O for queries due to less overall ranges and is very efficient in PK memory

[knudtty]

Hashing the Attributes column massively reduces PK memory

[iShiBin]

why is that? Is it because some attribute key or values are too long?

[knudtty]

Yes, both the length of keys or values as well as the number in a single entry. Additionally, ClickHouse does well with known sized parameters, like the 64 bit hash value

[iShiBin]

Got it and thanks.

[github-actions]

Deep Review

Scope: Metrics ClickHouse schema redesign (00003_otel_metrics.sql) + a timeFilterExpr fix in renderChartConfig.ts and one new test. Base 39e062f0, report-only mode.

Intent (author-stated, advisory): reduce metrics primary-key memory usage via a new ORDER BY, LowCardinality(MetricName), DateTime64(9)→DateTime downgrade, and a minmax time index; the query-side fix wraps includedDataInterval-expanded bounds in the PK's toStartOf function.

The timeFilterExpr refactor itself is correct and well-tested — the new rawStartBound/rawEndBound → toStartOf-wrapping is provably equivalent to prior behavior in every branch, and the new AND'd raw-column bound keeps the coarse toStartOfHour filter from over-selecting. Findings below concern the schema change's downstream effects.

🔴 P0/P1 — must fix

• docker/otel-collector/schema/seed/00003_otel_metrics.sql:15 — Downgrading TimeUnix from DateTime64(9) to DateTime (second resolution) makes multiple same-second samples in one series collide, and the Sum/rate CTE orders its window functions strictly by TimeUnix (renderChartConfig.ts:1761 lagInFrame(...) OVER (PARTITION BY AttributesHash ORDER BY TimeUnix), :1765 running SUM, :1810 argMax(Source.Sum, TimeUnix)), so tied timestamps order non-deterministically and yield wrong rate/increase and last-value results that the old nanosecond precision prevented.
  • Fix: Verify realistic sub-second per-series ingest cadence and either retain sub-second precision on TimeUnix for the counter tables or add a deterministic secondary sort key (e.g. an ingest ordinal) to the window ORDER BY.
  • _{adversarial, performance, correctness}

🟡 P2 — recommended

• docker/otel-collector/schema/seed/00003_otel_metrics.sql:2 — All five tables use CREATE TABLE IF NOT EXISTS in an in-place-edited goose seed run with WithNoVersioning, so on already-provisioned deployments the DDL is a no-op and ClickHouse cannot ALTER a MergeTree ORDER BY/primary key; existing installs keep the old schema forever while only fresh installs get the memory win, producing permanent fleet drift with no delivered benefit.
  • Fix: Add an explicit upgrade migration (create-new-table + INSERT SELECT + RENAME swap) or document in the changeset/release notes that existing deployments require manual migration.
  • _{data-migrations, maintainability, adversarial}
• docker/otel-collector/schema/seed/00003_otel_metrics.sql:35 — The new ORDER BY hashes only cityHash64(Attributes), but the memory-sensitive Sum/Gauge window queries partition on cityHash64(ScopeAttributes, ResourceAttributes, Attributes) (renderChartConfig.ts:1643), so physical row order need not align with the query partition key and the intended streaming/memory benefit may not materialize when Scope/Resource attributes vary.
  • Fix: Confirm via EXPLAIN PIPELINE on representative data that the window functions stream without a full re-sort, or include the same three-map hash in the ORDER BY.
  • _{performance, adversarial}
• docker/otel-collector/schema/seed/00003_otel_metrics.sql:21 — Exemplars.TimeUnix is downgraded DateTime64(9)→DateTime even though it is not part of the ORDER BY and contributes nothing to the PK-memory goal, collapsing sub-second exemplars to one timestamp and degrading exemplar-to-span/trace time correlation.
  • Fix: Keep Exemplars.TimeUnix at DateTime64(9) unless second-granularity correlation is explicitly acceptable.
• packages/common-utils/src/__tests__/renderChartConfig.test.ts:2098 — The added test hardcodes the already-compound expression timestamp, toStartOfHour(timestamp) and never drives optimizeTimestampValueExpression against the real new PK string (cityHash64(Attributes) interposed between toStartOfHour(TimeUnix) and TimeUnix), and no integration test verifies time-filtered query correctness against the actual reshaped tables.
  • Fix: Add a timeFilterExpr case using the real new PK shape plus includedDataInterval, and a metrics integration/smoke test that queries across hour boundaries.
  • _{testing, correctness}

🔵 P3 nitpicks (3)

• packages/common-utils/src/core/renderChartConfig.ts:899 — The rationale for re-wrapping the includedDataInterval-widened bound in the PK's toStartOf lives only in a test description.
  • Fix: Add a short comment above rawStartBound/rawEndBound explaining the combined widening + toStartOf case.
• packages/common-utils/src/core/renderChartConfig.ts:907 — A PK toStartOf carrying remaining args (e.g. toStartOfInterval(col, INTERVAL n)) combined with includedDataInterval is unexercised, leaving the nested-interval SQL unverified.
  • Fix: Add a test case for a non-empty formattedRemainingArgs toStartOf combined with includedDataInterval.
• packages/common-utils/src/__tests__/renderChartConfig.test.ts:1897 — A Date-typed column combined with includedDataInterval (the toDate()-wrapped expanded-bound path) has no coverage.
  • Fix: Add a Date-column + includedDataInterval test case.

---

Reviewers (10): correctness, adversarial, data-migrations, performance, kieran-typescript, testing, maintainability, project-standards, agent-native, learnings-researcher.

Testing gaps: No fixture exercises the rate/cumulative CTE against same-second-truncated TimeUnix ties; no end-to-end verification that an upgrade over pre-existing tables adopts (or documents not adopting) the new schema; no query-correctness test against the old PK shape for deployments that stay on it.

[pulpdrew]

changeset LGTM