[Metrics UI] Correct inaccurate offsetting for non-rate aggregations inside of metrics threshold alerts

x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/evaluate_alert.ts

@@ -6,6 +6,7 @@
  */
 
 import { mapValues, first, last, isNaN } from 'lodash';
+import moment from 'moment';
 import { ElasticsearchClient } from 'kibana/server';
 import {
   isTooManyBucketsPreviewException,
@@ -121,7 +122,10 @@ const getMetric: (
   const intervalAsSeconds = getIntervalInSeconds(interval);
   const intervalAsMS = intervalAsSeconds * 1000;
 
-  const to = roundTimestamp(timeframe ? timeframe.end : Date.now(), timeUnit);
+  const to = moment(timeframe ? timeframe.end : Date.now())
+    .add(1, timeUnit)
+    .startOf(timeUnit)
+    .valueOf();
   // We need enough data for 5 buckets worth of data. We also need
   // to convert the intervalAsSeconds to milliseconds.
   const minimumFrom = to - intervalAsMS * MINIMUM_BUCKETS;
@@ -223,30 +227,44 @@ const getValuesFromAggregations = (
   try {
     const { buckets } = aggregations.aggregatedIntervals;
     if (!buckets.length) return null; // No Data state
+
     if (aggType === Aggregators.COUNT) {
-      return buckets
-        .map((bucket) => ({
-          key: bucket.from_as_string,
-          value: bucket.doc_count,
-        }))
-        .filter(dropPartialBuckets(dropPartialBucketsOptions));
+      return buckets.map((bucket) => ({
+        key: bucket.from_as_string,
+        value: bucket.doc_count,
+      }));
     }
     if (aggType === Aggregators.P95 || aggType === Aggregators.P99) {
-      return buckets
-        .map((bucket) => {
-          const values = bucket.aggregatedValue?.values || [];
-          const firstValue = first(values);
-          if (!firstValue) return null;
-          return { key: bucket.from_as_string, value: firstValue.value };
-        })
-        .filter(dropPartialBuckets(dropPartialBucketsOptions));
+      return buckets.map((bucket) => {
+        const values = bucket.aggregatedValue?.values || [];
+        const firstValue = first(values);
+        if (!firstValue) return null;
+        return { key: bucket.from_as_string, value: firstValue.value };
+      });
     }
-    return buckets
-      .map((bucket) => ({
+
+    if (aggType === Aggregators.AVERAGE) {
+      return buckets.map((bucket) => ({
         key: bucket.key_as_string ?? bucket.from_as_string,
         value: bucket.aggregatedValue?.value ?? null,
-      }))
-      .filter(dropPartialBuckets(dropPartialBucketsOptions));
+      }));
+      // AVG can only be calculated accurately on full buckets
+      // .filter(dropPartialBuckets(dropPartialBucketsOptions))
+    }
+
+    if (aggType === Aggregators.RATE) {
+      return buckets
+        .map((bucket) => ({
+          key: bucket.key_as_string ?? bucket.from_as_string,
+          value: bucket.aggregatedValue?.value ?? null,
+        }))
+        .filter(dropPartialBuckets(dropPartialBucketsOptions));
+    }
+
+    return buckets.map((bucket) => ({
+      key: bucket.key_as_string ?? bucket.from_as_string,
+      value: bucket.aggregatedValue?.value ?? null,
+    }));
   } catch (e) {
     return NaN; // Error state
   }

x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/metric_query.ts

@@ -38,8 +38,6 @@ export const getElasticsearchMetricQuery = (
   const intervalAsMS = intervalAsSeconds * 1000;
   const to = timeframe.end;
   const from = timeframe.start;
-  const offset = calculateDateHistogramOffset({ from, to, interval, field: timefield });
-  const offsetInMS = parseInt(offset, 10);
 
   const aggregations =
     aggType === Aggregators.COUNT
@@ -63,7 +61,7 @@ export const getElasticsearchMetricQuery = (
             date_histogram: {
               field: timefield,
               fixed_interval: interval,
-              offset,
+              offset: calculateDateHistogramOffset({ from, to, interval, field: timefield }),
               extended_bounds: {
                 min: from,
                 max: to,
@@ -77,8 +75,8 @@ export const getElasticsearchMetricQuery = (
             date_range: {
               field: timefield,
               ranges: Array.from(Array(Math.floor((to - from) / intervalAsMS)), (_, i) => ({
-                from: from + intervalAsMS * i + offsetInMS,
-                to: from + intervalAsMS * (i + 1) + offsetInMS,
+                from: from + intervalAsMS * i,
+                to: from + intervalAsMS * (i + 1),
               })),
             },
             aggregations,

x-pack/plugins/infra/server/lib/alerting/metric_threshold/metric_threshold_executor.test.ts

@@ -202,7 +202,7 @@ describe('The metric threshold alert type', () => {
     });
     test('sends no alert when some, but not all, criteria cross the threshold', async () => {
       const instanceID = '*';
-      await execute(Comparator.LT_OR_EQ, [1.0], [3.0]);
+      await execute(Comparator.LT_OR_EQ, [1.0], [2.5]);
       expect(mostRecentAction(instanceID)).toBe(undefined);
     });
     test('alerts only on groups that meet all criteria when querying with a groupBy parameter', async () => {
@@ -221,7 +221,7 @@ describe('The metric threshold alert type', () => {
       expect(reasons[0]).toContain('test.metric.1');
       expect(reasons[1]).toContain('test.metric.2');
       expect(reasons[0]).toContain('current value is 1');
-      expect(reasons[1]).toContain('current value is 3.5');
+      expect(reasons[1]).toContain('current value is 3');
       expect(reasons[0]).toContain('threshold of 1');
       expect(reasons[1]).toContain('threshold of 3');
     });
@@ -245,9 +245,9 @@ describe('The metric threshold alert type', () => {
         },
       });
     test('alerts based on the doc_count value instead of the aggregatedValue', async () => {
-      await execute(Comparator.GT, [2]);
+      await execute(Comparator.GT, [0.9]);
       expect(mostRecentAction(instanceID).id).toBe(FIRED_ACTIONS.id);
-      await execute(Comparator.LT, [1.5]);
+      await execute(Comparator.LT, [0.5]);
       expect(mostRecentAction(instanceID)).toBe(undefined);
     });
   });