[Response Ops][Alerting] Adding group by options to ES query rule type #144689
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…g/es-query-grouping
…g/es-query-grouping
…g/es-query-grouping
ymao1
commented
Nov 9, 2022
| filter: [ | ||
| ...filterWithTime, | ||
| { | ||
| match_all: {}, |
There was a problem hiding this comment.
not sure why this match_all was in here, so removed.
ymao1
commented
Nov 9, 2022
| @@ -12,12 +12,6 @@ import { DataPublicPluginStart } from '@kbn/data-plugin/public'; | |||
| import { DataViewEditorStart } from '@kbn/data-view-editor-plugin/public'; | |||
| import { EXPRESSION_ERRORS } from './constants'; | |||
|
|
|||
| export interface Comparator { | |||
There was a problem hiding this comment.
this wasn't used anywhere
…g/es-query-grouping
…with showing existing rule params
…g/es-query-grouping
…g/es-query-grouping
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
doakalexi
approved these changes
Dec 12, 2022
|
@elasticmachine merge upstream |
…g/es-query-grouping
pmuellr
approved these changes
Dec 14, 2022
There was a problem hiding this comment.
LGTM - tested locally, works as expected.
I added a question on the migration, as it's technically not needed if we defaulted the values. More of a thinking exercise about future "rolling upgrade" migrations really ...
Also noted a check to see if the rule is grouping by checking the alert id, which will fail if the alert id for a grouped rule happens to equal the alert id we use for ungrouped rules. Seems like we can make that more robust.
| ...doc.attributes, | ||
| params: { | ||
| ...doc.attributes.params, | ||
| aggType: 'count', |
There was a problem hiding this comment.
Did we consider defaulting these fields, if not set in the params, to the values being set here? I can see in various places in the code where we are apply defaults for these, at least in the UX. I guess when referenced by the executorIt avoids migration, but ... kinda messy/sloppy?
It's certainly nice to have it very explicit, and don't have an issue with this migration - but am kinda wondering now with the "migration changes for rolling upgrades", if this pushes us in one direction or the other ...
There was a problem hiding this comment.
I did consider it but in the past we've always taken the approach of "explicit is better" so I went for the migration instead. Definitely something we'll have to consider harder with rolling upgrades though.
x-pack/plugins/stack_alerts/server/rule_types/es_query/executor.ts
Outdated
Show resolved
Hide resolved
lcawl
reviewed
Dec 14, 2022
.../stack_alerts/public/rule_types/es_query/rule_common_expressions/rule_common_expressions.tsx
Outdated
Show resolved
Hide resolved
...s/stack_alerts/public/rule_types/es_query/rule_common_expressions/threshold_help_popover.tsx
Outdated
Show resolved
Hide resolved
…g/es-query-grouping
Co-authored-by: Lisa Cawley <[email protected]>
…/kibana into alerting/es-query-grouping
💛 Build succeeded, but was flakyFailed CI StepsTest Failures
Metrics [docs]Module Count
Adoption-tracked APIs
Adoption-tracked APIs that are not used anywhere
Public APIs missing comments
Any counts in public APIs
Async chunks
Public APIs missing exports
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
async chunk count
ESLint disabled in files
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
Unreferenced deprecated APIs
History
To update your PR or re-run it, just comment with: cc @ymao1 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #89481
Summary
Adds group by options to the ES query rule type, both DSL and KQL options. This is the same limited group by options that are offered in the index threshold rule type so I used the same UI components and rule parameter names. I moved some aggregation building code to
commonso they could be reused. All existing ES query rules are migrated to becount over allrules.To Verify
count over allrule - this should run the same as before, where it counts the number of documents that matches the query and applies the threshold condition to that value.{{context.hits}}is all the documents that match the query if the threshold condition is met.<metric> over allrule - this calculates the specific aggregation metric and applies the threshold condition to the aggregated metric (for example,avg event.duration).{{context.hits}}is all the documents that match the query if the threshold condition is met.count over top N terms- this will apply a term aggregation to the query and matches the threshold condition to each term bucket (for example,count over top 10 event.actionwill apply the threshold condition to the count of documents within eachevent.actionbucket).{{context.hits}}is the result of the top hits aggregation within each term bucket if the threshold condition is met for that bucket.<metric> over top N terms- this will apply a term aggregation and a metric sub-aggregation to the query and matches the threshold condition to the metric value within each term bucket (for example,avg event.duration over top 10 event.actionwill apply the threshold condition to the average value ofevent.durationwithin eachevent.actionbucket).{{context.hits}}is the result of the top hits aggregation within each term bucket if the threshold condition is met for that bucket.Checklist