[Metrics UI] Fix p95/p99 charts and alerting error (elastic#65579)

* [Metrics UI] Fix p95/p99 charts and alerting error - Fixes elastic#65561 * Fixing open in visualize for percentiles * Adding test for P95; refactoring to use first consitently
simianhacker · May 8, 2020 · 1099f21 · 1099f21
1 parent 7966756
commit 1099f21
Show file tree

Hide file tree

Showing 10 changed files with 144 additions and 18 deletions.
diff --git a/x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_chart.tsx b/x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_chart.tsx
@@ -118,10 +118,7 @@ export const ExpressionChart: React.FC<Props> = ({
   const series = {
     ...firstSeries,
     rows: firstSeries.rows.map(row => {
-      const newRow: MetricsExplorerRow = {
-        timestamp: row.timestamp,
-        metric_0: row.metric_0 || null,
-      };
+      const newRow: MetricsExplorerRow = { ...row };
       thresholds.forEach((thresholdValue, index) => {
         newRow[`metric_threshold_${index}`] = thresholdValue;
       });

diff --git a/x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_row.tsx b/x-pack/plugins/infra/public/alerting/metric_threshold/components/expression_row.tsx
@@ -234,4 +234,20 @@ export const aggregationType: { [key: string]: any } = {
     value: AGGREGATION_TYPES.SUM,
     validNormalizedTypes: ['number'],
   },
+  p95: {
+    text: i18n.translate('xpack.infra.metrics.alertFlyout.aggregationText.p95', {
+      defaultMessage: '95th Percentile',
+    }),
+    fieldRequired: false,
+    value: AGGREGATION_TYPES.P95,
+    validNormalizedTypes: ['number'],
+  },
+  p99: {
+    text: i18n.translate('xpack.infra.metrics.alertFlyout.aggregationText.p99', {
+      defaultMessage: '99th Percentile',
+    }),
+    fieldRequired: false,
+    value: AGGREGATION_TYPES.P99,
+    validNormalizedTypes: ['number'],
+  },
 };
diff --git a/x-pack/plugins/infra/public/alerting/metric_threshold/types.ts b/x-pack/plugins/infra/public/alerting/metric_threshold/types.ts
@@ -29,6 +29,8 @@ export enum AGGREGATION_TYPES {
   MAX = 'max',
   RATE = 'rate',
   CARDINALITY = 'cardinality',
+  P95 = 'p95',
+  P99 = 'p99',
 }
 
 export interface MetricThresholdAlertParams {

diff --git a/...lugins/infra/public/pages/metrics/metrics_explorer/components/helpers/create_tsvb_link.ts b/...lugins/infra/public/pages/metrics/metrics_explorer/components/helpers/create_tsvb_link.ts
@@ -46,6 +46,24 @@ export const metricsExplorerMetricToTSVBMetric = (metric: MetricsExplorerOptions
         field: derivativeId,
       },
     ];
+  } else if (metric.aggregation === 'p95' || metric.aggregation === 'p99') {
+    const percentileValue = metric.aggregation === 'p95' ? '95' : '99';
+    return [
+      {
+        id: uuid.v1(),
+        type: 'percentile',
+        field: metric.field,
+        percentiles: [
+          {
+            id: uuid.v1(),
+            value: percentileValue,
+            mode: 'line',
+            percentile: '',
+            shade: 0.2,
+          },
+        ],
+      },
+    ];
   } else {
     return [
       {

diff --git a/x-pack/plugins/infra/public/pages/metrics/metrics_explorer/components/series_chart.tsx b/x-pack/plugins/infra/public/pages/metrics/metrics_explorer/components/series_chart.tsx
@@ -20,6 +20,7 @@ import {
   MetricsExplorerOptionsMetric,
   MetricsExplorerChartType,
 } from '../hooks/use_metrics_explorer_options';
+import { getMetricId } from './helpers/get_metric_id';
 
 type NumberOrString = string | number;
 
@@ -45,10 +46,12 @@ export const MetricsExplorerAreaChart = ({ metric, id, series, type, stack, opac
     colorTransformer(MetricsExplorerColor.color0);
 
   const yAccessors = Array.isArray(id)
-    ? id.map(i => `metric_${i}`).slice(id.length - 1, id.length)
-    : [`metric_${id}`];
+    ? id.map(i => getMetricId(metric, i)).slice(id.length - 1, id.length)
+    : [getMetricId(metric, id)];
   const y0Accessors =
-    Array.isArray(id) && id.length > 1 ? id.map(i => `metric_${i}`).slice(0, 1) : undefined;
+    Array.isArray(id) && id.length > 1
+      ? id.map(i => getMetricId(metric, i)).slice(0, 1)
+      : undefined;
   const chartId = `series-${series.id}-${yAccessors.join('-')}`;
 
   const seriesAreaStyle: RecursivePartial<AreaSeriesStyle> = {
@@ -85,8 +88,10 @@ export const MetricsExplorerBarChart = ({ metric, id, series, stack }: Props) =>
     (metric.color && colorTransformer(metric.color)) ||
     colorTransformer(MetricsExplorerColor.color0);
 
-  const yAccessor = `metric_${id}`;
-  const chartId = `series-${series.id}-${yAccessor}`;
+  const yAccessors = Array.isArray(id)
+    ? id.map(i => getMetricId(metric, i)).slice(id.length - 1, id.length)
+    : [getMetricId(metric, id)];
+  const chartId = `series-${series.id}-${yAccessors.join('-')}`;
 
   const seriesBarStyle: RecursivePartial<BarSeriesStyle> = {
     rectBorder: {
@@ -100,13 +105,13 @@ export const MetricsExplorerBarChart = ({ metric, id, series, stack }: Props) =>
   };
   return (
     <BarSeries
-      id={yAccessor}
+      id={chartId}
       key={chartId}
       name={createMetricLabel(metric)}
       xScaleType={ScaleType.Time}
       yScaleType={ScaleType.Linear}
       xAccessor="timestamp"
-      yAccessors={[yAccessor]}
+      yAccessors={yAccessors}
       data={series.rows}
       stackAccessors={stack ? ['timestamp'] : void 0}
       barSeriesStyle={seriesBarStyle}

diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/create_percentile_aggregation.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/create_percentile_aggregation.ts
@@ -0,0 +1,22 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import { Aggregators } from './types';
+export const createPercentileAggregation = (
+  type: Aggregators.P95 | Aggregators.P99,
+  field: string
+) => {
+  const value = type === Aggregators.P95 ? 95 : 99;
+  return {
+    aggregatedValue: {
+      percentiles: {
+        field,
+        percents: [value],
+        keyed: false,
+      },
+    },
+  };
+};
diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts
@@ -233,6 +233,58 @@ describe('The metric threshold alert type', () => {
       expect(getState(instanceID).alertState).toBe(AlertStates.OK);
     });
   });
+  describe('querying with the p99 aggregator', () => {
+    const instanceID = 'test-*';
+    const execute = (comparator: Comparator, threshold: number[]) =>
+      executor({
+        services,
+        params: {
+          criteria: [
+            {
+              ...baseCriterion,
+              comparator,
+              threshold,
+              aggType: 'p99',
+              metric: 'test.metric.2',
+            },
+          ],
+        },
+      });
+    test('alerts based on the p99 values', async () => {
+      await execute(Comparator.GT, [1]);
+      expect(mostRecentAction(instanceID).id).toBe(FIRED_ACTIONS.id);
+      expect(getState(instanceID).alertState).toBe(AlertStates.ALERT);
+      await execute(Comparator.LT, [1]);
+      expect(mostRecentAction(instanceID)).toBe(undefined);
+      expect(getState(instanceID).alertState).toBe(AlertStates.OK);
+    });
+  });
+  describe('querying with the p95 aggregator', () => {
+    const instanceID = 'test-*';
+    const execute = (comparator: Comparator, threshold: number[]) =>
+      executor({
+        services,
+        params: {
+          criteria: [
+            {
+              ...baseCriterion,
+              comparator,
+              threshold,
+              aggType: 'p95',
+              metric: 'test.metric.1',
+            },
+          ],
+        },
+      });
+    test('alerts based on the p95 values', async () => {
+      await execute(Comparator.GT, [0.25]);
+      expect(mostRecentAction(instanceID).id).toBe(FIRED_ACTIONS.id);
+      expect(getState(instanceID).alertState).toBe(AlertStates.ALERT);
+      await execute(Comparator.LT, [0.95]);
+      expect(mostRecentAction(instanceID)).toBe(undefined);
+      expect(getState(instanceID).alertState).toBe(AlertStates.OK);
+    });
+  });
   describe("querying a metric that hasn't reported data", () => {
     const instanceID = 'test-*';
     const execute = (alertOnNoData: boolean) =>

diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.ts
@@ -3,7 +3,7 @@
  * or more contributor license agreements. Licensed under the Elastic License;
  * you may not use this file except in compliance with the Elastic License.
  */
-import { mapValues } from 'lodash';
+import { mapValues, first } from 'lodash';
 import { i18n } from '@kbn/i18n';
 import { InfraDatabaseSearchResponse } from '../../adapters/framework/adapter_types';
 import { createAfterKeyHandler } from '../../../utils/create_afterkey_handler';
@@ -21,12 +21,16 @@ import { AlertServices, AlertExecutorOptions } from '../../../../../alerting/ser
 import { getIntervalInSeconds } from '../../../utils/get_interval_in_seconds';
 import { getDateHistogramOffset } from '../../snapshot/query_helpers';
 import { InfraBackendLibs } from '../../infra_types';
+import { createPercentileAggregation } from './create_percentile_aggregation';
 
 const TOTAL_BUCKETS = 5;
 
 interface Aggregation {
   aggregatedIntervals: {
-    buckets: Array<{ aggregatedValue: { value: number }; doc_count: number }>;
+    buckets: Array<{
+      aggregatedValue: { value: number; values?: Array<{ key: number; value: number }> };
+      doc_count: number;
+    }>;
   };
 }
 
@@ -47,6 +51,12 @@ const getCurrentValueFromAggregations = (
     if (aggType === Aggregators.COUNT) {
       return mostRecentBucket.doc_count;
     }
+    if (aggType === Aggregators.P95 || aggType === Aggregators.P99) {
+      const values = mostRecentBucket.aggregatedValue?.values || [];
+      const firstValue = first(values);
+      if (!firstValue) return null;
+      return firstValue.value;
+    }
     const { value } = mostRecentBucket.aggregatedValue;
     return value;
   } catch (e) {
@@ -86,6 +96,8 @@ export const getElasticsearchMetricQuery = (
       ? {}
       : aggType === Aggregators.RATE
       ? networkTraffic('aggregatedValue', metric)
+      : aggType === Aggregators.P95 || aggType === Aggregators.P99
+      ? createPercentileAggregation(aggType, metric)
       : {
           aggregatedValue: {
             [aggType]: {
@@ -275,7 +287,7 @@ export const createMetricThresholdExecutor = (libs: InfraBackendLibs, alertId: s
     );
 
     // Because each alert result has the same group definitions, just grap the groups from the first one.
-    const groups = Object.keys(alertResults[0]);
+    const groups = Object.keys(first(alertResults));
     for (const group of groups) {
       const alertInstance = services.alertInstanceFactory(`${alertId}-${group}`);
 

diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/test_mocks.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/test_mocks.ts
@@ -7,22 +7,22 @@
 const bucketsA = [
   {
     doc_count: 2,
-    aggregatedValue: { value: 0.5 },
+    aggregatedValue: { value: 0.5, values: [{ key: 95.0, value: 0.5 }] },
   },
   {
     doc_count: 3,
-    aggregatedValue: { value: 1.0 },
+    aggregatedValue: { value: 1.0, values: [{ key: 95.0, value: 1.0 }] },
   },
 ];
 
 const bucketsB = [
   {
     doc_count: 4,
-    aggregatedValue: { value: 2.5 },
+    aggregatedValue: { value: 2.5, values: [{ key: 99.0, value: 2.5 }] },
   },
   {
     doc_count: 5,
-    aggregatedValue: { value: 3.5 },
+    aggregatedValue: { value: 3.5, values: [{ key: 99.0, value: 3.5 }] },
   },
 ];
 

diff --git a/x-pack/plugins/infra/server/lib/alerting/metric_threshold/types.ts b/x-pack/plugins/infra/server/lib/alerting/metric_threshold/types.ts
@@ -23,6 +23,8 @@ export enum Aggregators {
   MAX = 'max',
   RATE = 'rate',
   CARDINALITY = 'cardinality',
+  P95 = 'p95',
+  P99 = 'p99',
 }
 
 export enum AlertStates {