From 2910461669f12e26ae04b24d1601fe2ebf908dac Mon Sep 17 00:00:00 2001 From: Mike Shi Date: Thu, 4 Jan 2024 15:02:58 -0800 Subject: [PATCH] Bug fix: Restore dashboard filters, fetch log property type mappings correctly for charts (#182) --- .changeset/famous-walls-brake.md | 7 +++ .../clickhouse/__tests__/clickhouse.test.ts | 20 ++---- packages/api/src/clickhouse/index.ts | 57 ++++++++++++----- packages/api/src/routers/api/chart.ts | 38 ----------- packages/api/src/tasks/checkAlerts.ts | 8 --- packages/app/src/ChartUtils.tsx | 63 ++++++++++++++----- packages/app/src/DashboardPage.tsx | 32 +++++----- packages/app/src/HDXLineChart.tsx | 2 +- packages/app/styles/HDXLineChart.module.scss | 6 +- 9 files changed, 120 insertions(+), 113 deletions(-) create mode 100644 .changeset/famous-walls-brake.md diff --git a/.changeset/famous-walls-brake.md b/.changeset/famous-walls-brake.md new file mode 100644 index 000000000..d2a33aa20 --- /dev/null +++ b/.changeset/famous-walls-brake.md @@ -0,0 +1,7 @@ +--- +'@hyperdx/api': patch +'@hyperdx/app': patch +--- + +Bug fix: Restore dashboard filters, use correct field lookup for metrics, and +remove extra log property type mapping fetches. diff --git a/packages/api/src/clickhouse/__tests__/clickhouse.test.ts b/packages/api/src/clickhouse/__tests__/clickhouse.test.ts index 9157cf8ba..1f9b970b2 100644 --- a/packages/api/src/clickhouse/__tests__/clickhouse.test.ts +++ b/packages/api/src/clickhouse/__tests__/clickhouse.test.ts @@ -169,7 +169,7 @@ Array [ }), ]); - const propertyTypeMappingsModel = mockLogsPropertyTypeMappingsModel({ + mockLogsPropertyTypeMappingsModel({ testGroup: 'string', awesomeNumber: 'number', runId: 'string', @@ -202,7 +202,6 @@ Array [ granularity: '5 minute', maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => { return _.pick(d, [ @@ -264,7 +263,6 @@ Array [ granularity: '5 minute', maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Ratio, - propertyTypeMappingsModel, }) ).data.map(d => { return _.pick(d, ['group', 'series_0.data', 'ts_bucket']); @@ -388,7 +386,7 @@ Array [ }), ); - const propertyTypeMappingsModel = mockLogsPropertyTypeMappingsModel({}); + mockLogsPropertyTypeMappingsModel({}); mockSpyMetricPropertyTypeMappingsModel({ runId: 'string', @@ -415,7 +413,6 @@ Array [ granularity: '5 minute', maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => { return _.pick(d, ['group', 'series_0.data', 'ts_bucket']); @@ -466,7 +463,6 @@ Array [ granularity: '5 minute', maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => { return _.pick(d, ['group', 'series_0.data', 'ts_bucket']); @@ -517,7 +513,6 @@ Array [ granularity: '5 minute', maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => { return _.pick(d, ['group', 'series_0.data', 'ts_bucket']); @@ -558,7 +553,6 @@ Array [ granularity: '5 minute', maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => { return _.pick(d, ['group', 'series_0.data', 'ts_bucket']); @@ -608,7 +602,6 @@ Array [ granularity: '5 minute', maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Ratio, - propertyTypeMappingsModel, }) ).data.map(d => { return _.pick(d, ['group', 'series_0.data', 'ts_bucket']); @@ -746,7 +739,7 @@ Array [ }), ); - const propertyTypeMappingsModel = mockLogsPropertyTypeMappingsModel({}); + mockLogsPropertyTypeMappingsModel({}); mockSpyMetricPropertyTypeMappingsModel({ runId: 'string', @@ -782,7 +775,6 @@ Array [ granularity: undefined, maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => { return _.pick(d, [ @@ -834,7 +826,7 @@ Array [ ]), ); - const propertyTypeMappingsModel = mockLogsPropertyTypeMappingsModel({ + mockLogsPropertyTypeMappingsModel({ testGroup: 'string', awesomeNumber: 'number', runId: 'string', @@ -869,7 +861,6 @@ Array [ granularity: '5 minute', maxNumGroups: 3, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => _.pick(d, ['series_0.data', 'series_1.data', 'group', 'ts_bucket']), @@ -945,7 +936,6 @@ Array [ granularity: '5 minute', maxNumGroups: 3, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => _.pick(d, ['series_0.data', 'series_1.data', 'group', 'ts_bucket']), @@ -1021,7 +1011,6 @@ Array [ granularity: '5 minute', maxNumGroups: 3, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data.map(d => _.pick(d, ['series_0.data', 'series_1.data', 'group', 'ts_bucket']), @@ -1142,7 +1131,6 @@ Array [ granularity: '5 minute', maxNumGroups: 20, seriesReturnType: clickhouse.SeriesReturnType.Column, - propertyTypeMappingsModel, }) ).data; diff --git a/packages/api/src/clickhouse/index.ts b/packages/api/src/clickhouse/index.ts index f4bb6ad5d..ccd3af299 100644 --- a/packages/api/src/clickhouse/index.ts +++ b/packages/api/src/clickhouse/index.ts @@ -869,6 +869,7 @@ export const buildMetricSeriesQuery = async ({ startTime, teamId, sortOrder, + propertyTypeMappingsModel, }: { aggFn: AggFn; dataType: MetricsDataType; @@ -880,12 +881,9 @@ export const buildMetricSeriesQuery = async ({ startTime: number; // unix in ms teamId: string; sortOrder?: 'asc' | 'desc'; + propertyTypeMappingsModel: MetricsPropertyTypeMappingsModel; }) => { const tableName = `default.${TableName.Metric}`; - const propertyTypeMappingsModel = await buildMetricsPropertyTypeMappingsModel( - undefined, // default version - teamId, - ); const isRate = isRateAggFn(aggFn); @@ -1299,12 +1297,6 @@ export const queryMultiSeriesChart = async ({ const rows = await client.query({ query, format: 'JSON', - clickhouse_settings: { - additional_table_filters: buildLogStreamAdditionalFilters( - tableVersion, - teamId, - ), - }, }); const result = await rows.json< @@ -1322,7 +1314,6 @@ export const getMultiSeriesChart = async ({ endTime, granularity, maxNumGroups, - propertyTypeMappingsModel, startTime, tableVersion, teamId, @@ -1333,7 +1324,6 @@ export const getMultiSeriesChart = async ({ startTime: number; // unix in ms granularity: string | undefined; // can be undefined in the number chart maxNumGroups: number; - propertyTypeMappingsModel?: LogsPropertyTypeMappingsModel; tableVersion: number | undefined; teamId: string; seriesReturnType?: SeriesReturnType; @@ -1349,8 +1339,37 @@ export const getMultiSeriesChart = async ({ (series[0].table === 'logs' || series[0].table == null)) || !('table' in series[0]) ) { - if (propertyTypeMappingsModel == null) { - throw new Error('propertyTypeMappingsModel is required for logs chart'); + const propertyTypeMappingsModel = await buildLogsPropertyTypeMappingsModel( + tableVersion, + teamId.toString(), + startTime, + endTime, + ); + + const propertySet = new Set(); + series.map(s => { + if ('field' in s && s.field != null) { + propertySet.add(s.field); + } + if ('groupBy' in s && s.groupBy.length > 0) { + s.groupBy.map(g => propertySet.add(g)); + } + }); + + // Hack to refresh property cache if needed + const properties = Array.from(propertySet); + + if ( + properties.some(p => { + return !doesLogsPropertyExist(p, propertyTypeMappingsModel); + }) + ) { + logger.warn({ + message: `getChart: Property type mappings cache is out of date (${properties.join( + ', ', + )})`, + }); + await propertyTypeMappingsModel.refresh(); } queries = await Promise.all( @@ -1378,6 +1397,12 @@ export const getMultiSeriesChart = async ({ }), ); } else if ('table' in series[0] && series[0].table === 'metrics') { + const propertyTypeMappingsModel = + await buildMetricsPropertyTypeMappingsModel( + undefined, // default version + teamId, + ); + queries = await Promise.all( series.map(s => { if (s.type != 'time' && s.type != 'table') { @@ -1404,6 +1429,7 @@ export const getMultiSeriesChart = async ({ startTime, teamId, dataType: s.metricDataType, + propertyTypeMappingsModel, }); }), ); @@ -1423,7 +1449,6 @@ export const getMultiSeriesChartLegacyFormat = async ({ endTime, granularity, maxNumGroups, - propertyTypeMappingsModel, startTime, tableVersion, teamId, @@ -1434,7 +1459,6 @@ export const getMultiSeriesChartLegacyFormat = async ({ startTime: number; // unix in ms granularity: string | undefined; // can be undefined in the number chart maxNumGroups: number; - propertyTypeMappingsModel?: LogsPropertyTypeMappingsModel; tableVersion: number | undefined; teamId: string; seriesReturnType?: SeriesReturnType; @@ -1444,7 +1468,6 @@ export const getMultiSeriesChartLegacyFormat = async ({ endTime, granularity, maxNumGroups, - propertyTypeMappingsModel, startTime, tableVersion, teamId, diff --git a/packages/api/src/routers/api/chart.ts b/packages/api/src/routers/api/chart.ts index ba4324700..3058ab72e 100644 --- a/packages/api/src/routers/api/chart.ts +++ b/packages/api/src/routers/api/chart.ts @@ -40,43 +40,6 @@ router.post( return res.sendStatus(403); } - const propertyTypeMappingsModel = - await clickhouse.buildLogsPropertyTypeMappingsModel( - team.logStreamTableVersion, - teamId.toString(), - startTime, - endTime, - ); - - const propertySet = new Set(); - series.map(s => { - if ('field' in s && s.field != null) { - propertySet.add(s.field); - } - if ('groupBy' in s && s.groupBy.length > 0) { - s.groupBy.map(g => propertySet.add(g)); - } - }); - - // Hack to refresh property cache if needed - const properties = Array.from(propertySet); - - if ( - properties.some(p => { - return !clickhouse.doesLogsPropertyExist( - p, - propertyTypeMappingsModel, - ); - }) - ) { - logger.warn({ - message: `getChart: Property type mappings cache is out of date (${properties.join( - ', ', - )})`, - }); - await propertyTypeMappingsModel.refresh(); - } - // TODO: expose this to the frontend const MAX_NUM_GROUPS = 20; @@ -86,7 +49,6 @@ router.post( endTime: endTime, granularity, maxNumGroups: MAX_NUM_GROUPS, - propertyTypeMappingsModel, startTime: startTime, tableVersion: team.logStreamTableVersion, teamId: teamId.toString(), diff --git a/packages/api/src/tasks/checkAlerts.ts b/packages/api/src/tasks/checkAlerts.ts index 6d3627a8c..889e917ee 100644 --- a/packages/api/src/tasks/checkAlerts.ts +++ b/packages/api/src/tasks/checkAlerts.ts @@ -394,20 +394,12 @@ export const processAlert = async (now: Date, alert: AlertDocument) => { targetDashboard = dashboard; const startTimeMs = fns.getTime(checkStartTime); const endTimeMs = fns.getTime(checkEndTime); - const propertyTypeMappingsModel = - await clickhouse.buildLogsPropertyTypeMappingsModel( - dashboard.team.logStreamTableVersion, - dashboard.team._id.toString(), - startTimeMs, - endTimeMs, - ); checksData = await clickhouse.getMultiSeriesChartLegacyFormat({ series: chart.series, endTime: endTimeMs, granularity: `${windowSizeInMins} minute`, maxNumGroups: MAX_NUM_GROUPS, - propertyTypeMappingsModel, startTime: startTimeMs, tableVersion: dashboard.team.logStreamTableVersion, teamId: dashboard.team._id.toString(), diff --git a/packages/app/src/ChartUtils.tsx b/packages/app/src/ChartUtils.tsx index 5ec93acaa..b8ed2c83a 100644 --- a/packages/app/src/ChartUtils.tsx +++ b/packages/app/src/ChartUtils.tsx @@ -64,18 +64,32 @@ export type Granularity = | '7 day' | '30 day'; -const seriesDisplayName = (s: ChartSeries) => - s.type === 'time' || s.type === 'table' - ? `${s.aggFn}${ - s.aggFn !== 'count' - ? `(${ - s.table === 'metrics' - ? s.field?.split(' - ')?.[0] ?? s.field - : s.field - })` - : '()' - }${s.where ? `{${s.where}}` : ''}` - : ''; +const seriesDisplayName = ( + s: ChartSeries, + { + showAggFn, + showField, + showWhere, + }: { + showAggFn?: boolean; + showField?: boolean; + showWhere?: boolean; + } = {}, +) => { + if (s.type === 'time' || s.type === 'table') { + const displayField = + s.aggFn !== 'count' + ? s.table === 'metrics' + ? s.field?.split(' - ')?.[0] ?? s.field + : s.field + : ''; + + return `${showAggFn === false ? '' : s.aggFn}${ + showField === false ? '' : `(${displayField})` + }${s.where && showWhere !== false ? `{${s.where}}` : ''}`; + } + return ''; +}; export function seriesColumns({ series, @@ -84,14 +98,28 @@ export function seriesColumns({ seriesReturnType: 'ratio' | 'column'; series: ChartSeries[]; }) { + const uniqueWhere = new Set( + series.map(s => ('where' in s ? s.where : undefined)), + ); + const uniqueFields = new Set( + series.map(s => ('field' in s ? s.field : undefined)), + ); + + const showField = uniqueFields.size > 1; + const showWhere = uniqueWhere.size > 1; + const seriesMeta = seriesReturnType === 'ratio' ? [ { dataKey: `series_0.data`, - displayName: `${seriesDisplayName(series[0])}/${seriesDisplayName( - series[1], - )}`, + displayName: `${seriesDisplayName(series[0], { + showField, + showWhere, + })}/${seriesDisplayName(series[1], { + showField, + showWhere, + })}`, sortOrder: 'sortOrder' in series[0] ? series[0].sortOrder : undefined, }, @@ -99,7 +127,10 @@ export function seriesColumns({ : series.map((s, i) => { return { dataKey: `series_${i}.data`, - displayName: seriesDisplayName(s), + displayName: seriesDisplayName(s, { + showField, + showWhere, + }), sortOrder: 'sortOrder' in s ? s.sortOrder : undefined, }; }); diff --git a/packages/app/src/DashboardPage.tsx b/packages/app/src/DashboardPage.tsx index bb851c0cf..8cc3b034e 100644 --- a/packages/app/src/DashboardPage.tsx +++ b/packages/app/src/DashboardPage.tsx @@ -138,6 +138,14 @@ const Tile = forwardRef( granularity ?? convertDateRangeToGranularityString(dateRange, 60), dateRange, numberFormat: chart.series[0].numberFormat, + seriesReturnType: chart.seriesReturnType, + series: chart.series.map(s => ({ + ...s, + where: buildAndWhereClause( + query, + s.type === 'time' ? s.where : '', + ), + })), } : type === 'table' ? { @@ -152,6 +160,14 @@ const Tile = forwardRef( granularity ?? convertDateRangeToGranularityString(dateRange, 60), dateRange, numberFormat: chart.series[0].numberFormat, + series: chart.series.map(s => ({ + ...s, + where: buildAndWhereClause( + query, + s.type === 'table' ? s.where : '', + ), + })), + seriesReturnType: chart.seriesReturnType, } : type === 'histogram' ? { @@ -272,22 +288,10 @@ const Tile = forwardRef( onMouseDown={e => e.stopPropagation()} > {chart.series[0].type === 'time' && config.type === 'time' && ( - + )} {chart.series[0].type === 'table' && config.type === 'table' && ( - + )} {config.type === 'histogram' && ( diff --git a/packages/app/src/HDXLineChart.tsx b/packages/app/src/HDXLineChart.tsx index 2c37e8b50..7e0d2ef91 100644 --- a/packages/app/src/HDXLineChart.tsx +++ b/packages/app/src/HDXLineChart.tsx @@ -304,7 +304,7 @@ export const HDXLineChartTooltip = memo((props: any) => { .sort((a: any, b: any) => b.value - a.value) .map((p: any) => (
- {truncateMiddle(p.name ?? p.dataKey, 50)}:{' '} + {truncateMiddle(p.name ?? p.dataKey, 70)}:{' '} {numberFormat ? formatNumber(p.value, numberFormat) : p.value}
))} diff --git a/packages/app/styles/HDXLineChart.module.scss b/packages/app/styles/HDXLineChart.module.scss index 506a03ca4..14777a547 100644 --- a/packages/app/styles/HDXLineChart.module.scss +++ b/packages/app/styles/HDXLineChart.module.scss @@ -37,15 +37,15 @@ } .chartTooltip { - background-color: rgba(0, 0, 0, 0.5); - backdrop-filter: blur(1px); + background-color: rgba(0, 0, 0, 0.8); + backdrop-filter: blur(2px); border-radius: 4px; font-size: 11px; } .chartTooltipHeader { padding: 2px 12px; - color: $slate-400; + color: $slate-200; border-bottom: 1px solid $slate-700; }