Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
phillipb merged 8 commits into from
Jul 28, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fb27ec8
Don't skip last bucket for most aggs
phillipb Jul 27, 2021
d646204
Allow alerting on partial buckets for certain aggs
phillipb Jul 28, 2021
5616385
Fix test, PR feedback, and some comments
phillipb Jul 28, 2021
6087cfe
Remove all offset logic for date_range aggs
phillipb Jul 28, 2021
fa48ef8
Remove code comment
phillipb Jul 28, 2021
bbee8be
Add delivery delay
phillipb Jul 28, 2021
a47ba28
Fix the date range for query
phillipb Jul 28, 2021
59524a8
Add TODO
phillipb Jul 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 24 additions & 13 deletions x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/evaluate_alert.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import { mapValues, first, last, isNaN } from 'lodash';
import moment from 'moment';
import { ElasticsearchClient } from 'kibana/server';
import {
isTooManyBucketsPreviewException,
Expand Down Expand Up @@ -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())
Comment thread
phillipb marked this conversation as resolved.
.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;
Comment thread
phillipb marked this conversation as resolved.
Expand Down Expand Up @@ -223,13 +227,12 @@ 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
Expand All @@ -241,12 +244,20 @@ const getValuesFromAggregations = (
})
.filter(dropPartialBuckets(dropPartialBucketsOptions));
}
return buckets
.map((bucket) => ({
key: bucket.key_as_string ?? bucket.from_as_string,
value: bucket.aggregatedValue?.value ?? null,
}))
.filter(dropPartialBuckets(dropPartialBucketsOptions));

if (aggType === Aggregators.AVERAGE) {
buckets
Comment thread
phillipb marked this conversation as resolved.
Outdated
.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
}
Expand Down
5 changes: 4 additions & 1 deletion x-pack/plugins/infra/server/lib/alerting/metric_threshold/lib/metric_query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ export const getElasticsearchMetricQuery = (
const to = timeframe.end;
const from = timeframe.start;
const offset = calculateDateHistogramOffset({ from, to, interval, field: timefield });
const offsetInMS = parseInt(offset, 10);
const offsetInMS =
Comment thread
phillipb marked this conversation as resolved.
Outdated
[Aggregators.P95, Aggregators.P99, Aggregators.AVERAGE].indexOf(aggType) > -1
? parseInt(offset, 10)
: 0;

const aggregations =
aggType === Aggregators.COUNT
Expand Down