-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Logs UI] Index reason in log threshold executor #106291
Conversation
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
To update your PR or re-run it, just comment with: cc @weltenwort |
const alertInstance = alertInstanceFactory( | ||
group.name, | ||
getReasonMessageForGroupedCountAlert( | ||
documentCount, |
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.
Just a simple comment nothing really wrong about that. getReasonMessageForGroupedCountAlert
accepts as first parameter the actual value and second parameter the expected value. The alertInstanceFactory
accepts first the expected value and then the actual value. Maybe a symmetry in the order of parameters between the two functions would makes sense? I just had to ensure that arguments were passed the correct way because they were the other way around.
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.
Good point - I'll change the order of arguments.
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 merged without it to unblock dependent PRs. I'll create a separate PR for the improvement: #106532
const { field, comparator, value } = criterion; | ||
return `${field} ${comparator} ${value}`; | ||
}) | ||
.join(' and '); |
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.
Shall this be internationalized?
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 didn't change the content here, just fixed it to avoid superfluous leading whitespace.
LGTM! |
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Felix Stürmer <[email protected]>
📝 Summary
This adds the
<alerts-namespace>.reason
(where<alerts-namespace>
is incorrect at the moment, see #102089) to the "technical" mappings. It also updates the log threshold executor to index the reason as part of the lifecycle and changes the browser-side formatter to pass through that indexed reason.closes #105785
🎨 Previews
🕵️ Review notes
kibana.rac.alert
right now. This will be fixed separately.