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

Replace alert workflow status (open/acknowledged/closed) and filter with alert status filter (active/recovered) in Alerts View #117686

Closed
vinaychandrasekhar opened this issue Nov 5, 2021 · 16 comments · Fixed by #118723 or #119608
Labels
Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Theme: rac label obsolete v8.0.0

Comments

@vinaychandrasekhar
Copy link

vinaychandrasekhar commented Nov 5, 2021

image

On the alerts page, we want to replace the "workflow status" EuiButtonGroup to show "alert status" instead. So, instead of "Open / Acknowledged / Closed", we'll have "Show all / Active / Recovered" instead.

Also, we should hide or remove the acknowledge/close/reopen actions in the table since they are no longer useful.

More details:

Mockup:

image

Please check with the Design team if further assistance is needed.

@botelastic botelastic bot added the needs-team Issues missing a team label label Nov 5, 2021
@vinaychandrasekhar vinaychandrasekhar added v8.0.0 Theme: rac label obsolete Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" labels Nov 5, 2021
@botelastic botelastic bot removed the needs-team Issues missing a team label label Nov 5, 2021
@fkanout fkanout self-assigned this Nov 8, 2021
@fkanout
Copy link
Contributor

fkanout commented Nov 9, 2021

@vinaychandrasekhar As you probably know, we have a filter in the table also based on the alerts status (Active, Recovered). In the screenshot (1 then 2)

Alerts-Observability-Elastic

However, when a filter is selected, e.g., Active, that will update the search bar with the correspondent query.

Alerts-Observability-Elastic (1)

Do you want to have the same behavior, which means updating the search bar when you click on one of the Show all / Active / Recovered buttons❓

@vinaychandrasekhar
Copy link
Author

@fkanout thanks for the question.
@hbharding could you please let us know if there is precedence for the filtering via the table "filter in" option (or by simply entering the filter in the search bar manually) plus offering the buttons to filter?

@hbharding
Copy link
Contributor

hbharding commented Nov 9, 2021

Thanks for catching this @fkanout.

I spoke with @vinaychandrasekhar and I think we should remove the "filter in" action from the Alert Status column since this can cause a conflicting query. For now, let's keep the current behavior the same where the EuiButtonGroup does not apply filters to the query bar above.


It's an interesting idea that the EuiButtonGroup options could interact with the query bar above it. There is precedence for this behavior but it's not consistent throughout Kibana. I imagine our behavior would look something like:

  • Show all = no query (i.e. shows everything)
  • Active = kibana.alert.status: "active"
  • Recovered = kibana.alert.status: "recovered"

It get's a little tricky when users are able to type and add their own queries to the mix. There are some things I'd like to think through some more before we go down this route. As one example, questions like should the EuiButtonGroup automatically change it's selected button to be Recovered if a user were to manually type kibana.alert.status: "recovered" in the query? Github does something similar:
image

Let's circle back on this in another issue. Created one here: #118078

@fkanout
Copy link
Contributor

fkanout commented Nov 10, 2021

@hbharding, thanks for your reply.

I think we should remove the "filter in" action from the Alert Status column

Yes, it's feasible.

Show all = no query (i.e. shows everything)
Active = kibana.alert.status: "active"
Recovered = kibana.alert.status: "recovered"

That's correct.

BTW, after some testing, I discovered that the current behavior is keeping the query bar and the URL in the browser always in sync when a filter is applied, and it's attentional for consistency.

It get's a little tricky when users can type and add their queries to the mix

Maybe it is worth mentioning #116135

As one example, questions like should the EuiButtonGroup automatically change its selected button to be Recovered if a user were to manually type kibana.alert.status: "recovered" in the query?

Currently, we have something similar, if you change the workflow status in the browser URL to closed, the closed button in EuiButtonGroup will be selected.

Screenshot 2021-11-10 at 13 48 21

That leaves us with two questions:

  • Is it ok to show the filter applied in the query bar once one of the EuiButtonGroup buttons is clicked (we don't really have a choice ¯_(ツ)_/¯ )?
  • Is it ok to active the correspondedEuiButtonGroup button when a filter is applied via the browser URL OR the query bar?

cc @vinaychandrasekhar

@hbharding
Copy link
Contributor

Yes to both questions! Thanks @fkanout

@vinaychandrasekhar
Copy link
Author

One follow up question @fkanout -

BTW, after some testing, I discovered that the current behavior is keeping the query bar and the URL in the browser always in sync when a filter is applied, and it's attentional for consistency.

When I test that on a snapshot 8.0 build, I only see the button and the browser URL being in sync but the KQL query bar is not kept in sync. What am I missing?
image

cc @hbharding

@hbharding
Copy link
Contributor

hbharding commented Nov 10, 2021

@vinaychandrasekhar I don't think @fkanout means they are truly "in sync". Rather, the Browser URL is able to capture all the currently applied filters from the page (whether that be from a search bar or button). This is useful because it allows someone to copy the URL and open it somewhere else and see the same exact view.

As you describe, the browser URL and KQL searchbar don't show the same information. The KQL query searchbar does not reflect all the currently applied filters like the Browser URL does. It's just one part of potentially many other filters. This is the part I think we should explore changing in another issue. I created this #118078 for now, which also feels related to the one @fkanout mentioned #116135

@vinaychandrasekhar
Copy link
Author

vinaychandrasekhar commented Nov 10, 2021

@hbharding ah, okay if that's what @fkanout meant, that makes sense to me.

Otherwise, keeping the KQL query bar in sync with button state might get tricky with needing to parse out or update complex KQL queries to get to alert status filters.

@fkanout
Copy link
Contributor

fkanout commented Nov 15, 2021

Thank you, @hbharding, that's correct!

When I test that on a snapshot 8.0 build, I only see the button and the browser URL being in sync, but the KQL query bar is not kept in sync. What am I missing?

@vinaychandrasekhar, as @hbharding mentioned, query search bar does not reflect all the currently applied filters like the Browser URL does . That's right, and the current "Open / Acknowledged / Closed" is one of this unreflected info.

Currently, adding a filter from the grid e.g., Active, will be reflected in the query search bar as mentioned in the screenshot here #117686 (comment)

@vinaychandrasekhar
Copy link
Author

@fkanout, yes that's what I'm seeing as well thanks.

@fkanout
Copy link
Contributor

fkanout commented Nov 18, 2021

Panagiota got to my attention that we still have a way to access the Mark as actions when selecting an alert using the checkbox.

I assume we need to hide these leading checkboxes. Do you confirm that? @hbharding, @vinaychandrasekhar?

Alerts-Observability-Elastic (1)

@vinaychandrasekhar
Copy link
Author

vinaychandrasekhar commented Nov 18, 2021

Hi @fkanout , yes please hide the ''Mark as'' actions in 8.0. And I assume if they are the only two actions possible, that the selection feature also needs to be hidden? cc @hbharding

@hbharding
Copy link
Contributor

Good catch @fkanout / @mgiota. Unless these are the only "bulk actions" we provide, I don't see a need to keep the checkboxes in the first column and the "Select X alerts" + "Select all X alerts" buttons above the table.

Just thinking ahead, I could see us potentially needing a way to bulk select and do:

  • Assign to case
  • Remove from case (maybe?)
  • Delete alert (maybe?)
  • ...

So while we don't need bulk selection today, it might be good to keep some of this code around / readily availabile in case we need to reimplement a bulk select feature in the future.

@fkanout
Copy link
Contributor

fkanout commented Nov 19, 2021

I was working on the keep-in-sync requirement, and yes, it was tricky, as @vinaychandrasekhar mentioned 😅
However, I tried to cover all possible use cases I can think of. I mentioned them in the PR under the Expected behavior section with an illustration for each one
Please feel free to check them and share your feedback if you have any! @hbharding @vinaychandrasekhar

@fkanout
Copy link
Contributor

fkanout commented Nov 23, 2021

Hiding the alerts' leading checkboxes could not be done in the linked PR, as there is a bug in the T Grid. (please check this issue for more details).

cc @hbharding @vinaychandrasekhar

@hbharding
Copy link
Contributor

Do you know what tests are failing and who to reach out to? Do you think it's possible to get this solved in another PR for 8.0?

If not, perhaps there are some short term alternatives we could pursue. For example, hiding the checkbox cells with CSS... probably not the best idea, but I'm open to other ideas if we need to pursue an alternative route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" Theme: rac label obsolete v8.0.0
Projects
None yet
3 participants