[Metrics UI] Fix p95/p99 charts and alerting error

- Fixes elastic#65561
simianhacker · May 6, 2020 · a847cc5 · a847cc5
1 parent eef220e
commit a847cc5
Show file tree

Hide file tree

Showing 9 changed files with 97 additions and 15 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/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,32 @@ 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 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]: {

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
@@ -18,11 +18,11 @@ const bucketsA = [
 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 {