[Logs UI] Add timestamp as a context variable to log threshold alerts #78932
Conversation
Kerry350
added
v8.0.0
Feature:Logs UI
|
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
| @@ -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(), | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
#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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
One idea for the current implementation: How about capturing the timestamp outside of the loop to make sure all actions receive the same value?
There was a problem hiding this comment.
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.
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
…elastic#78932) * Add timestamp context variable
Summary
Closes #71751, and adds
timestampas a context variable to log threshold alerts.