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

Remove unused code for redundant groupBy field in custom threshold and metric threshold rules #184787

Merged
merged 9 commits into from
Jul 8, 2024

Conversation

csyager
Copy link
Contributor

@csyager csyager commented Jun 4, 2024

Summary

Closes #184712

Previously, when a user tries to create an alert with a groupBy with a field that is already filtered down to just one match by the KQL query, the UI would warn them that the filter query already contains an exact match. However, this is not desirable behavior, as adding "redundant" groupBy fields can be used to trigger alerts for data storage scenarios. Regardless, this warning is not currently being enforced by the UI and the warning was never enforced by the API, so this change removes the logic.

Change that had initially implemented this logic: #111891

This screenshot shows an example alert configuration that would previously have gotten a warning:

Screenshot 2024-06-04 at 4 48 37 PM

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@csyager csyager requested a review from a team as a code owner June 4, 2024 21:32
@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-management Observability Management User Experience Team labels Jun 4, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@maryam-saeidi maryam-saeidi added the release_note:skip Skip the PR/issue when compiling release notes label Jun 5, 2024
@maryam-saeidi
Copy link
Member

buildkite test this

@maryam-saeidi
Copy link
Member

@csyager Thanks for your contribution! :)

Would you please fix the issue mentioned in the CI? You should be able to do so using the node scripts/i18n_check.js --fix command.

@csyager
Copy link
Contributor Author

csyager commented Jun 5, 2024

@elasticmachine merge upstream

@csyager
Copy link
Contributor Author

csyager commented Jun 5, 2024

buildkite test this

@csyager
Copy link
Contributor Author

csyager commented Jun 5, 2024

@maryam-saeidi I must be doing something wrong. The buildkite links are giving me "Page not found." I'm not finding anything in the contributor docs about it, any idea what I'm missing?

@maryam-saeidi
Copy link
Member

@csyager Maybe it is due to not having permission, here is the error:

Committing the result of the node scripts/i18n_check.js --fix command should fix it.

@maryam-saeidi
Copy link
Member

buildkite test this

1 similar comment
@maryam-saeidi
Copy link
Member

buildkite test this

@maryam-saeidi maryam-saeidi self-requested a review June 13, 2024 15:03
Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in the review. I've tested the PR locally, and it worked as expected. ✨

I saw in the PR that this change was introduced, an errorOptions prop was added to the MetricsExplorerGroupBy component, any reason for not removing that prop too?

If there is no usage for it in the code, would you please remove that prop both from MetricsExplorerGroupBy component for metric threshold and GroupBy component for the custom threshold rule? Thanks!

@csyager
Copy link
Contributor Author

csyager commented Jun 22, 2024

Not a problem, thanks for taking a look. I thought about removing that too, I'll go ahead and do that and put a new commit in.

@csyager csyager requested a review from a team as a code owner June 22, 2024 06:01
@csyager
Copy link
Contributor Author

csyager commented Jun 22, 2024

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@csyager csyager force-pushed the csyager/unused-groupby-code branch from d916f06 to 991fc6e Compare June 22, 2024 15:33
@maryam-saeidi
Copy link
Member

buildkite test this

@maryam-saeidi
Copy link
Member

buildkite test this

@maryam-saeidi
Copy link
Member

buildkite test this

Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally, and filter and group were saved as expected.

Copy link
Contributor

@MiriamAparicio MiriamAparicio left a comment

Choose a reason for hiding this comment

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

LGTM

@maryam-saeidi
Copy link
Member

buildkite test this

@kibana-ci
Copy link
Collaborator

⏳ Build in-progress

  • Buildkite Build
  • Commit: fa1a4c4
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-184787-fa1a4c4b823e

Failed CI Steps

History

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

@maryam-saeidi
Copy link
Member

buildkite test this

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 8, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #10 / TemplateTags renders loading state

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
infra 1.5MB 1.5MB -1.5KB
observability 365.7KB 364.3KB -1.4KB
total -2.9KB

History

@maryam-saeidi maryam-saeidi merged commit 6c1e955 into elastic:main Jul 8, 2024
24 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project 💝community release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unused code related to the redundant groupBy field in custom threshold and metric threshold rules
7 participants