-
Notifications
You must be signed in to change notification settings - Fork 196
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
Multi series support for event/metric line charts and alerts #171
Conversation
🦋 Changeset detectedLatest commit: 6fd5d7e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
packages/api/src/utils/testUtils.ts
Outdated
@@ -0,0 +1,142 @@ | |||
import * as clickhouse from '@/clickhouse'; |
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.
nit: can move this to fixtures.ts
packages/api/src/clickhouse/index.ts
Outdated
SqlString.raw( | ||
isRate | ||
? rateMetricSource | ||
: // Max/Min aggs are the same for both Sum and Gauge metrics |
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.
Not sure I fully understand this. So if Mas/Min aggs are the same for both, why do we need extra aggFn checking here ?
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 is moved from the existing getMetricsChart
function;
In a future PR I actually intend to get rid of this condition, since the sumMetricSource
and gaugeMetricSource
are nearly identical after the agg fn bug fix I talked about on Slack.
This logic was here because the min/max aggFn didn't require the more expensive sumMetricSource
query, even for sum type metrics. However, the gaugeMetricSource
and sumMetricSource
will in the future be the same query in general, so this shouldn't really matter.
@@ -1,4 +1,14 @@ | |||
import _ from 'lodash'; |
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.
Great tests!! not required in this PR, but I'd write tests for buildEventSeriesQuery
and queryMultiSeriesChart
to make sure queries are properly generated
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.
Ty - I think testing those fns are pretty low ROI test-wise compared to testing it all e2e to make sure the values are actually correct, otherwise I think those tests alone are just going to test fragile implementation details.
query, | ||
format: 'JSON', | ||
clickhouse_settings: { | ||
additional_table_filters: buildLogStreamAdditionalFilters( |
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.
hmm....this is critical and tricky. How do we apply filters separately for log and metric tables ? for now at least we need to execute two queries and merge them afterward
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.
We don't today - the series endpoint can only take series completely of logs or completely of metrics, so we shouldn't need to worry about multi-source charts for now.
I can probably add more validation logic (probably in follow up PR) to throw if that's not true. (For now it'll just create non-sensical queries, but I don't expect us to enter that state to begin with).
packages/api/src/clickhouse/index.ts
Outdated
seriesReturnType?: SeriesReturnType; | ||
}) => { | ||
let queries: { query: string; hasGroupBy: boolean }[] = []; | ||
if ('table' in series[0] && series[0].table === 'logs') { |
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 assumes all series have the same table ?
}); | ||
}; | ||
|
||
export const getMultiSeriesChartLegacyFormat = async ({ |
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.
I think we need to test this guy to make sure it outputs the same data as the original chart method
query: z.object({ | ||
endTime: z.string(), | ||
granularity: z.nativeEnum(clickhouse.Granularity).optional(), | ||
startTime: z.string(), | ||
seriesReturnType: z.optional(z.nativeEnum(clickhouse.SeriesReturnType)), | ||
}), |
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.
nit: can move this to body
later
@@ -336,8 +336,8 @@ export const processAlert = async (now: Date, alert: AlertDocument) => { | |||
// Logs Source | |||
let checksData: | |||
| Awaited<ReturnType<typeof clickhouse.checkAlert>> | |||
| Awaited<ReturnType<typeof clickhouse.getLogsChart>> | |||
| Awaited<ReturnType<typeof clickhouse.getMetricsChart>> |
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.
can also move getMetricsChart
return type
}; | ||
}; | ||
|
||
export const queryMultiSeriesChart = async ({ |
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.
I can't really read this one and it hurts my brain. I think a simple test helps (at least I can see what the output query looks like). What I can tell so far is it concats a bunch of CTE and merges them afterward
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.
Which lines were too dense to read? If the code can't be read - best that I rewrite the code to make it readable. Fwiw this function literally does just concats a bunch of CTEs and then merges them.
If you want to see the query output - I heard there's a cool tool to observe your systems ;)
|
||
const selectClause = [ | ||
isCountFn | ||
? 'toFloat64(count()) as data' |
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.
do we want to do this for other counts ?
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.
Aghhh good catch
packages/api/src/clickhouse/index.ts
Outdated
maxNumGroups: number; | ||
tableVersion: number | undefined; | ||
teamId: string; | ||
seriesReturnType?: 'ratio' | 'column'; |
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.
nit: SeriesReturnType
packages/api/src/clickhouse/index.ts
Outdated
seriesReturnType === 'column' | ||
? `greatest(${queries | ||
.map((_, i) => `series_${i}.data`) | ||
.join(', ')})` |
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.
style suggestion: any chance we can move this to the format 2nd arg ? (to improve readability)
packages/api/src/clickhouse/index.ts
Outdated
// For now only supports same-table series with the same groupBy | ||
|
||
const seriesCTEs = SqlString.raw( | ||
'WITH ' + queries.map((q, i) => `series_${i} AS (${q.query})`).join(',\n'), |
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.
style suggestion: I'd move 'WITH' to the final query so its clear that we are building a bunch of CTEs
packages/api/src/clickhouse/index.ts
Outdated
let leftJoin = ''; | ||
// Join every series after the first one | ||
for (let i = 1; i < queries.length; i++) { | ||
leftJoin += `LEFT JOIN series_${i} AS series_${i} ON series_${i}.ts_bucket=series_0.ts_bucket${ |
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.
curious why do we need 'AS' here ?
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.
more sql is not more better? 😢
f4ea1ad
to
7f667c7
Compare
if (dataType === MetricsDataType.Gauge || dataType === MetricsDataType.Sum) { | ||
selectClause.push( | ||
aggFn === AggFn.Count | ||
? 'COUNT(value) as data' |
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.
nit: I wonder if we also want to do toFloat64
here
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.
Oh good catch, we actually don't technically support the Count
aggfn in metrics (not sure why anyone would want it either). Not sure why this is here. I'll ticket this to see if we should just clean it up later instead.
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.
Great work! Very excited about this 🚢 🚢 🚢
/chart/series
endpoint that will take an array of ChartSeries (logs or metrics) and return them back inseries_${i}.data
columns. Can also return inratio
mode ifseriesReturnType
is specified and 2 series are submitted.HDXMultiSeriesTimeChart
to render a chart with multiple series. I have a draft of mostly-doneHDXMultiSeriesTableChart
as well but I figured I'd spare the PR from blowing up further.ChartSeriesFormCompact
, a hacky-ish UI to fit multiple series in an edit form easily (part ofEditMultiSeriesChartForm
which will be reused for tables).getMultiSeriesChartLegacyFormat
which will flatten a multi-series query back into the{ data:.., ts_bucket:..., group:... }
format.checkAlert.test.ts
andclickhouse.test.ts
with some util functions.