-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[alerting] initial index threshold alertType and supporting APIs #57030
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
Conversation
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
2e1ae16 to
87f95fd
Compare
|
Though this code isn't ready for a full review, there's a lot of new external function so I thought it best to make sure that's in good shape before finishing. Things to focus on:
TODOs:
|
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.
interval is actually ignored in the case of intervalsBefore +intervalsAfter === 1. Having to send something is awkward in this case, seems like it should be optional ... would be a validation error to have total intervals != 1 and interval not set ...
mikecote
left a comment
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.
High level architecture looks good to me so far 👍
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.
It looks like the runQuery function is the only thing we need to replace in order to reuse all of this logic for threshold alerting on other things (e.g. metrics). Maybe we can produce a factory that takes in a (params: Object) => QueryResult function to easily register threshold alert types with common functionality?
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.
Ya, that would be an interesting way to try to make this reusable. The runQuery function is also used to generate data for the UI preview, and perhaps history view. So if we spec it just right, it could be possible to supply a runQuery function that would get used in all those environments. I think the params and probably need to be configurable, but the result (at least right now) for the query is simply an array of [dateString, number] tuples.
I'd like to get this merged into master before looking at generalizing it more, but I'll keep this in mind while finishing this.
I should mention that 1/4 of the content of this code is setting the context for the scheduleActions() call - and most of the things being set are generic off the alert, params, etc. We've talked about providing these for "free" to action groups, so that we'd still have the context we currently support, for alertType-specific things, but you would also somehow have access to everything in the alert as well, based on some other variable.
e2de083 to
23c50cc
Compare
|
For some reason, never linked this PR to it's issue, which is here: issue #53041 |
4c1c281 to
a0b5dd6
Compare
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.
Pulled down locally and ran the example, it works! 🎉
PR LGTM. I'm good if this merges with only the i18n changes done in the TODO, issues can be created for the remaining. Looks like i18n is needed within the alert type definition and the action_context.ts messages.
One nit I have which doesn't have to be done now but I have a feeling the plugin's README will get busy fast if we write docs from each built in there. The index threshold specific docs could move within a new alert_types/index_threshold/README.md file.
Worth making that change. I want to have a good example for other alert authors to work off of, they may have multiple alert types, would be good if they did the same. |
YulNaumenko
left a comment
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.
LGTM!
|
index threshold doc moved into it's own directory in commit f82ca22 i18n all the things in commit cfbd390 The i18n commit forced me to refactor some of the validations to return the same error messages, which means they also got standardized to the same messages :-) Assuming this passes CI, will be ready to merge tomorrow!! |
|
@elasticmachine merge upstream |
…stic#57030) Adds the first built-in alertType for Kibana alerting, an index threshold alert, and associated HTTP endpoint to generate preview data for it. addresses the server-side requirements for issue elastic#53041
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_and_spaces.x-pack/test/saved_object_api_integration/security_and_spaces/apis/find·ts.saved objects security and spaces enabled find user with no access within the space_1 space "before all" hook for "finding a hiddentype should return 403 with forbidden find hiddentype message"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_and_spaces.x-pack/test/saved_object_api_integration/security_and_spaces/apis/find·ts.saved objects security and spaces enabled find user with no access within the space_1 space "after all" hook for "finding a hiddentype should return 403 with forbidden find hiddentype message"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack Saved Object API Integration Tests -- security_and_spaces.x-pack/test/saved_object_api_integration/security_and_spaces/apis/find·ts.saved objects security and spaces enabled find user with no access within the space_1 space "before all" hook for "finding a hiddentype should return 403 with forbidden find hiddentype message"Standard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
This is a follow-on to elastic#57030 , "[alerting] initial index threshold alertType and supporting APIs", to get it working with the existing alerting UI. The parameter shape was different between the two, so the alertType was changed to fix the existing UI shapes expected.
…old (elastic#59064) This is a follow-on to elastic#57030 , "[alerting] initial index threshold alertType and supporting APIs", to get it working with the existing alerting UI. The parameter shape was different between the two, so the alertType was changed to fix the existing UI shapes expected.
Summary
Alerting needs it's own HTTP endpoint to get data for alerting visualizations, much like what's shown in the current Kibana watcher UI when creating an index threshold alert.
Currently this is in a temporary NP plugin,
alerting_index_threshold, but belongs in thealertingplugin. As thealertingplugin is currently in the process of being converted to NP, using this temporary plugin as a home, until the conversion is merged to master, to avoid disruptions.Checklist
TODOs