Skip to content
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

[Logs UI] Add timestamp as a context variable to log threshold alerts #78932

Merged
merged 4 commits into from
Oct 6, 2020
Merged

[Logs UI] Add timestamp as a context variable to log threshold alerts #78932

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1cdb44c
Add timestamp context variable
Kerry350 Sep 30, 2020
837622b
Merge branch 'master' into 71751-log-threshold-timestamp-context-vari…
elasticmachine Sep 30, 2020
9c64f21
Merge branch 'master' into 71751-log-threshold-timestamp-context-vari…
kibanamachine Oct 5, 2020
9bc45b4
Capture timestamp outside of loop
Kerry350 Oct 5, 2020
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
5 changes: 4 additions & 1 deletion x-pack/plugins/infra/server/lib/alerting/log_threshold/log_threshold_executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,10 @@ export const updateAlertInstance: AlertInstanceUpdater = (alertInstance, state,
if (actions && actions.length > 0) {
actions.forEach((actionSet) => {
const { actionGroup, context } = actionSet;
alertInstance.scheduleActions(actionGroup, context);
const sharedContext = {
timestamp: new Date().toISOString(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering whether we need to take special care of the time zone here, but since this runs on the server side it's probably reasonable to just stick to UTC. 👍 (Especially since the label makes it explicit.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#71751 suggests that the goal is for the timestamp to be useful for linking to dashboards and other services for the purposes of investigating the cause of the alert.

Not being particularly familiar with the alert execution logic, I wonder if generating the timestamp at this point in this way fulfills that goal. Could the timestamp differ from the ones used in the query to such a degree that it becomes misleading?

Would it be very difficult to populate the context with the start and end timestamps used in the range filter when executing the query?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering whether we need to take special care of the time zone here, but since this runs on the server side it's probably reasonable to just stick to UTC. 👍 (Especially since the label makes it explicit.)

Yeah, I'd considered whether this should use Kibana's settings for timezone / formatting. However, UTC has been requested before, so I believe it's useful as a standalone context variable.

Metrics are using the key of the aggregation result, and when format isn't specified explicitly (which I don't think they do) the docs note that If you don’t specify format, the first date format specified in the field mapping is used., so if we wanted to add a more specific timestamp, with formatting, I guess it would have to be based off our field mapping for whatever is set as the timestamp field.

Not being particularly familiar with the alert execution logic, I wonder if generating the timestamp at this point in this way fulfills that goal. Could the timestamp differ from the ones used in the query to such a degree that it becomes misleading?

I'd interpreted the ticket description differently at implementation, specifically:

When alert notifications are forwarded to a queue, they might go through a few steps before getting parsed and pushed forward to event management systems. This introduces delays, which today makes it hard to understand when an alert was triggered

My understanding was we want the timestamp of when the alert was actually triggered, due to queues that might be hit along the way. The timestamp here is as close as we can get in our executor to when the alert was triggered, as it's when we schedule the action (I guess if there's a delay on the framework side actually scheduling things it's slightly off, but we can't control that 🤔)

I think the problem is, in the description, understand when an alert was triggered is at odds with Add {{context.timestamp}} parameter to threshold alerts which can be used in notification to link to dashboards with appropriate time range or just as a variable to provide context to on call operator about the time window of the alert.

One needs the trigger time (which is here), and one needs the actual range, as you suggest, which isn't here. And both are useful on their own.

Would it be very difficult to populate the context with the start and end timestamps used in the range filter when executing the query?

No, this is easy to do. It's just a case of "once it's in it's hard to take away".

Should these similarly be in UTC?

I wonder if @mukeshelastic has any thoughts here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I guess there's value in having this timestamp as well as the data range the query looks at. But we could add that in a later PR once we have thought it over a bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea for the current implementation: How about capturing the timestamp outside of the loop to make sure all actions receive the same value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I guess there's value in having this timestamp as well as the data range the query looks at. But we could add that in a later PR once we have thought it over a bit.

👍 I will file a followup ticket once this is merged.

One idea for the current implementation: How about capturing the timestamp outside of the loop to make sure all actions receive the same value?

Yeah, good point. I'll change this.

};
alertInstance.scheduleActions(actionGroup, { ...sharedContext, ...context });
});
}

Expand Down
8 changes: 8 additions & 0 deletions x-pack/plugins/infra/server/lib/alerting/log_threshold/register_log_threshold_alert_type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@ import {
import { InfraBackendLibs } from '../../infra_types';
import { decodeOrThrow } from '../../../../common/runtime_types';

const timestampActionVariableDescription = i18n.translate(
'xpack.infra.logs.alerting.threshold.timestampActionVariableDescription',
{
defaultMessage: 'UTC timestamp of when the alert was triggered',
}
);

const documentCountActionVariableDescription = i18n.translate(
'xpack.infra.logs.alerting.threshold.documentCountActionVariableDescription',
{
Expand Down Expand Up @@ -85,6 +92,7 @@ export async function registerLogThresholdAlertType(
executor: createLogThresholdExecutor(libs),
actionVariables: {
context: [
{ name: 'timestamp', description: timestampActionVariableDescription },
{ name: 'matchingDocuments', description: documentCountActionVariableDescription },
{ name: 'conditions', description: conditionsActionVariableDescription },
{ name: 'group', description: groupByActionVariableDescription },
Expand Down