-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Actionable Observability] Add Alert Details page header #140299
Conversation
5ac1b02
to
9000318
Compare
6cc1661
to
2e95480
Compare
Pinging @elastic/actionable-observability (Team: Actionable Observability) |
6107987
to
68f062d
Compare
@elasticmachine merge upstream |
x-pack/plugins/observability/public/pages/alert_details/types.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/types.ts
Outdated
Show resolved
Hide resolved
Great to have a storybook as well! 🎉 |
x-pack/plugins/observability/public/pages/alert_details/components/page_title.stories.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
x-pack/plugins/observability/public/pages/alert_details/translations.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/types.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/index.tsx
Outdated
Show resolved
Hide resolved
d801ba2
to
f6c00db
Compare
x-pack/plugins/observability/public/pages/alert_details/components/page_title.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/components/header_actions.tsx
Outdated
Show resolved
Hide resolved
239e6ed
to
1e9691a
Compare
x-pack/plugins/observability/public/pages/alert_details/components/header_actions.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/components/header_actions.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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?
x-pack/plugins/observability/public/pages/alert_details/components/page_title.test.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/components/page_title.tsx
Show resolved
Hide resolved
A good question. I am not totally sure. @maciejforcone, @vinaychandrasekhar could you weigh in? |
x-pack/plugins/observability/public/pages/alert_details/components/header_actions.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/components/header_actions.tsx
Outdated
Show resolved
Hide resolved
? [ | ||
{ | ||
alertId: alert?.fields[ALERT_UUID] || '', | ||
index: '.internal.alerts-observability.metrics.alerts-*', |
There was a problem hiding this comment.
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?
return alert.hits.hits[0]._source; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
x-pack/plugins/observability/public/pages/alert_details/components/header_actions.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/components/alert_summary.tsx
Show resolved
Hide resolved
x-pack/plugins/observability/public/pages/alert_details/components/header_actions.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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!! 👏
0ad5fe1
to
7d2bd7d
Compare
There was a problem hiding this 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.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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:
Pressing the "Actions" button
Add Alert to case:
View edit rule condition:
Snooze rule:
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.