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

[Actionable Observability] Integrate alert search bar on rule details page #144718

Conversation

maryam-saeidi
Copy link
Member

@maryam-saeidi maryam-saeidi commented Nov 7, 2022

Resolves #143962

📝 Summary

In this PR, an alerts search bar was added to the rule details page by syncing its state to the URL. This will enable navigating to the alerts table for a specific rule with a filtered state based on active or recovered.

Notes

  • Renamed alert page container to alert search bar container and used it both in alerts and rule details page (it will be responsible to sync search bar params to the URL) --> moved to a shared component
  • Moved AlertsStatusFilter to be a sub-component of the shared observability search bar
  • Allowed ObservabilityAlertSearchBar to be used both as a stand-alone component and as a wired component with syncing params to the URL (ObservabilityAlertSearchBar, ObservabilityAlertSearchbarWithUrlSync)
  • Set a minHeight for the Alerts and Execution tab, otherwise, the page will have extra scroll on the tab change while content is loading (very annoying!)

🎨 Preview

image

🧪 How to test

  • Create a rule and go to the rule details page
  • Click on the alerts tab and change the search criteria, you should be able to see the criteria in the query parameter
  • Refresh the page, alerts tab should be selected and you should be able to see the filters that you applied in the previous step
  • As a side test, check alert search bar on alerts page as well, it should work as before


import { toQuery, fromQuery } from './url';

describe('toQuery', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

💬 Successfully copy pasted from APM 😂

@@ -36,15 +34,6 @@ export interface FetchRuleSummaryProps {
ruleId: string;
http: HttpSetup;
}
export interface FetchRuleActionConnectorsProps {
Copy link
Member Author

Choose a reason for hiding this comment

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

💬 Some of the types are duplicated and unused, so I removed them.

@maryam-saeidi maryam-saeidi marked this pull request as ready for review November 8, 2022 13:32
@maryam-saeidi maryam-saeidi requested a review from a team as a code owner November 8, 2022 13:32
@maryam-saeidi maryam-saeidi added the release_note:feature Makes this part of the condensed release notes label Nov 8, 2022
@maryam-saeidi maryam-saeidi self-assigned this Nov 8, 2022
@maryam-saeidi maryam-saeidi added the Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" label Nov 8, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/actionable-observability (Team: Actionable Observability)

@CoenWarmer
Copy link
Contributor

  • After entering a query and reloading the page, the page goes to the alert tab and shows previous query ✅

Did notice that when an incorrect query is entered, the page goes blank.

To reproduce:

  1. Enter a query: host.cpu.usage >= "25%""" (notice double quotes at the end)
  2. Page goes blank
  3. Console shows:

Screenshot 2022-11-08 at 16 54 46

1 similar comment
@CoenWarmer
Copy link
Contributor

  • After entering a query and reloading the page, the page goes to the alert tab and shows previous query ✅

Did notice that when an incorrect query is entered, the page goes blank.

To reproduce:

  1. Enter a query: host.cpu.usage >= "25%""" (notice double quotes at the end)
  2. Page goes blank
  3. Console shows:

Screenshot 2022-11-08 at 16 54 46

@maryam-saeidi
Copy link
Member Author

Did notice that when an incorrect query is entered, the page goes blank.

To reproduce:

  1. Enter a query: host.cpu.usage >= "25%""" (notice double quotes at the end)
  2. Page goes blank
  3. Console shows:

@CoenWarmer Nice catch! Unfortunately, this is an issue that we also have on the Alerts page and I've created a ticket for it previously (#143641)
Since the component is shared between these 2 pages, when we fix it in TriggersActionsUI, then it will be fixed everywhere.
I will add your example to the ticket as well 👍🏻

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #1 / Upgrade Assistant Deprecation pages "before all" hook for "renders the Elasticsearch deprecations page"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 426 431 +5

Async chunks

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

id before after diff
observability 485.8KB 486.6KB +819.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 108 113 +5
securitySolution 440 446 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 67 73 +6
osquery 109 115 +6
securitySolution 517 523 +6
total +20

History

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

cc @maryam-saeidi

@CoenWarmer
Copy link
Contributor

Hey @maryam-saeidi, code looks excellent.

I am a little concerned that if this gets merged before the issue in TriggersActionsUI is fixed, end-users could have a sub par experience. So I am going to approve, but I will leave it to your best judgement whether or not it is good to merge for 8.6.0.

Copy link
Contributor

@fkanout fkanout left a comment

Choose a reason for hiding this comment

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

Tested locally and it works as expected and I like the fast autocompletion which not only for the field name but also its value!! 👏🏻

@maryam-saeidi, I found something confusing (we have discussed this topic previously) when we set kibana.alert.status : "active" in the search bar and then we apply the UI filter but with different value e.g recovered, the table will show the active alert while in the UI filter says recovered i.e. the search override the UI filter
This could not be a big issue if the query in the search bar is simple, the user will figure it out. But if the user have many conditions in the search bar it could be misleading.

I would suggest once the user click on one of the UI filter, we remove any condition mentioned for kibana.alert.status in the search bar.
Screenshot 2022-11-09 at 15 18 20

@maryam-saeidi
Copy link
Member Author

maryam-saeidi commented Nov 9, 2022

Thanks @CoenWarmer and @fkanout for reviewing. 💖

@CoenWarmer Since this is an issue that already exists on the alerts page for a couple of versions, so, unfortunately, it already can cause issues on the alerts page which is IMO more important than the Rule Details page. For the related bug ticket, I think we should be able to fix it after the feature freeze and backport it.

@fkanout Indeed we discussed this case, I am not sure how often this issue might happen but if it is challenging for our users, we can implement it as you suggested we can come up with an idea on how to implement it because manipulating search input makes the code unnecessarily complicated and fragile. I assume when users are working with both filter and query input, they know both filters will be applied together (similar to the case that we have for the controls and filters in the search bar.)
Anyway, I will double check it with Vinay and I will create a ticket for it if it is needed.

@maryam-saeidi maryam-saeidi merged commit ef7c1a6 into elastic:main Nov 9, 2022
@kibanamachine kibanamachine added v8.6.0 backport:skip This commit does not require backporting labels Nov 9, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 9, 2022
* main:
  [Lens] Rearrange options (elastic#144891)
  [Actionable Observability] Integrate alert search bar on rule details page (elastic#144718)
  [Security Solution] [Exceptions] Adds options to create a shared exception list and to create a single item from the manage exceptions view (elastic#144575)
  [Actionable Observability] Add context.alertDetailsUrl to connector template for Uptime > Monitor status & Uptime TLS rules (elastic#144740)
  [Security Solution] [Feat] Add Bulk Events to Timeline. (elastic#142737)
  [TIP] Env specific cypress config (elastic#144894)
  skip flaky suite (elastic#144885)
  [Enterprise Search] Fixes Search Index page to go blank when connection lost (elastic#144022)
  [Cloud Posture] track findings pages (elastic#144822)
  [ContentManagement] Inspector flyout (elastic#144240)
  [Cloud Posture] Dashboard Redesign - data counter cards (elastic#144565)
  [TIP] Run e2e pipeline on CI (elastic#144776)
  [Guided onboarding] Config updates for the Security guide (elastic#144844)
  Cleanup unused code for claiming tasks by id (elastic#144408)
  Ping the response-ops team whenever a new connector type is registered (elastic#144736)
@maryam-saeidi maryam-saeidi deleted the 143962-integrate-alert-search-bar-on-rule-details-page branch December 19, 2022 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Alerting release_note:feature Makes this part of the condensed release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Actionable Observability] Use shareable alert search bar on Rule details page
6 participants