-
Notifications
You must be signed in to change notification settings - Fork 439
refactor(perf): offload metric ratio computation to native ClickHouse CTE #2681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0efa560
f0b61f5
e5e3ed4
652d2db
ae2a6a1
66fcce0
1f714bb
4c2d7f0
7363869
afe1fd7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@hyperdx/common-utils": patch | ||
| --- | ||
|
|
||
| Refactor metric ratios to use native ClickHouse CTE |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -834,9 +834,30 @@ export abstract class BaseClickhouseClient { | |
| }; | ||
| querySettings: QuerySettings | undefined; | ||
| }): Promise<ResponseJSON<Record<string, string | number>>> { | ||
| config = isBuilderChartConfig(config) | ||
| ? setChartSelectsAlias(config) | ||
| : config; | ||
| if (isBuilderChartConfig(config)) { | ||
| config = setChartSelectsAlias(config); | ||
| if (config.seriesReturnType === 'ratio' && config.ratioMode !== 'share_of_total') { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like there are some lint / formatting issues here - please run |
||
| if (config.groupBy) { | ||
| if (typeof config.groupBy === 'string') { | ||
| config.groupBy = splitAndTrimWithBracket(config.groupBy).map(gb => ({ | ||
| type: 'string', | ||
| valueExpression: gb, | ||
| alias: gb, // Assign the raw expression as the alias so the CTE outputs exactly this column name | ||
| })); | ||
| } else if (Array.isArray(config.groupBy)) { | ||
| config.groupBy = config.groupBy.map(gb => { | ||
| if (typeof gb === 'string') { | ||
| return { type: 'string', valueExpression: gb, alias: gb }; | ||
| } | ||
| if (!gb.alias) { | ||
| return { ...gb, alias: gb.valueExpression }; | ||
| } | ||
| return gb; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| const queries: ChSql[] = await Promise.all( | ||
| splitChartConfigs(config).map(c => | ||
| renderChartConfig(c, metadata, querySettings), | ||
|
|
@@ -845,6 +866,63 @@ export abstract class BaseClickhouseClient { | |
|
|
||
| const isTimeSeries = isTimeSeriesDisplayType(config.displayType); | ||
|
|
||
| if ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| isBuilderChartConfig(config) && | ||
| config.seriesReturnType === 'ratio' && | ||
| config.ratioMode !== 'share_of_total' && | ||
| queries.length === 2 && | ||
| Array.isArray(config.select) | ||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
| ) { | ||
| const q0Alias = config.select[0].alias ?? 'q0_val'; | ||
| const originalQ1Alias = config.select[1].alias ?? 'q1_val'; | ||
| const ratioAlias = `${q0Alias}/${originalQ1Alias}`; | ||
|
|
||
| const joinKeys: string[] = []; | ||
| if (isTimeSeries) { | ||
| joinKeys.push('__hdx_time_bucket'); | ||
| } | ||
| if (config.groupBy) { | ||
| for (const gb of config.groupBy) { | ||
| if (typeof gb === 'string') { | ||
| joinKeys.push(gb); | ||
| } else { | ||
| joinKeys.push(gb.alias || gb.valueExpression); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
greptile-apps[bot] marked this conversation as resolved.
|
||
| // De-duplicate join keys just in case | ||
| const uniqueJoinKeys = Array.from(new Set(joinKeys)); | ||
|
|
||
| let ratioSql: ChSql; | ||
| const selectCols = [ | ||
| chSql`(COALESCE(q0.${{ Identifier: q0Alias }}, 0) / q1.${{ Identifier: originalQ1Alias }}) AS ${{ Identifier: ratioAlias }}`, | ||
| ...uniqueJoinKeys.map(k => chSql`${{ Identifier: k }}`), | ||
|
Aryainguz marked this conversation as resolved.
|
||
| ]; | ||
|
Aryainguz marked this conversation as resolved.
|
||
| const selectClause = concatChSql(', ', selectCols); | ||
|
|
||
| if (uniqueJoinKeys.length > 0) { | ||
| const joinKeysSql = uniqueJoinKeys.map( | ||
| 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})`; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When viewing the ratio between two gauge metrics, grouping by |
||
| } else { | ||
| ratioSql = chSql`WITH q0 AS (${queries[0]}), q1 AS (${queries[1]}) SELECT ${selectClause} FROM q0 CROSS JOIN q1`; | ||
| } | ||
|
|
||
| const resp = await this.query<'JSON'>({ | ||
| query: ratioSql.sql, | ||
| query_params: ratioSql.params, | ||
| format: 'JSON', | ||
| abort_signal: opts?.abort_signal, | ||
| connectionId: config.connection, | ||
| clickhouse_settings: opts?.clickhouse_settings, | ||
| }); | ||
|
|
||
| return resp.json<any>(); | ||
| } | ||
|
|
||
| const resultSets = await Promise.all( | ||
| queries.map(async query => { | ||
| const resp = await this.query<'JSON'>({ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Attributesvalues and table columns.