[SLOs] Use named keys to reference range aggregations for APM Latency…

… and Histogram Metric (#173548) ## Summary Fixes #171329 it now works for value over 9999 Fix APM SLI Preview value calculation !! <img width="1726" alt="image" src="https://github.com/elastic/kibana/assets/3505601/d875e2cf-6195-455f-9b28-35c49c6d5eee">
elastic · Jan 5, 2024 · 57e50a6 · 57e50a6
1 parent 2759182
commit 57e50a6
Show file tree

Hide file tree

Showing 8 changed files with 82 additions and 59 deletions.
diff --git a/.../services/slo/aggregations/__snapshots__/get_histogram_indicator_aggregation.test.ts.snap b/.../services/slo/aggregations/__snapshots__/get_histogram_indicator_aggregation.test.ts.snap
diff --git a/...ins/observability/server/services/slo/aggregations/get_histogram_indicator_aggregation.ts b/...ins/observability/server/services/slo/aggregations/get_histogram_indicator_aggregation.ts
@@ -6,6 +6,7 @@
  */
 
 import { HistogramIndicator } from '@kbn/slo-schema';
+import { AggregationsAggregationContainer } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
 import { getElastichsearchQueryOrThrow } from '../transform_generators/common';
 
 type HistogramIndicatorDef =
@@ -15,7 +16,7 @@ type HistogramIndicatorDef =
 export class GetHistogramIndicatorAggregation {
   constructor(private indicator: HistogramIndicator) {}
 
-  private buildAggregation(type: 'good' | 'total', indicator: HistogramIndicatorDef) {
+  private buildAggregation(indicator: HistogramIndicatorDef): AggregationsAggregationContainer {
     const filter = indicator.filter
       ? getElastichsearchQueryOrThrow(indicator.filter)
       : { match_all: {} };
@@ -43,40 +44,30 @@ export class GetHistogramIndicatorAggregation {
       throw new Error('Invalid Range: "from" should be less that "to".');
     }
 
-    const range: { from?: number; to?: number } = {};
-    if (indicator.from != null) {
-      range.from = indicator.from;
-    }
-
-    if (indicator.to != null) {
-      range.to = indicator.to;
-    }
-
     return {
       filter,
       aggs: {
         total: {
           range: {
             field: indicator.field,
             keyed: true,
-            ranges: [range],
+            ranges: [
+              {
+                key: 'target',
+                from: indicator.from,
+                to: indicator.to,
+              },
+            ],
           },
         },
       },
     };
   }
 
-  private formatNumberAsFloatString(value: number) {
-    return value % 1 === 0 ? `${value}.0` : `${value}`;
-  }
-
-  private buildRangeKey(from: number | undefined, to: number | undefined) {
-    const fromString = from != null ? this.formatNumberAsFloatString(from) : '*';
-    const toString = to != null ? this.formatNumberAsFloatString(to) : '*';
-    return `${fromString}-${toString}`;
-  }
-
-  private buildBucketScript(type: 'good' | 'total', indicator: HistogramIndicatorDef) {
+  private buildBucketScript(
+    type: 'good' | 'total',
+    indicator: HistogramIndicatorDef
+  ): AggregationsAggregationContainer {
     if (indicator.aggregation === 'value_count') {
       return {
         bucket_script: {
@@ -87,11 +78,10 @@ export class GetHistogramIndicatorAggregation {
         },
       };
     }
-    const rangeKey = this.buildRangeKey(indicator.from, indicator.to);
     return {
       bucket_script: {
         buckets_path: {
-          value: `_${type}>total['${rangeKey}']>_count`,
+          value: `_${type}>total['target']>_count`,
         },
         script: 'params.value',
       },
@@ -101,7 +91,7 @@ export class GetHistogramIndicatorAggregation {
   public execute({ type, aggregationKey }: { type: 'good' | 'total'; aggregationKey: string }) {
     const indicatorDef = this.indicator.params[type];
     return {
-      [`_${type}`]: this.buildAggregation(type, indicatorDef),
+      [`_${type}`]: this.buildAggregation(indicatorDef),
       [aggregationKey]: this.buildBucketScript(type, indicatorDef),
     };
   }

diff --git a/x-pack/plugins/observability/server/services/slo/get_preview_data.ts b/x-pack/plugins/observability/server/services/slo/get_preview_data.ts
@@ -6,7 +6,6 @@
  */
 
 import { calculateAuto } from '@kbn/calculate-auto';
-import { ElasticsearchClient } from '@kbn/core-elasticsearch-server';
 import { fromKueryExpression, toElasticsearchQuery } from '@kbn/es-query';
 import {
   ALL_VALUE,
@@ -20,6 +19,9 @@ import {
 } from '@kbn/slo-schema';
 import { assertNever } from '@kbn/std';
 import moment from 'moment';
+import { ElasticsearchClient } from '@kbn/core/server';
+import { estypes } from '@elastic/elasticsearch';
+import { typedSearch } from '../../utils/queries';
 import { APMTransactionDurationIndicator } from '../../domain/models';
 import { computeSLI } from '../../domain/services';
 import { InvalidQueryError } from '../../errors';
@@ -43,7 +45,7 @@ export class GetPreviewData {
     indicator: APMTransactionDurationIndicator,
     options: Options
   ): Promise<GetPreviewDataResponse> {
-    const filter = [];
+    const filter: estypes.QueryDslQueryContainer[] = [];
     if (indicator.params.service !== ALL_VALUE)
       filter.push({
         match: { 'service.name': indicator.params.service },
@@ -61,11 +63,11 @@ export class GetPreviewData {
         match: { 'transaction.type': indicator.params.transactionType },
       });
     if (!!indicator.params.filter)
-      filter.push(getElastichsearchQueryOrThrow(indicator.params.filter));
+      filter.push(getElasticsearchQueryOrThrow(indicator.params.filter));
 
     const truncatedThreshold = Math.trunc(indicator.params.threshold * 1000);
 
-    const result = await this.esClient.search({
+    const result = await typedSearch(this.esClient, {
       index: indicator.params.index,
       size: 0,
       query: {
@@ -89,13 +91,14 @@ export class GetPreviewData {
             _good: {
               range: {
                 field: 'transaction.duration.histogram',
-                ranges: [{ to: truncatedThreshold }],
+                keyed: true,
+                ranges: [{ to: truncatedThreshold, key: 'target' }],
               },
             },
             good: {
               bucket_script: {
                 buckets_path: {
-                  _good: `_good['*-${truncatedThreshold}.0']>_count`,
+                  _good: `_good['target']>_count`,
                 },
                 script: 'params._good',
               },
@@ -110,17 +113,21 @@ export class GetPreviewData {
       },
     });
 
-    // @ts-ignore buckets is not improperly typed
-    return result.aggregations?.perMinute.buckets.map((bucket) => ({
-      date: bucket.key_as_string,
-      sliValue:
-        !!bucket.good && !!bucket.total ? computeSLI(bucket.good.value, bucket.total.value) : null,
-      events: {
-        good: bucket.good?.value ?? 0,
-        bad: (bucket.total?.value ?? 0) - (bucket.good?.value ?? 0),
-        total: bucket.total?.value ?? 0,
-      },
-    }));
+    return (
+      result.aggregations?.perMinute.buckets.map((bucket) => {
+        const good = (bucket.good?.value as number) ?? 0;
+        const total = bucket.total?.value ?? 0;
+        return {
+          date: bucket.key_as_string,
+          sliValue: computeSLI(good, total),
+          events: {
+            good,
+            total,
+            bad: total - good,
+          },
+        };
+      }) ?? []
+    );
   }
 
   private async getAPMTransactionErrorPreviewData(
@@ -145,7 +152,7 @@ export class GetPreviewData {
         match: { 'transaction.type': indicator.params.transactionType },
       });
     if (!!indicator.params.filter)
-      filter.push(getElastichsearchQueryOrThrow(indicator.params.filter));
+      filter.push(getElasticsearchQueryOrThrow(indicator.params.filter));
 
     const result = await this.esClient.search({
       index: indicator.params.index,
@@ -208,7 +215,7 @@ export class GetPreviewData {
     options: Options
   ): Promise<GetPreviewDataResponse> {
     const getHistogramIndicatorAggregations = new GetHistogramIndicatorAggregation(indicator);
-    const filterQuery = getElastichsearchQueryOrThrow(indicator.params.filter);
+    const filterQuery = getElasticsearchQueryOrThrow(indicator.params.filter);
     const timestampField = indicator.params.timestampField;
     const result = await this.esClient.search({
       index: indicator.params.index,
@@ -259,7 +266,7 @@ export class GetPreviewData {
     options: Options
   ): Promise<GetPreviewDataResponse> {
     const timestampField = indicator.params.timestampField;
-    const filterQuery = getElastichsearchQueryOrThrow(indicator.params.filter);
+    const filterQuery = getElasticsearchQueryOrThrow(indicator.params.filter);
     const getCustomMetricIndicatorAggregation = new GetCustomMetricIndicatorAggregation(indicator);
     const result = await this.esClient.search({
       index: indicator.params.index,
@@ -310,7 +317,7 @@ export class GetPreviewData {
     options: Options
   ): Promise<GetPreviewDataResponse> {
     const timestampField = indicator.params.timestampField;
-    const filterQuery = getElastichsearchQueryOrThrow(indicator.params.filter);
+    const filterQuery = getElasticsearchQueryOrThrow(indicator.params.filter);
     const getCustomMetricIndicatorAggregation = new GetTimesliceMetricIndicatorAggregation(
       indicator
     );
@@ -349,9 +356,9 @@ export class GetPreviewData {
     indicator: KQLCustomIndicator,
     options: Options
   ): Promise<GetPreviewDataResponse> {
-    const filterQuery = getElastichsearchQueryOrThrow(indicator.params.filter);
-    const goodQuery = getElastichsearchQueryOrThrow(indicator.params.good);
-    const totalQuery = getElastichsearchQueryOrThrow(indicator.params.total);
+    const filterQuery = getElasticsearchQueryOrThrow(indicator.params.filter);
+    const goodQuery = getElasticsearchQueryOrThrow(indicator.params.good);
+    const totalQuery = getElasticsearchQueryOrThrow(indicator.params.total);
     const timestampField = indicator.params.timestampField;
     const result = await this.esClient.search({
       index: indicator.params.index,
@@ -429,7 +436,7 @@ export class GetPreviewData {
   }
 }
 
-function getElastichsearchQueryOrThrow(kuery: string | undefined = '') {
+function getElasticsearchQueryOrThrow(kuery: string | undefined = '') {
   try {
     return toElasticsearchQuery(fromKueryExpression(kuery));
   } catch (err) {

diff --git a/...ver/services/slo/transform_generators/__snapshots__/apm_transaction_duration.test.ts.snap b/...ver/services/slo/transform_generators/__snapshots__/apm_transaction_duration.test.ts.snap
diff --git a/...servability/server/services/slo/transform_generators/__snapshots__/histogram.test.ts.snap b/...servability/server/services/slo/transform_generators/__snapshots__/histogram.test.ts.snap
diff --git a/...lugins/observability/server/services/slo/transform_generators/apm_transaction_duration.ts b/...lugins/observability/server/services/slo/transform_generators/apm_transaction_duration.ts
@@ -11,6 +11,7 @@ import {
   apmTransactionDurationIndicatorSchema,
   timeslicesBudgetingMethodSchema,
 } from '@kbn/slo-schema';
+import { AggregationsAggregationContainer } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
 import { getElastichsearchQueryOrThrow, TransformGenerator } from '.';
 import {
   getSLOTransformId,
@@ -137,25 +138,30 @@ export class ApmTransactionDurationTransformGenerator extends TransformGenerator
     };
   }
 
-  private buildAggregations(slo: SLO, indicator: APMTransactionDurationIndicator) {
+  private buildAggregations(
+    slo: SLO,
+    indicator: APMTransactionDurationIndicator
+  ): Record<string, AggregationsAggregationContainer> {
     // threshold is in ms (milliseconds), but apm data is stored in us (microseconds)
     const truncatedThreshold = Math.trunc(indicator.params.threshold * 1000);
 
     return {
       _numerator: {
         range: {
           field: 'transaction.duration.histogram',
+          keyed: true,
           ranges: [
             {
               to: truncatedThreshold,
+              key: 'target',
             },
           ],
         },
       },
       'slo.numerator': {
         bucket_script: {
           buckets_path: {
-            numerator: `_numerator['*-${truncatedThreshold}.0']>_count`,
+            numerator: `_numerator['target']>_count`,
           },
           script: 'params.numerator',
         },