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] Add Alert Details page header #140299

Conversation

CoenWarmer
Copy link
Contributor

@CoenWarmer CoenWarmer commented Sep 8, 2022

Summary

This PR adds two components:

  • PageTitle: the title of the Alert Detail including the "Active / Inactive" badge underneath;
  • HeaderActions: the popover with "Add to case", "View / edit rule conditions" and "Snooze rule" functionalities.

Resolves #140030

Alert Detail Page with both components:
Screenshot 2022-09-27 at 16 57 48

Pressing the "Actions" button
Screenshot 2022-09-27 at 17 06 56

Add Alert to case:
Screenshot 2022-09-27 at 16 57 57

View edit rule condition:
Screenshot 2022-09-27 at 16 58 05

Snooze rule:
Screenshot 2022-09-27 at 17 02 14

All functionalities in the header actions component have been implemented.

Additionally, PageTitle has been added to Storybook.

Checklist

Delete any items that are not applicable to this PR.

@CoenWarmer CoenWarmer changed the title [Draft] Add Alert Details page header [Actionable Observability] [Draft] Add Alert Details page header Sep 8, 2022
@CoenWarmer CoenWarmer added v8.5.0 release_note:feature Makes this part of the condensed release notes labels Sep 8, 2022
@CoenWarmer CoenWarmer marked this pull request as draft September 8, 2022 15:25
@CoenWarmer CoenWarmer requested a review from a team September 8, 2022 15:25
@CoenWarmer CoenWarmer force-pushed the feat/140030-header-and-actions-for-alert-detail-page branch 2 times, most recently from 5ac1b02 to 9000318 Compare September 12, 2022 13:10
@CoenWarmer CoenWarmer marked this pull request as ready for review September 12, 2022 15:48
@CoenWarmer CoenWarmer changed the title [Actionable Observability] [Draft] Add Alert Details page header [Actionable Observability] Add Alert Details page header Sep 12, 2022
@fkanout fkanout self-requested a review September 12, 2022 15:56
@CoenWarmer CoenWarmer force-pushed the feat/140030-header-and-actions-for-alert-detail-page branch from 6cc1661 to 2e95480 Compare September 12, 2022 17:02
@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 Sep 13, 2022
@elasticmachine
Copy link
Contributor

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

@CoenWarmer CoenWarmer force-pushed the feat/140030-header-and-actions-for-alert-detail-page branch 2 times, most recently from 6107987 to 68f062d Compare September 13, 2022 09:11
@CoenWarmer
Copy link
Contributor Author

@elasticmachine merge upstream

@maryam-saeidi
Copy link
Member

Great to have a storybook as well! 🎉
I am waiting for the build to see the component in action! :D

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.

@CoenWarmer, I left you some comments. However, awesome work here! And thank you for shipping the PR with tests.

@CoenWarmer CoenWarmer marked this pull request as draft September 19, 2022 10:30
@CoenWarmer CoenWarmer force-pushed the feat/140030-header-and-actions-for-alert-detail-page branch 7 times, most recently from d801ba2 to f6c00db Compare September 27, 2022 14:45
@CoenWarmer CoenWarmer force-pushed the feat/140030-header-and-actions-for-alert-detail-page branch from 239e6ed to 1e9691a Compare September 29, 2022 10:49
Copy link
Member

@maryam-saeidi maryam-saeidi 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, LGTM!

Question: When I snooze the related rule, should I still see the active badge?

@CoenWarmer
Copy link
Contributor Author

Tested Locally, LGTM!

Question: When I snooze the related rule, should I still see the active badge?

A good question. I am not totally sure. @maciejforcone, @vinaychandrasekhar could you weigh in?

? [
{
alertId: alert?.fields[ALERT_UUID] || '',
index: '.internal.alerts-observability.metrics.alerts-*',
Copy link
Contributor

Choose a reason for hiding this comment

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

The API that we are using to fetch alert data can be possibly modified to return index as well. Currently it returns _source. The alert.hits.hits[0] also has _index property.
@simianhacker @fkanout what do you think?

Copy link
Contributor

@benakansara benakansara Sep 30, 2022

Choose a reason for hiding this comment

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

@simianhacker Not related to the scope of PR, but there is a comment above this line of code "move away from pulling data from _source in the future". What is the other way if we don't want to use "_source"? Is this implemented somewhere else to have a look?

Copy link
Member

Choose a reason for hiding this comment

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

The API that we are using to fetch alert data can be possibly modified to return index as well. Currently it returns _source. The alert.hits.hits[0] also has _index property. @simianhacker @fkanout what do you think?

What would we use the _index field for?

What is the other way if we don't want to use "_source"? Is this implemented somewhere else to have a look?

The alternative is to use the fields API (which is a setting on the request). I think the fields API becomes more advantageous when the index is not setup to store the _source and only has fieldData. I believe the fields API also resolves runtime fields as well (which we are not using).

Copy link
Contributor

Choose a reason for hiding this comment

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

@simianhacker We need the index name here - index: '.internal.alerts-observability.metrics.alerts-*',
Currently it is hardcoded.

Copy link
Member

Choose a reason for hiding this comment

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

Then yes, we need to get the _index field as well.

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. Nifty work here, @CoenWarmer!
I left some comments for minor issues.

Copy link
Contributor

@benakansara benakansara left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally, works as expected. Nice work!! 👏

@CoenWarmer CoenWarmer force-pushed the feat/140030-header-and-actions-for-alert-detail-page branch from 0ad5fe1 to 7d2bd7d Compare September 30, 2022 13:31
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.

LGTM, @CoenWarmer, good job!
Following up PRs to address the comments in this one.

@CoenWarmer CoenWarmer enabled auto-merge (squash) October 3, 2022 12:39
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 469 471 +2
triggersActionsUi 506 508 +2
total +4

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
triggersActionsUi 485 487 +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 523.6KB 526.4KB +2.8KB
triggersActionsUi 665.8KB 672.3KB +6.5KB
total +9.3KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
triggersActionsUi 48 49 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
triggersActionsUi 98.9KB 99.1KB +176.0B
Unknown metric groups

API count

id before after diff
triggersActionsUi 512 514 +2

async chunk count

id before after diff
triggersActionsUi 49 50 +1

ESLint disabled line counts

id before after diff
triggersActionsUi 130 131 +1

Total ESLint disabled count

id before after diff
triggersActionsUi 133 134 +1

History

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

@CoenWarmer CoenWarmer merged commit c7a5660 into elastic:main Oct 4, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 4, 2022
@CoenWarmer CoenWarmer deleted the feat/140030-header-and-actions-for-alert-detail-page branch October 4, 2022 08:03
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
)

Co-authored-by: Faisal Kanout <faisal.kanout@elastic.co>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
)

Co-authored-by: Faisal Kanout <faisal.kanout@elastic.co>
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 release_note:skip Skip the PR/issue when compiling 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] - Create the header and the actions for the Alert Details page
10 participants