Alerts in overview page by mgiota · Pull Request #125337 · elastic/kibana

mgiota · 2022-02-11T07:35:09Z

Part of #124370. There will be some follow-up PR to polish the table for the overview page.

@estermv Here's a quick draft PR that brings the alerts table in Overview page. I did two changes to make this work:

created a useAlertIndexNames hook
migrated the OverviewPage to use a stateContainer similar to how AlertsPage was refactored in this PR [Observability] Make Alerts page use shared Kibana time range #115192

Above two changes were enough to bring existing alerts table in the Overview page.

I did this change both to old and new Overview page.
AlertsTableTGrid is expecting a setRefetch prop. I didn't dive deeper into this, so for now I just passed an empty function.

As you will see in the screenshots below, there are UI issues, since there is a min-height applied to the alerts table. We would need to see how we could parameterize it in the timelines plugin or playaround with css.

This is an initial draft. I can clean it up a bit and open it for review. Or you can take it from here. Let me know what's best for you.

mgiota · 2022-02-11T07:53:49Z

@XavierM There's a new requirement to display alerts in the Observability Overview page #124370 (comment). I drafted this PR to quickly bring existing Observability alerts table in overview page and pretty much works apart from an issue in the UI part.

From what I can tell there's a hardcoded min-height: 490 coming from timelines plugin. Do you think we could parameterize this? Also on latest main alerts table looks broken #125191 [Actionable Observability] Alerts table looks broken with unnecessary scrollbars. Since more pages want to display alerts table, I think we should also work on the responsiveness of the Alerts table.

mgiota · 2022-02-11T10:49:03Z

x-pack/plugins/timelines/public/components/t_grid/standalone/index.tsx

 import { AddToCaseAction } from '../../actions/timeline/cases/add_to_case_action';
 import { TGridLoading, TGridEmpty, TimelineContext } from '../shared';

+const FullWidthFlexGroup = styled(EuiFlexGroup)<{ $visible: boolean }>`


@XavierM FullWidthFlexGroup has a hardcoded min-height, so I stopped using the one defined in the styles https://github.com/elastic/kibana/blob/main/x-pack/plugins/timelines/public/components/t_grid/styles.tsx#L466 and redefined a new style for FullWidthFlexGroup. Do we actually need this hardcoded min-height? Even integrated tGrid gets rid of the min-height https://github.com/elastic/kibana/blob/main/x-pack/plugins/timelines/public/components/t_grid/integrated/index.tsx#L83

With this change table looks much better (apart from the unnecessary scrollbar which is related to another issue)

@mgiota either way should be fine 👍

Would it make sense to make the min-height a prop of the pre-existing FullWidthFlexGroup instead of duplicating it?

@afgomez It looks like min-height is not being used at all in the integrated version of t-grid and with this PR is it also removed from the standalone version of t-grid. Soon Alerts 2.0 is coming, where quite many things will be cleaned up. I suggest we keep it as it is for now if you don't mind.

estermv · 2022-02-14T12:34:21Z

@mgiota thanks a lot for this PR! I'm going to push some changes, if that's ok with you, mainly removing the old alerts table and probably reverting the changes to the new overview page.

I need to discuss this with the team, but I'm unsure of the need for the new page. It was a decision taken months ago about how to proceed with the overview page redesign and we thought that having a completely new page behind a feature toggle was the right approach, but now we are making changes to the "old" one and maybe that "new" page doesn't make sense anymore.

So, to keep future changes easier I think is better if we only make them to the "old" overview page. If in the future we want to have a "new" page we can add the alerts table there at that point.

…ections

estermv · 2022-02-14T15:51:50Z

@mgiota I pushed 3 commits to this PR:

I removed the old alerts section from the overview page and adapt the style to match the other sections from the page. I'll clean up the code in a future PR.
I removed the stateContainer. If we use it in the overview page it causes other problems (and without it, the table still seems to work):
We already have a mechanism to keep the timepicker in sync with the kibana one. I'm not really familiar with how it works but I think is better to keep all sections at the same time range. We can introduce it back as part of a future PR, after better understanding both implementations.

Revert changes from the "new" overview page as mentioned in my previous comment.

If you see that everything is fine, I think this PR should be ready for review.

estermv · 2022-02-14T16:04:39Z

x-pack/plugins/observability/public/pages/overview/old_overview_page.tsx

-                    </EuiPanel>
-                  </EuiFlexItem>
+                  <AlertsTableTGrid
+                    setRefetch={() => {}}


I'm also not sure what's this, but looking at the code for the alerts page maybe we don't need this. Maybe one option is to mark it as optional?

@estermv This was my initial thought as well, to make setRefetch optional, but then the whole page breaks, because apparently tGrid expects setRefetch to be there. I didn't have a closer look but you can have a look at how it is implemented in alerts_page https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/pages/alerts/containers/alerts_page/alerts_page.tsx#L222 and do something similar.

I added the setRefetch function. From what I understand setRefetch is used to set a function to update the tGrid from outside it. On the overview page, we need it when the user clicks on the "Refresh" button next to the datepicker.

mgiota · 2022-02-14T16:54:20Z

@estermv I agree with the changes you pushed to the PR. Especially removing the state container. I was planning to revert this changes, since you already have the mechanism to get the information you need from routeParams.

estermv · 2022-02-15T17:59:08Z

@elasticmachine merge upstream

estermv · 2022-02-16T15:07:03Z

@elasticmachine merge upstream

elasticmachine · 2022-02-17T09:50:18Z

Pinging @elastic/unified-observability (Team:Unified observability)

kqualters-elastic · 2022-02-17T19:53:36Z

x-pack/plugins/timelines/public/components/t_grid/standalone/index.tsx

 import { AddToCaseAction } from '../../actions/timeline/cases/add_to_case_action';
 import { TGridLoading, TGridEmpty, TimelineContext } from '../shared';

+const FullWidthFlexGroup = styled(EuiFlexGroup)<{ $visible: boolean }>`


@mgiota either way should be fine 👍

estermv · 2022-02-21T10:41:18Z

@elasticmachine merge upstream

fkanout · 2022-02-21T14:56:30Z

The behavior in the Alerts page, when no alerts occurred, is to show an empty table. Is consistent behavior between the Alerts page and the overview page required from the Alert table perspective?

Overview page without alerts:

Alerts page without alerts:

~~Note: Not related to this PR, but~~ The Alerts title at the top of the Overview page without any explanation. i.e., No alerts? Or there is an error while fetching the alerts.

fkanout · 2022-02-21T14:26:24Z

x-pack/plugins/observability/public/hooks/use_alert_index_names.ts

+
+export function useAlertIndexNames() {
+  const { data: indexNames = NO_INDEX_NAMES } = useFetcher(({ signal }) => {
+    return callObservabilityApi('GET /api/observability/rules/alerts/dynamic_index_pattern', {


Is it ok to hardcode this endpoint URL? Usually, this kind of code is shared centrally.

Do you mean to pass it as a parameter of the hook? I think in this case the URL is directly tied to the hook because, as far as I understand, this is the endpoint that will return us the index names, so I don't think it will make sense to use the hook with a different URL.
What do you think @mgiota?

@estermv I think it is fine to keep it here

fkanout · 2022-02-21T14:33:37Z

x-pack/plugins/observability/public/hooks/use_alert_index_names.ts

+        query: {
+          namespace: 'default',
+          registrationContexts: [
+            'observability.apm',


Same here for hardcoded registrationContexts. It seems that these values are technical filed-like?

I understand your point here, but I would prefer to keep this inside the hook because at this point we want always the same registrationContexts. We can always refactor it in the future if we need to.

@fkanout I see your point and it would be nice to have constants for the registration contexts. As part of this PR I extracted existing functionality to a new hook https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability/public/pages/alerts/containers/alerts_page/alerts_page.tsx#L148, so we could definitely refactor this in a follow up PR

afgomez

Code looks alright!

The thing I'm missing is an empty state window. When there are no alerts the page looks strange.

afgomez · 2022-02-21T14:03:52Z

x-pack/plugins/observability/public/hooks/use_alert_index_names.ts

@@ -0,0 +1,32 @@
+/*


Nicely extracted

afgomez · 2022-02-21T14:35:32Z

x-pack/plugins/timelines/public/components/t_grid/standalone/index.tsx

 import { AddToCaseAction } from '../../actions/timeline/cases/add_to_case_action';
 import { TGridLoading, TGridEmpty, TimelineContext } from '../shared';

+const FullWidthFlexGroup = styled(EuiFlexGroup)<{ $visible: boolean }>`


Would it make sense to make the min-height a prop of the pre-existing FullWidthFlexGroup instead of duplicating it?

estermv · 2022-02-21T17:45:17Z

@afgomez @fkanout I fixed the empty state when there are no alerts. There was an old check (not needed anymore) that was preventing the new table to render. Now the behavior is consistent with the Alerts page

estermv · 2022-02-22T09:17:01Z

@elasticmachine merge upstream

kibana-ci · 2022-02-22T12:42:16Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Security Solution Tests / Detections > Need Admin Callouts indicating an admin is needed to migrate the alert data set The users index_mapping_outdated is "null" and their admin callouts should not show up On Rule Details page "before each" hook for "We show 1 primary callout"
[job] [logs] Security Solution Tests / Detections > Need Admin Callouts indicating an admin is needed to migrate the alert data set The users index_mapping_outdated is "true" and their admin callouts should show up On Detections home page We show the need admin primary callout
[job] [logs] Security Solution Tests / Detections > Need Admin Callouts indicating an admin is needed to migrate the alert data set The users index_mapping_outdated is "true" and their admin callouts should show up On Rule Details page "before each" hook for "We show 1 primary callout"

Metrics [docs]

Async chunks

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

id	before	after	diff
`observability`	394.7KB	391.4KB	-3.3KB
`timelines`	226.0KB	226.1KB	+137.0B
total			-3.2KB

History

💔 Build #25379 failed 1a2209c
💔 Build #25372 failed 5378801
💔 Build #25250 failed ba36270
💛 Build #24688 was flaky 2e3ed54
💔 Build #24668 failed 98cd7e5

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

cc @estermv @mgiota

kibanamachine · 2022-02-25T10:16:18Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125337 or prevent reminders by adding the backport:skip label.

kibanamachine · 2022-02-28T10:26:17Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 125337 or prevent reminders by adding the backport:skip label.

* render alerts in overview page * pass routeParams * create useAlertIndexNames hook * remove unused file * use alertIndexNames hook in new overview page * remove unused stuff * fix failing tests * remove min-height from FullWidthFlexGroup in standalone t-grid * Remove old alerts section from overview and use same style as other sections * remove alertsStateContainer from overview page * revert changes in new overview page * Add refetch function to alerts table * Fix type * rename file * remove not needed check * fix types Co-authored-by: Ester Marti <ester.martivilaseca@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

mgiota added 5 commits February 10, 2022 23:09

render alerts in overview page

ef4fcf5

pass routeParams

bb7020f

create useAlertIndexNames hook

5efde9b

remove unused file

5c7f75e

use alertIndexNames hook in new overview page

4c65b10

mgiota added 3 commits February 11, 2022 09:11

remove unused stuff

8d62b70

fix failing tests

931d603

remove min-height from FullWidthFlexGroup in standalone t-grid

e8a1383

mgiota commented Feb 11, 2022

View reviewed changes

estermv added 3 commits February 14, 2022 16:08

Remove old alerts section from overview and use same style as other s…

cd3ba94

…ections

remove alertsStateContainer from overview page

99d4ede

revert changes in new overview page

3a675c7

estermv reviewed Feb 14, 2022

View reviewed changes

kibanamachine and others added 3 commits February 15, 2022 12:59

Merge branch 'main' into draft_alerts_in_overview_page

7500727

Add refetch function to alerts table

2c96cdb

Fix type

44fd379

kibanamachine and others added 2 commits February 16, 2022 10:07

Merge branch 'main' into draft_alerts_in_overview_page

98cd7e5

rename file

2e3ed54

mgiota marked this pull request as ready for review February 17, 2022 09:43

mgiota requested a review from a team as a code owner February 17, 2022 09:43

mgiota requested a review from a team February 17, 2022 09:43

mgiota requested a review from a team as a code owner February 17, 2022 09:43

mgiota assigned mgiota and estermv Feb 17, 2022

mgiota added the Team:Unified observability label Feb 17, 2022

mgiota added Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.2.0 labels Feb 17, 2022

mgiota changed the title ~~Draft alerts in overview page~~ Alerts in overview page Feb 17, 2022

mgiota added the release_note:enhancement label Feb 17, 2022

estermv mentioned this pull request Feb 17, 2022

[Unified Observability] Alerts table redesign for the Overview page #124370

Closed

3 tasks

kqualters-elastic approved these changes Feb 17, 2022

View reviewed changes

Merge branch 'main' into draft_alerts_in_overview_page

ba36270

fkanout reviewed Feb 21, 2022

View reviewed changes

afgomez suggested changes Feb 21, 2022

View reviewed changes

remove not needed check

5378801

fix types

1a2209c

Merge branch 'main' into draft_alerts_in_overview_page

d899c7c

mgiota requested a review from afgomez February 22, 2022 14:22

afgomez approved these changes Feb 22, 2022

View reviewed changes

fkanout approved these changes Feb 23, 2022

View reviewed changes

mgiota merged commit 3195a54 into elastic:main Feb 23, 2022

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 25, 2022

estermv added the backport:skip This PR does not require backporting label Feb 28, 2022

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 28, 2022

mgiota mentioned this pull request Mar 14, 2022

[Actionable Observability] Alerts table looks broken #125191

Closed

Conversation

mgiota commented Feb 11, 2022 • edited by estermv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgiota commented Feb 11, 2022

Uh oh!

mgiota Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estermv commented Feb 14, 2022

Uh oh!

estermv commented Feb 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgiota Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgiota commented Feb 14, 2022

Uh oh!

estermv commented Feb 15, 2022

Uh oh!

estermv commented Feb 16, 2022

Uh oh!

elasticmachine commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estermv commented Feb 21, 2022

Uh oh!

fkanout commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afgomez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estermv commented Feb 21, 2022

Uh oh!

estermv commented Feb 22, 2022

Uh oh!

kibana-ci commented Feb 22, 2022

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

Async chunks

History

Uh oh!

kibanamachine commented Feb 25, 2022

Uh oh!

kibanamachine commented Feb 28, 2022

Uh oh!

Reviewers

Assignees

Labels

mgiota commented Feb 11, 2022 •

edited by estermv

Loading

mgiota Feb 11, 2022 •

edited

Loading

mgiota Feb 14, 2022 •

edited

Loading

fkanout commented Feb 21, 2022 •

edited

Loading