[Obs Alert Table] Refactor alert table registration and change default columns #175119

maryam-saeidi · 2024-01-18T14:20:19Z

Summary

This PR refactors the alert table registrations and registers a new alert table for the rule details page. This PR also fixes the SLO embeddable alert table issue by opening the flyout when the user clicks on the reason.

Here is the old vs the new table:

Old alert table	New alert table

🧪 How to test

Create some alerts and check the alert table on the alerts page, it should
- have 2 hours time range instead of 15 mins
- show the following fields
  - Triggered
  - Duration
  - Rule name
    - on hover: it shows rule category
  - Group
  - Observed value
  - Threshold
  - Tags
Check one of the rules page alert tables, it should not show the rule name column by default
Create an SLO and check the related alert table embeddable
- Clicking on the reason should show the flyout (this was the fix in this PR)
- Everything should work as before

What will be covered in the future PRs

Making sure all rules set the threshold field
Adding a column with difference (Needs to be discussed with ResponseOps team)
Having source column instead of group by field (Needs to be discussed with @maciejforcone )

apmmachine · 2024-01-18T14:20:32Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2024-01-18T17:10:23Z

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

mgiota · 2024-01-18T18:53:29Z

@maryam-saeidi Nice! Since you made some changes to the slo alerts table configuration, I suggest we update the How to test section to include this as well. I assigned my self as a reviewer to verify slo alerts embeddable works as expected, so I can update the description. As discussed I will create a ticket to start writing some tests for the embeddable, cause it reuses a shareable component and it is error prone to changes like that.

maryam-saeidi · 2024-01-22T14:46:36Z

@maryam-saeidi Nice! Since you made some changes to the slo alerts table configuration, I suggest we update the How to test section to include this as well. I assigned my self as a reviewer to verify slo alerts embeddable works as expected, so I can update the description. As discussed I will create a ticket to start writing some tests for the embeddable, cause it reuses a shareable component and it is error prone to changes like that.

@mgiota For clarification, I didn't change any functionality related to the SLO alert table embeddable, just fixed a bug that I noticed, so everything should work as before. That being said, I appreciate you reviewing this PR to make sure that's the case.
Also, I adjusted the How to test section in the PR description, but feel free to adjust it if you think something is missing. :)

mgiota · 2024-01-22T16:25:20Z

@maryam-saeidi Exactly as you said. I just wanted to verify that SLO alerts embeddable will stay intact. I tested it and works fine! Your PR fixes a bug, where flyout wouldn't open when clicking on the alert reason on the SLO alerts table embeddable. This bug was introduced with this PR.

Here's the new issue I created for writing tests for the SLO alerts table embeddable.

mgiota · 2024-01-22T16:29:06Z

Here's a few screenshots from my local testing. The new columns look great and clicking on the SLO alerts table embeddable reason column opens the flyout. Thanks for the fix!

...plugins/observability/public/components/alerts_table/register_alerts_table_configuration.tsx

mykolaharmash

approve for Infra changes ✨

benakansara · 2024-01-23T12:01:39Z

@maryam-saeidi Can we show "below 10" or "above 10" in the "Threshold" column instead of only threshold value? It will be easy to read Observed and Threshold values with this info. WDYT?
For Inventory rule, I see in the screenshot that the "Source" is empty, but can we use the same group key used to format the reason message (where host name is displayed)?

maryam-saeidi · 2024-01-23T13:16:00Z

@maryam-saeidi Can we show "below 10" or "above 10" in the "Threshold" column instead of only threshold value? It will be easy to read Observed and Threshold values with this info. WDYT?

We want to show the difference instead of the threshold based on Maciej's input, but since we need to discuss with ResponseOps team to see what is the best way to add this field, I created a separate ticket for it. Feel free to add a comment to this ticket if you have a different suggestion for that column then we can discuss it with Maciej.

For Inventory rule, I see in the screenshot that the "Source" is empty, but can we use the same group key used to format the reason message (where host name is displayed)?

When discussing with Maciej, we wanted to show a different source depending on what rule type is selected, so for now, I am just showing the group by field in that column. In case we want to show multiple fields we need to discuss it with ResponseOps and also come up with a list of fields per rule type, which I have this ticket for this topic.

maryam-saeidi · 2024-01-23T15:05:03Z

@XavierM To check the alert table loading.

benakansara · 2024-01-24T09:06:12Z

When discussing with Maciej, we wanted to show a different source depending on what rule type is selected, so for now, I am just showing the group by field in that column. In case we want to show multiple fields we need to discuss it with ResponseOps and also come up with a list of fields per rule type, which I have this #175141 for this topic.

It makes sense to show group by fields in Source. The Inventory rule doesn't have explicit group by field in UI, but it does grouping based on node type selected (Hosts, Kubernetes Pods, etc.). I think it falls under same category as group by fields, and would be useful to show this info in Source. If we want to make source clickable in future, we will also need this information extracted from Inventory alert.

We want to show the difference instead of the threshold based on Maciej's input, but since we need to discuss with ResponseOps team to see what is the best way to add this field, I created a separate #175139 for it. Feel free to add a comment to this ticket if you have a different suggestion for that column then we can discuss it with Maciej.

I see. Do you mean we will replace the Threshold column with Diff column or both Threshold and Diff columns will exist?

maryam-saeidi · 2024-01-24T12:16:04Z

It makes sense to show group by fields in Source. The Inventory rule doesn't have explicit group by field in UI, but it does grouping based on node type selected (Hosts, Kubernetes Pods, etc.). I think it falls under same category as group by fields, and would be useful to show this info in Source. If we want to make source clickable in future, we will also need this information extracted from Inventory alert.

I used ALERT_INSTANCE_ID instead of the group to cover inventory as well 👍🏻

I see. Do you mean we will replace the Threshold column with Diff column or both Threshold and Diff columns will exist?

Diff will replace the threshold and we will show the threshold information on hover.

benakansara

LGTM. Thanks for the changes to cover Inventory rule as well.

maryam-saeidi · 2024-01-24T15:59:21Z

...gins/observability/public/components/alerts_table/slo/get_slo_alerts_table_configuration.tsx

  sort: [
    {
-      [TIMESTAMP]: {
+      [ALERT_DURATION]: {


@mgiota, @umbopepato found the cause of the forever loading issue and it was due to sorting based on a field that we didn't have in the default columns.

Great finding @umbopepato 💪🏻

mgiota · 2024-01-24T16:14:04Z

@maryam-saeidi The PR is missing version label.

maryam-saeidi · 2024-01-24T16:22:04Z

@maryam-saeidi The PR is missing version label.

You mean the version that we want this change to be merged? It will be added automatically.

kibana-ci · 2024-01-25T15:16:19Z

💚 Build Succeeded

Buildkite Build
Commit: be17b8e

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`observability`	591	593	+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`	646.8KB	630.6KB	-16.3KB

Page load bundle

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

id	before	after	diff
`observability`	101.9KB	101.8KB	-94.0B

Unknown metric groups

async chunk count

id	before	after	diff
`observability`	23	22	-1

References to deprecated APIs

id	before	after	diff
`observability`	7	5	-2

History

💔 Build #189036 failed c493199
💛 Build #188956 was flaky 705d879
💛 Build #188717 was flaky 39b2e1e
💔 Build #188546 failed c2be17b
💔 Build #188507 failed e01c2ab

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

cc @maryam-saeidi

@maciejforcone

…t columns (elastic#175119) Closes elastic#174239 ## Summary This PR refactors the alert table registrations and registers a new alert table for the rule details page. This PR also fixes the SLO embeddable alert table issue by opening the flyout when the user clicks on the reason. Here is the old vs the new table: |Old alert table|New alert table| |---|---| |![image](https://github.com/elastic/kibana/assets/12370520/0289dc2d-b6e8-4727-96fa-b2cf6485e8e5)|![image](https://github.com/elastic/kibana/assets/12370520/226fe529-5ee0-4f5c-9e9f-5ee782c8afe6)| ## 🧪 How to test - Create some alerts and check the alert table on the alerts page, it should - have 2 hours time range instead of 15 mins - show the following fields - Triggered - Duration - Rule name - on hover: it shows rule category - Group - Observed value - Threshold - Tags - Check one of the rules page alert tables, it should not show the rule name column by default - Create an SLO and check the related alert table embeddable - Clicking on the reason should show the flyout (this was the fix in this PR) - Everything should work as before ## What will be covered in the future PRs - [ ] Making sure all rules set the `threshold` field - [ ] Adding a column with difference (Needs to be discussed with ResponseOps team) - [ ] Having source column instead of group by field (Needs to be discussed with @maciejforcone )

Refactor alert table registration and change default columns

cb6f582

maryam-saeidi added the release_note:feature Makes this part of the condensed release notes label Jan 18, 2024

maryam-saeidi self-assigned this Jan 18, 2024

maryam-saeidi added 3 commits January 18, 2024 16:06

Add rule category on hover for rule name

27c2cab

Merge branch 'main' into 174239-improve-obs-alert-table

819d5e4

Change default search box time range to now-2h

199c443

maryam-saeidi mentioned this pull request Jan 18, 2024

Add rule name to the default fields of alert table #172135

Closed

maryam-saeidi marked this pull request as ready for review January 18, 2024 17:07

maryam-saeidi requested a review from a team as a code owner January 18, 2024 17:07

Merge branch 'main' into 174239-improve-obs-alert-table

3801e00

botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Jan 18, 2024

Fix translations

f183b51

mgiota self-requested a review January 18, 2024 18:48

Adjust number of cells and columns in the alert table tests

3c3f42e

maryam-saeidi requested review from a team as code owners January 22, 2024 14:41

Merge branch 'main' into 174239-improve-obs-alert-table

e01c2ab

maryam-saeidi added 2 commits January 22, 2024 16:58

Fix default alert search bar time range in test

113e3c5

Merge branch 'main' into 174239-improve-obs-alert-table

c2be17b

mgiota mentioned this pull request Jan 22, 2024

[SLO] Write tests for SLO alerts table embeddable #175228

Closed

mgiota reviewed Jan 22, 2024

View reviewed changes

...plugins/observability/public/components/alerts_table/register_alerts_table_configuration.tsx Outdated Show resolved Hide resolved

mgiota reviewed Jan 22, 2024

View reviewed changes

...plugins/observability/public/components/alerts_table/register_alerts_table_configuration.tsx Show resolved Hide resolved

smith added the apm:review label Jan 22, 2024

mykolaharmash approved these changes Jan 23, 2024

View reviewed changes

maryam-saeidi added 2 commits January 23, 2024 13:59

Remove any type and fix test

e5951ec

Merge branch 'main' into 174239-improve-obs-alert-table

39b2e1e

mgiota approved these changes Jan 24, 2024

View reviewed changes

maryam-saeidi added 2 commits January 24, 2024 13:12

Use ALERT_INSTANCE_ID instead of ALERT_GROUP_VALUE

8202ace

Merge branch 'main' into 174239-improve-obs-alert-table

705d879

benakansara approved these changes Jan 24, 2024

View reviewed changes

umbopepato mentioned this pull request Jan 24, 2024

[Response Ops] Alerts table issues #171738

Closed

maryam-saeidi added 2 commits January 24, 2024 16:55

Fix sorting

024ae8d

Merge branch 'main' into 174239-improve-obs-alert-table

c493199

maryam-saeidi commented Jan 24, 2024

View reviewed changes

maryam-saeidi added 3 commits January 25, 2024 15:06

Fix test after changing the default sort

e45faae

Add type

a26ebb0

Merge branch 'main' into 174239-improve-obs-alert-table

be17b8e

maryam-saeidi merged commit 86ad245 into elastic:main Jan 25, 2024
18 checks passed

maryam-saeidi deleted the 174239-improve-obs-alert-table branch January 25, 2024 15:34

kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Obs Alert Table] Refactor alert table registration and change default columns #175119

[Obs Alert Table] Refactor alert table registration and change default columns #175119

maryam-saeidi commented Jan 18, 2024 •

edited

Loading

apmmachine commented Jan 18, 2024

elasticmachine commented Jan 18, 2024

mgiota commented Jan 18, 2024 •

edited

Loading

maryam-saeidi commented Jan 22, 2024

mgiota commented Jan 22, 2024

mgiota commented Jan 22, 2024

mykolaharmash left a comment

benakansara commented Jan 23, 2024 •

edited

Loading

maryam-saeidi commented Jan 23, 2024

maryam-saeidi commented Jan 23, 2024

benakansara commented Jan 24, 2024

maryam-saeidi commented Jan 24, 2024

benakansara left a comment

maryam-saeidi Jan 24, 2024

mgiota commented Jan 24, 2024

maryam-saeidi commented Jan 24, 2024

kibana-ci commented Jan 25, 2024

async chunk count

References to deprecated APIs

[Obs Alert Table] Refactor alert table registration and change default columns #175119

[Obs Alert Table] Refactor alert table registration and change default columns #175119

Conversation

maryam-saeidi commented Jan 18, 2024 • edited Loading

Summary

🧪 How to test

What will be covered in the future PRs

apmmachine commented Jan 18, 2024

🤖 GitHub comments

elasticmachine commented Jan 18, 2024

mgiota commented Jan 18, 2024 • edited Loading

maryam-saeidi commented Jan 22, 2024

mgiota commented Jan 22, 2024

mgiota commented Jan 22, 2024

mykolaharmash left a comment

Choose a reason for hiding this comment

benakansara commented Jan 23, 2024 • edited Loading

maryam-saeidi commented Jan 23, 2024

maryam-saeidi commented Jan 23, 2024

benakansara commented Jan 24, 2024

maryam-saeidi commented Jan 24, 2024

benakansara left a comment

Choose a reason for hiding this comment

maryam-saeidi Jan 24, 2024

Choose a reason for hiding this comment

mgiota commented Jan 24, 2024

maryam-saeidi commented Jan 24, 2024

kibana-ci commented Jan 25, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

References to deprecated APIs

History

maryam-saeidi commented Jan 18, 2024 •

edited

Loading

mgiota commented Jan 18, 2024 •

edited

Loading

benakansara commented Jan 23, 2024 •

edited

Loading