[HDX-5076] Expand int test coverage for multi-series metric merge (regression baseline) - #2858
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Greptile SummaryThis PR adds black-box integration coverage that establishes the current multi-series metric merge behavior as a baseline for its planned ClickHouse-side rewrite.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/tests/queryChartConfig.int.test.ts | Adds deterministic ClickHouse fixtures and integration scenarios covering multi-series metric merging without changing production behavior. |
Reviews (3): Last reviewed commit: "test: pin multi-series metric merge beha..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| expect(Number(ratio)).toBeCloseTo(17 / 68, 5); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This block adds roughly 740 lines to an already large test module, bringing it to about 1,633 lines and making fixture ownership, cleanup, and individual scenarios harder to maintain; move the suite into a dedicated integration-test file.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Deep Review✅ No critical issues found. This is a test-only PR: one file, ~776 insertions, zero production code changes. The new 🟡 P2 -- recommended
🔵 P3 nitpicks (2)
Reviewers (6): correctness, testing, maintainability, project-standards, kieran-typescript, learnings-researcher. Testing gaps:
|
E2E Test Results✅ All tests passed • 276 passed • 1 skipped • 1101s
Tests ran across 4 shards in parallel. |
f0fd6a4 to
e8b8ac0
Compare
Regression baseline for HDX-5077 (moving the node-side mergeResultSets /
computeResultSetRatio merge into a single composed ClickHouse query).
Pins the observable contract of queryChartConfig for multi-series metric
charts end-to-end:
- full-outer join on time buckets (and group-by values when grouped),
with gaps (nullish/NaN, never 0) where a series has no data
- meta lists value columns first in select order (useChartNumberFormats
positional contract), with numeric type class
- ratio semantics: column named <num>/<denom> replaces the operands,
missing numerator reads 0, missing/zero denominator is a gap,
share_of_total divides by the per-bucket denominator total
- same-alias series disambiguated with a __{splitIndex} suffix (and the
suffix stripped from the ratio label)
- gauge+sum (increase) series from different tables join on bucket/group;
grouped histogram series keep their Array "group" column and never
join with gauge/sum rows
- table and number display shapes merge on group values / a single row
e8b8ac0 to
227416f
Compare
| ) ENGINE = MergeTree PARTITION BY toDate(TimeUnix) ${OTEL_ORDER_BY}`, | ||
| }); | ||
| await client.command({ | ||
| query: `CREATE OR REPLACE TABLE ${DATABASE}.${HIST_TABLE} ( |
There was a problem hiding this comment.
Since we have support for Exponential Histogram now, it would be nice if these tests covered that metric type as well! There are existing integration tests, but probably none that cover the multi-series case.
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
LGTM, but it would be good to cover Exp. Histogram as well since that's a different query.
2 participants
Why
HDX-5076 — regression baseline for HDX-5077 (moving the node-side multi-series metric merge into a single composed ClickHouse query).
Today a metric chart with N series fans out into N ClickHouse queries that are merged node/browser-side in
mergeResultSets/computeResultSetRatio. That merge's semantics are only covered by unit tests that will be deleted along with the code they test. This PR pins the current behavior with black-box integration tests throughqueryChartConfig, written against the current implementation, so the HDX-5077 rewrite must reproduce it exactly.What
12 new int tests in
packages/common-utils/src/__tests__/queryChartConfig.int.test.ts(new gauge/sum/histogram fixture tables, deterministic values):seriesReturnType: 'ratio'): ungrouped + groupedper_group+ groupedshare_of_total; missing numerator → 0, missing/zero denominator → gap; output column named<num>/<denom>, operand columns stripped__{splitIndex}suffix; suffix stripped from the ratio labelincrease) from different physical tables join on bucket (and group); grouped histogram rows keep their Arraygroupcolumn and never join with gauge/sum rowsuseChartNumberFormats), numeric type class viaconvertCHDataTypeToJSTypeGaps are asserted via a tolerance helper (
nullish or NaN, never 0) rather than pinning the exact nullish representation — the current merge yields an absent key / JSNaN, a SQL-side merge yields JSONnull, and every consumer treats all three identically (formatResponseForTimeChartcoerces both toNaN).Test plan
make dev-int-common-utils FILE=queryChartConfig— 26/26 pass (12 new + 14 pre-existing)ci:lintclean (0 errors, warning count unchanged at baseline)No production code changes.