Bug fix: Restore dashboard filters, fetch log property type mappings …

…correctly for charts (#182)
hyperdxio · Jan 4, 2024 · 2910461 · 2910461
1 parent 3c29bcf
commit 2910461
Show file tree

Hide file tree

Showing 9 changed files with 120 additions and 113 deletions.
diff --git a/.changeset/famous-walls-brake.md 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
@@ -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
@@ -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<string>();
+    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
@@ -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<string>();
-      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
@@ -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(),