refactor(perf): offload metric ratio computation to native ClickHouse CTE - #2681
refactor(perf): offload metric ratio computation to native ClickHouse CTE#2681Aryainguz wants to merge 10 commits into
Conversation
🦋 Changeset detectedLatest commit: afe1fd7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@Aryainguz is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR moves metric ratio calculation into a native ClickHouse query path. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (9): Last reviewed commit: "Merge branch 'main' into feat/native-cli..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…/native-clickhouse-ratios
Deep Review✅ No critical issues found. The new native-CTE ratio path is SQL-injection-safe — every user-influenceable value ( 🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (12): correctness, testing, maintainability, project-standards, security, performance, api-contract, reliability, kieran-typescript, adversarial, agent-native, learnings. Testing gaps: No test asserts the new native-CTE output equals the legacy in-memory output for the same input; zero/one-sided-denominator behavior ( |
… ratio select clause
…ng suffix to duplicate q1 aliases
…g processing for ratio-based queries
…o FULL OUTER JOIN in ClickHouse queries
Aryainguz
changed the title
|
@pulpdrew please review once |
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
Thanks for the PR! This is a nice problem to solve. The metrics queries are rough though, and it looks like there are some cases which are not handled. If you could handle those cases and add tests to demonstrate that they're all covered and correct, I can re-review.
| : config; | ||
| if (isBuilderChartConfig(config)) { | ||
| config = setChartSelectsAlias(config); | ||
| if (config.seriesReturnType === 'ratio' && config.ratioMode !== 'share_of_total') { |
There was a problem hiding this comment.
Looks like there are some lint / formatting issues here - please run make ci-lint and fix any errors.
| if (!gb.alias) { | ||
| return { ...gb, alias: gb.valueExpression }; | ||
| } | ||
| return gb; |
There was a problem hiding this comment.
This triggers a latent bug in renderSelectList (used to render group by lists as well). For group-bys with 2 items in the list, with seriesReturnType === 'ratio', the two group by items are divided (in renderChartConfig).
To reproduce, try GROUP BY Attributes['http.flavor'], ServiceName on a metrics ratio.
|
|
||
| const isTimeSeries = isTimeSeriesDisplayType(config.displayType); | ||
|
|
||
| if ( |
There was a problem hiding this comment.
Could we move this query building into renderChartConfig, so that other query paths that call renderChartConfig but not queryChartConfig (eg. rendering the SQL preview) do the same rendering?
| k => chSql`${{ Identifier: k }}`, | ||
| ); | ||
| const usingClause = concatChSql(', ', joinKeysSql); | ||
| ratioSql = chSql`WITH q0 AS (${queries[0]}), q1 AS (${queries[1]}) SELECT ${selectClause} FROM q0 FULL OUTER JOIN q1 USING (${usingClause})`; |
There was a problem hiding this comment.
When viewing the ratio between two gauge metrics, grouping by Attributes['http.flavor'], I get the following error:
JOIN FULL OUTER JOIN ... USING (__hdx_time_bucket, `Attributes[\\'http.flavor\\']`) using identifier 'Attributes[\'http.flavor\']' cannot be resolved from left table expression. In scope WITH q0 AS (WITH Source AS (SELECT *, cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash FROM default.otel_metrics_gauge WHERE ((TimeUnix >= fromUnixTimestamp64Milli(_CAST(1785348960000, 'Int64'))) AND (TimeUnix < fromUnixTimestamp64Milli(_CAST(1785352620000, 'Int64')))) AND (MetricName = 'ClickHouseAsyncMetrics_OSGuestTime')), Bucketed AS (SELECT toStartOfInterval(toDateTime(TimeUnix), toIntervalMinute(1)) AS __hdx_time_bucket2, AttributesHash, last_value(Value) AS LastValue, any(ScopeAttributes) AS ScopeAttributes, any(ResourceAttributes) AS ResourceAttributes, any(Attributes) AS Attributes, any(ResourceSchemaUrl) AS ResourceSchemaUrl, any(ScopeName) AS ScopeName, any(ScopeVersion) AS ScopeVersion, any(ScopeDroppedAttrCount) AS ScopeDroppedAttrCount, any(ScopeSchemaUrl) AS ScopeSchemaUrl, any(ServiceName) AS ServiceName, any(MetricDescription) AS MetricDescription, any(MetricUnit) AS MetricUnit, any(StartTimeUnix) AS StartTimeUnix, any(Flags) AS Flags FROM Source GROUP BY AttributesHash, __hdx_time_bucket2 ORDER BY AttributesHash ASC, __hdx_time_bucket2 ASC) SELECT count() AS `1`, Attributes['http.flavor'] AS `Attributes['http.flavor']`, toStartOfInterval(toDateTime(__hdx_time_bucket2), toIntervalMinute(1)) AS __hdx_time_bucket FROM Bucketed WHERE ((__hdx_time_bucket2 >= fromUnixTimestamp64Milli(_CAST(1785348960000, 'Int64'))) AND (__hdx_time_bucket2 < fromUnixTimestamp64Milli(_CAST(1785352620000, 'Int64')))) GROUP BY Attributes['http.flavor'] AS `Attributes['http.flavor']`, toStartOfInterval(toDateTime(__hdx_time_bucket2), toIntervalMinute(1)) AS __hdx_time_bucket ORDER BY toStartOfInterval(toDateTime(__hdx_time_bucket2), toIntervalMinute(1)) AS __hdx_time_bucket ASC LIMIT _CAST(100000, 'Int32') SETTINGS short_circuit_function_evaluation = 'force_enable'), q1 AS (WITH Source AS (SELECT *, cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash FROM default.otel_metrics_gauge WHERE ((TimeUnix >= fromUnixTimestamp64Milli(_CAST(1785348960000, 'Int64'))) AND (TimeUnix < fromUnixTimestamp64Milli(_CAST(1785352620000, 'Int64')))) AND (MetricName = 'ClickHouseAsyncMetrics_OSGuestTime')), Bucketed AS (SELECT toStartOfInterval(toDateTime(TimeUnix), toIntervalMinute(1)) AS __hdx_time_bucket2, AttributesHash, last_value(Value) AS LastValue, any(ScopeAttributes) AS ScopeAttributes, any(ResourceAttributes) AS ResourceAttributes, any(Attributes) AS Attributes, any(ResourceSchemaUrl) AS ResourceSchemaUrl, any(ScopeName) AS ScopeName, any(ScopeVersion) AS ScopeVersion, any(ScopeDroppedAttrCount) AS ScopeDroppedAttrCount, any(ScopeSchemaUrl) AS ScopeSchemaUrl, any(ServiceName) AS ServiceName, any(MetricDescription) AS MetricDescription, any(MetricUnit) AS MetricUnit, any(StartTimeUnix) AS StartTimeUnix, any(Flags) AS Flags FROM Source GROUP BY AttributesHash, __hdx_time_bucket2 ORDER BY AttributesHash ASC, __hdx_time_bucket2 ASC) SELECT count() AS `2`, Attributes['http.flavor'] AS `Attributes['http.flavor']`, toStartOfInterval(toDateTime(__hdx_time_bucket2), toIntervalMinute(1)) AS __hdx_time_bucket FROM Bucketed WHERE ((__hdx_time_bucket2 >= fromUnixTimestamp64Milli(_CAST(1785348960000, 'Int64'))) AND (__hdx_time_bucket2 < fromUnixTimestamp64Milli(_CAST(1785352620000, 'Int64')))) GROUP BY Attributes['http.flavor'] AS `Attributes['http.flavor']`, toStartOfInterval(toDateTime(__hdx_time_bucket2), toIntervalMinute(1)) AS __hdx_time_bucket ORDER BY toStartOfInterval(toDateTime(__hdx_time_bucket2), toIntervalMinute(1)) AS __hdx_time_bucket ASC LIMIT _CAST(100000, 'Int32') SETTINGS short_circuit_function_evaluation = 'force_enable') SELECT (COALESCE(q0.`1`, 0) / q1.`2`) AS `1/2`, __hdx_time_bucket, `Attributes[\\'http.flavor\\']` FROM q0 FULL OUTER JOIN q1 USING (__hdx_time_bucket, `Attributes[\\'http.flavor\\']`).
| } | ||
| }); | ||
|
|
||
| it('computes ratio via native CTE when seriesReturnType is "ratio"', async () => { |
There was a problem hiding this comment.
This test is a good start, but to be confident in these changes, I think we'd want to create tests that include all of the following:
- Each test is hitting a real ClickHouse integration test container (eg. don't mock clickhouse) so that the generated query is being validated as correct against ClickHouse.
- Each test should seed a small amount of data to be queried, so that the calculations can be validated against an expected ratio value. See the recently added histogram metrics integration tests for examples
- Each metric type (gauge, sum, histogram, exponential histogram) should be tested. Each should be tested with and without a group by, and the group by should include
Attributesvalues and table columns. - Edge cases (divide by 0, missing numerator or demoninator, etc) should all be tested.
2 participants
Summary
the metric ratios computation in @hyperdx/common-utils to execute natively inside ClickHouse via CTEs instead of fetching two separate streams and zipping them in memory using Node.js. Updated tests, changeset file as per the changes.
Why this change was made:
Currently, when evaluating metric ratio charts (e.g. dividing the sum of one metric by the sum of another), the backend executes two separate queries concurrently. The results are then loaded entirely into Node.js memory, zipped by their respective series, and the mathematical ratio calculation is executed sequentially in JavaScript.
For heavy, high-cardinality charts such as time series distributions across thousands of service.names this leads to:
Fixes #2680
Local Validations:
