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

[Metrics UI] Metrics Threshold doesn't warn about redundant groupBy when creating/editing through the API #116197

Closed
Zacqary opened this issue Oct 25, 2021 · 9 comments
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:obs-ux-management Observability Management User Experience Team

Comments

@Zacqary
Copy link
Contributor

Zacqary commented Oct 25, 2021

In #111891, we added this UI warning when the user tries to groupBy something that they're already filtering down to a single match:

Screen Shot 2021-09-10 at 1 12 02 PM

Unfortunately, the user doesn't see this warning when they edit alerts through the API. If they're trying to bulk edit a large group of alerts, or create them programmatically, they might end up with a lot of unnecessarily grouped alerts which are prone to errors.

Possible solutions are to:

  • Automatically remove the redundant groupBy fields, and send a warning in the response
  • Throw an error and require the user to resend the query with the offending groupBy fields removed (or with a force flag?)

Any implementation of this should be sure to also implement the OR fix detailed in #116195

@Zacqary Zacqary added bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Oct 25, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@smith smith added Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team and removed Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Nov 14, 2023
@elasticmachine
Copy link
Contributor

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

@botelastic
Copy link

botelastic bot commented May 12, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@botelastic botelastic bot added the stale Used to mark issues that were closed for being stale label May 12, 2024
@smith smith added the Team:obs-ux-management Observability Management User Experience Team label May 18, 2024
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot removed the stale Used to mark issues that were closed for being stale label May 18, 2024
@smith smith added stale Used to mark issues that were closed for being stale and removed Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels May 18, 2024
@botelastic botelastic bot removed the stale Used to mark issues that were closed for being stale label May 18, 2024
@smith
Copy link
Contributor

smith commented May 18, 2024

@elastic/obs-ux-management-team this came up as stale but it looks like it's for the alerting rule so I changed the team label.

@jasonrhodes
Copy link
Member

@maryam-saeidi do you know if we still do this check in the UI? I think we should probably not do it at all, since adding "Group By" fields is a way to trigger various alerts as data storage scenarios, so a user may have reasons for keeping a group by in place even if it's "redundant" as this issue describes.

@maryam-saeidi
Copy link
Member

I've tested locally, and I didn't see this check in the Metric threshold rule:

Checking the related PR, I see the code is there, but I am not sure why it is not working anymore. We also copied this logic to the custom threshold rule.

@jasonrhodes I agree with allowing users to add both group by field and filter for the same field (in this case, the value will be included in the instance id of the alert and can be used in the alert table), so shall we have a separate ticket for removing the related logic from both the custom threshold and metric threshold rules?

@jasonrhodes
Copy link
Member

jasonrhodes commented Jun 3, 2024

OK thanks, I vote to remove this code from all places (here, custom threshold.... anywhere else?)

We will create a new ticket to do this that references this one.

@maryam-saeidi
Copy link
Member

Created #184712 for removing unused code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:obs-ux-management Observability Management User Experience Team
Projects
None yet
Development

No branches or pull requests

5 participants