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

Open/Closed filter for observability alerts page #99217

Merged
merged 16 commits into from
May 25, 2021

Conversation

smith
Copy link
Contributor

@smith smith commented May 4, 2021

Allow filtering open/closed/all on the alerts page.

CleanShot 2021-05-25 at 08 43 49

As discussed on Slack with @katrin-freihofner, @cyrille-leclerc, and @jasonrhodes, we're changing the labels from "active"/"recovered" to "open"/"closed" to match their actual status at this time.

Fixes #98609.

@smith smith added release_note:skip Skip the PR/issue when compiling release notes Team:Observability Team label for Observability Team (for things that are handled across all of observability) v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed Theme: rac label obsolete labels May 4, 2021
@smith smith marked this pull request as draft May 4, 2021 15:59
@smith smith marked this pull request as ready for review May 24, 2021 17:46
@smith smith requested review from jasonrhodes, katrin-freihofner and a team May 24, 2021 17:47
@jasonrhodes
Copy link
Member

What's the best default here? My instinct says "Active", which maybe makes it better to do Active | Recovered | All, but I'm open to being convinced otherwise. :)

@@ -47,3 +47,41 @@ HTML coverage report can be found in target/coverage/jest after tests have run.
```bash
open target/coverage/jest/index.html
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this! 👏


export type Maybe<T> = T | null | undefined;

export const alertStatusRt = t.union([t.literal('all'), t.literal('open'), t.literal('closed')]);
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really in scope of this ticket directly, but I'm nervous that we're still using "open" and "closed" to mean "active" and "recovered" because they feel like very different concepts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only place "active" and "recovered" is used is on the control itself. kibana.rac.alert.status can be "open" or "closed". They aren't different in the context of observability alerts. @katrin-freihofner do you think we should change the text in the toggle control to be "open"/"closed"?

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to keep active and recovered as the language here (imo), but that the concepts are different in the underlying data so we'll eventually not want them linked. We've talked about it in 3 or 4 places but these things are hard to keep track of. But there will eventually be a difference between workflow status and active/recovered, since one is controlled by humans and the other is controlled by data + conditions of the rule.

Does the alert schema not have any other status fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Oh thanks. I think we may need to use alerts that have this value set:

kibana.rac.alert.end: the ISO timestamp of the time at which the alert recovered.

I just now realized we probably want a way to have the investigation window (date range) apply to recovery date and not just start date ... but that's for later :P

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it for now and address it as it comes up, but it will be important not to forget because I'm not 100% sure we will set that status to closed on resolution ... or if the semantics of when it gets set to closed / open could change and be surprising to us later.

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

LGTM on behalf of Security Solution -- thanks @smith! 🙂

@smith smith requested review from a team as code owners May 24, 2021 20:28
@smith
Copy link
Contributor Author

smith commented May 25, 2021

@jasonrhodes updated with your recommendation on the defaults and order in 3b17d0d.

image

Copy link
Contributor

@katrin-freihofner katrin-freihofner left a comment

Choose a reason for hiding this comment

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

@smith how many alerts does the table show by default? I'm asking because if it does not have any performance implications, I think we should show 50+ to make it easy for our users to go through them.

@smith
Copy link
Contributor Author

smith commented May 25, 2021

@smith how many alerts does the table show by default? I'm asking because if it does not have any performance implications, I think we should show 50+ to make it easy for our users to go through them.

@katrin-freihofner We're not doing anything about pagination until we add TGrid. Right now the response returns up to results with no pagination and you can give it a size parameter.

@smith smith changed the title Active/Recovered filter for observability alerts page Open/Closed filter for observability alerts page May 25, 2021
Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

LGTM!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 280 282 +2

Async chunks

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

id before after diff
observability 458.0KB 460.3KB +2.4KB

History

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

Copy link
Contributor

@ogupte ogupte left a comment

Choose a reason for hiding this comment

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

APM changes LGTM

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request May 25, 2021
Co-authored-by: Nathan L Smith <nathan.smith@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 26, 2021
…deprecation-ilm-policy

* 'master' of github.com:elastic/kibana: (101 commits)
  [ftr] migrate "docTable" service to FtrService class (elastic#100595)
  [ftr] migrate "listingTable" service to FtrService class (elastic#100606)
  Fixed comparing real value with formatted according to mode. (elastic#100456)
  [ftr] migrate "dataGrid" service to FtrService class (elastic#100593)
  [ftr] migrate "fieldEditor" to FtrService class (elastic#100597)
  [ftr] migrate "filterBar" service to FtrService class (elastic#100601)
  [triggersActionsUi] Reduce page load bundle to under 100kB (elastic#97770)
  [build] Clean jest configs (elastic#100594)
  refact(NA): remove extra pkg_npm target and add specific target folders for @kbn/analytics on Bazel (elastic#100569)
  Update dependency @elastic/charts to v29.2.0 (elastic#100587)
  [Maps] convert LayerPanel to typescript (elastic#100481)
  [Upgrade Assistant] Address copy feedback (elastic#99632)
  Open/Closed filter for observability alerts page (elastic#99217)
  One liner to expose the EQL query for debugging for users (elastic#100565)
  [KibanaPageLayout] Solution Nav specific styles & props (elastic#100089)
  [ftr] implement FtrService classes and migrate common services (elastic#99546)
  [XY] [Lens] Adds opacity slider (elastic#100453)
  [Reporting] ILM policy for managing reporting indices (elastic#100130)
  [Reporting] ILM policy for managing reporting indices (elastic#100130)
  [DOCS] Remove redundant maps attribute (elastic#100426)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/lib/store/report_ilm_policy.ts
#	x-pack/plugins/reporting/server/lib/store/store.test.ts
#	x-pack/plugins/reporting/server/lib/store/store.ts
ecezalp pushed a commit to ecezalp/kibana that referenced this pull request May 26, 2021
ecezalp pushed a commit to ecezalp/kibana that referenced this pull request May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Observability Team label for Observability Team (for things that are handled across all of observability) Theme: rac label obsolete v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alerts page Open/Closed toggle filter
7 participants