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.
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
[ES|QL] Enable ESQL alerts from the Discover app #165973
Conversation
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
👏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition for ES|QL!
src/plugins/discover/public/application/main/components/top_nav/open_alerts_popover.tsx
Show resolved
Hide resolved
src/plugins/discover/public/application/main/components/top_nav/open_alerts_popover.tsx
Show resolved
Hide resolved
@@ -54,6 +60,13 @@ export function AlertsPopover({ | |||
* Provides the default parameters used to initialize the new rule | |||
*/ | |||
const getParams = useCallback(() => { | |||
if (isPlainRecord) { | |||
return { | |||
searchType: 'esqlQuery', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not owned by us. If you have any recommendations for the alerting team it is better to create an issue for them cc @mikecote
@@ -188,6 +210,7 @@ export function openAlertsPopover({ | |||
stateContainer={stateContainer} | |||
adHocDataViews={adHocDataViews} | |||
services={services} | |||
isPlainRecord={isPlainRecord} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not related to us cc @mikecote maybe it makes sense to have a look on this when this PR is being merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have it configured so rules created from discover navigate to the following path whenever a user clicks "View in app": /app/discover#/viewAlert/${alert.id}
.
Is there something that should change with the path for ES|QL rules? The code comes from here: https://github.com/elastic/kibana/blob/main/x-pack/plugins/stack_alerts/public/rule_types/es_query/index.ts#L51C15-L51C51.
cc @XavierM as he is working on changing this in the recent release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can take a look to see if we can make it work from Discover side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx Julia! Code looks good, but I wonder if it is used when generating the link to Discover? I've created a rule with a notification containing a {{context.link}}
field:
But what was ingested was the link to the alert rule:
@stratoula we intend to link to Discover also in ES|QL
cases right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already exists in main unfortunately so better been handled on a separate PR. I synced with Matthias and we are going to open an issue on alerting.
@jughosta thanx for the review. From all your comments only one was related to this PR and was a limitation from the Alerts ui. I changed it to get the timestamp info also on initialization when there is an existing query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes related to enabling ES|QL alerts from Discover LGTM 👍
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @stratoula |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseOps changes LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ES|QL
in alerting 🥳 ... who could imagine this would be possible so quickly!
One thing I noticed when testing the preview, it also works with timestamp selection! Great! The preview table seems to ignore the custom timestamp, having selected the customer birth date as a timestamp, there seem to be lots of customers born in the last 5 minutes. While in theory this is not impossible, I think it's because in the request for the preview there's no time filter involved
@kertal this is correct. In ESQL the users can write a where clause with a timefield on their own if there is no @timestamp detected. The same is happening in Discover too. Possibly the alerting team should disable the timefilter selection in these cases cc @doakalexi but it is irrelevant with this PR I think. |
Summary
Enables the Alerts menu in Discover nav for the ES|QL mode and defaults to ESQL alerts by carrying the query that the user has typed.
Checklist