Skip to content
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

Elasticsearch Query rule can group by multi-terms #166146

Merged
merged 20 commits into from Sep 18, 2023

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Sep 11, 2023

Resolves: #163829

This PR allows multiple group-by terms to be selected in Elasticsearch query rule.

Screenshot 2023-09-11 at 22 53 34 Screenshot 2023-09-11 at 22 53 53

@ersin-erdal ersin-erdal added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:feature Makes this part of the condensed release notes v8.11.0 labels Sep 11, 2023
@ersin-erdal ersin-erdal self-assigned this Sep 11, 2023
@ersin-erdal
Copy link
Contributor Author

@elasticmachine merge upstream

@ersin-erdal ersin-erdal marked this pull request as ready for review September 11, 2023 21:00
@ersin-erdal ersin-erdal requested a review from a team as a code owner September 11, 2023 21:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran into this while switching data view for the KQL query after selecting multiple group by terms:

Did not see this on main

Sep-13-2023.10-38-43.mp4

) {
errors.termField.push(
i18n.translate('xpack.stackAlerts.esQuery.ui.validation.error.overNumberedTermFieldText', {
defaultMessage: 'Max. 4 terms can be selected',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: should use MAX_SELECTABLE_GROUP_BY_TERMS instead of 4 in message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ersin-erdal
Copy link
Contributor Author

Ran into this while switching data view for the KQL query after selecting multiple group by terms:

Thanks for catching this, must be fixed now.

@ymao1
Copy link
Contributor

ymao1 commented Sep 14, 2023

@ersin-erdal Still seeing a little funky behavior:

Create DSL query, select an index and add some terms. Then go back and change the index selection. It looks like the group clears correctly, but once you click into it, the previous terms are still in the input but there's a validation error. I think the previous terms should be completely cleared out of that input?
Screenshot 2023-09-14 at 10 29 17 AM

@ersin-erdal
Copy link
Contributor Author

@ersin-erdal Still seeing a little funky behavior:

Create DSL query, select an index and add some terms. Then go back and change the index selection. It looks like the group clears correctly, but once you click into it, the previous terms are still in the input but there's a validation error. I think the previous terms should be completely cleared out of that input?

Should be fixed this time :)
I didn't want to change the existing clear fields logic but it doesn't work with the new combobox...

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Verified everything works as expected. Nice job!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #6 / serverless observability UI Configure Case "before all" hook in "Configure Case"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
stackAlerts 204.0KB 204.0KB -34.0B
triggersActionsUi 1.4MB 1.4MB +336.0B
total +302.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
stackAlerts 21.0KB 21.5KB +496.0B
triggersActionsUi 91.7KB 91.8KB +79.0B
total +575.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @ersin-erdal

@ersin-erdal ersin-erdal merged commit c486443 into elastic:main Sep 18, 2023
19 of 20 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 18, 2023
@ersin-erdal ersin-erdal deleted the 163829-multi-terms branch September 18, 2023 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple group-by layers in ES rule type
5 participants