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

[Logs & Metrics UI] Create alert flyout to use columns in EuiExpression #69233

Closed
andreadelrio opened this issue Jun 16, 2020 · 8 comments
Closed
Assignees
Labels
Feature:Logs UI Logs UI feature 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

Comments

@andreadelrio
Copy link
Contributor

We just updated the Index Threshold trigger type to use the new props available in EuiExpression (display='columns' and isInvalid). In Kibana, the common expression components now have a new prop called display which you can set to fullWidth to enable this new layout. You would need to do something similar for other components that are using EuiExpression.

For reference, this is the result for Index Threshold:

image

@andreadelrio andreadelrio added the Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services label Jun 16, 2020
@elasticmachine
Copy link
Contributor

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

@weltenwort weltenwort added Feature:Logs UI Logs UI feature Feature:Metrics UI Metrics UI feature labels Jul 7, 2020
@Zacqary Zacqary self-assigned this Aug 24, 2020
@Zacqary
Copy link
Contributor

Zacqary commented Aug 24, 2020

I started working with this on the Metric Threshold alert, and my question is, do we actually want to implement this? I definitely want to see more visual consistency across alert types but switching away from inline expressions isn't seamless for metrics.

I think the columns layout looks good when the condition is expanded:

Screen Shot 2020-08-24 at 12 36 26 PM

But my instinct is that when you collapse the expression, it should go back to the inline view:
Screen Shot 2020-08-24 at 12 36 37 PM

And now with the FOR THE LAST expression still using fullWidth it looks kind of incongruous.

I tried taking fullWidth off of FOR THE LAST and it's still not ideal. Here's different states of multiple conditions expanded and collapsed:

Screen Shot 2020-08-24 at 12 38 59 PM

Screen Shot 2020-08-24 at 12 37 52 PM

Screen Shot 2020-08-24 at 12 38 16 PM

(Note that I did add the Complete the expression to generate a preview prompt when the condition's field is empty, which I do like. Metric Threshold isn't using EuiEmptyPrompt at all so at the very least I think we should incorporate that.)

@Zacqary
Copy link
Contributor

Zacqary commented Aug 24, 2020

Screen Shot 2020-08-24 at 2 07 43 PM

The layout works much more naturally with Inventory alerts

@Zacqary
Copy link
Contributor

Zacqary commented Aug 24, 2020

Screen Shot 2020-08-24 at 3 18 46 PM

Log alerts also look good. We'd need to decide on a consistent spacing. Note additional whitespace between Inventory conditions versus the lack of whitespace between Log conditions.

@Zacqary
Copy link
Contributor

Zacqary commented Aug 24, 2020

So in summary, the open questions are:

  • How do we get this layout to look good on Metric Threshold alerts?
  • Do we prefer this layout? Is the columnar layout easy enough to read with multiple-condition alert types, or is it worth breaking visual consistency to keep Logs/Metrics alerts using inline layouts?

@jasonrhodes
Copy link
Member

@elastic/observability-design how do we get this back on your radar so you can help us navigate this? The main issues are:

a) Multi-condition alerts, like @Zacqary explains in this comment above in this thread, and

b) Ratio alerts, which @Kerry350 explains here in the attached PR

Thanks!

@hbharding
Copy link
Contributor

Thanks for bringing this back up @jasonrhodes - I tried to do some quick designs for the multi-condition alerts awhile back, but hadn't considered how this would work with ratio alerts. I think we'll need to take another look at this to accommodate both types. Unfortunately I don't have the bandwidth to tackle this in 7.12, but perhaps we can get this scheduled for the 7.13 cycle. We can probably design and implement this in the same release. (cc @katrin-freihofner @katefarrar)

@smith
Copy link
Contributor

smith commented Feb 27, 2023

Not prioritizing this at this time.

@smith smith closed this as not planned Won't fix, can't repro, duplicate, stale Feb 27, 2023
@zube zube bot closed this as completed May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Logs UI Logs UI feature 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants