[ResponseOps] Execution log - data grid component, date picker, and status filter by JiaweiWu · Pull Request #128183 · elastic/kibana

JiaweiWu · 2022-03-21T17:00:47Z

Summary

Resolves the following issues:
#126841
#126842

Implements the execution log list component inside the rules summary page. Uses the new aggregation API and supports sorting/filtering/pagination via the data grid component.

We're using the super date picker to allow filtering on dates.

Currently the filtering is only on the event outcome status (success, failure, or 'unknown`). In the future we will implement a KQL query bar to support filtering on more fields.

Sorting is limited to the following fields:

timestamp
execution_duration
total_search_duration
es_search_duration
schedule_delay
num_triggered_actions

This PR has a dependency on #128179, so it contains changes from there as well.

elasticmachine · 2022-03-21T17:00:50Z

Pinging @elastic/response-ops (Team:ResponseOps)

ymao1

LGTM! Left some minor comments and questions but verified that the exec log table shows up on the rule details view :)

ymao1 · 2022-03-24T14:19:08Z

x-pack/plugins/triggers_actions_ui/public/application/sections/rule_details/components/rule.tsx

    .sort((leftAlert, rightAlert) => leftAlert.sortPriority - rightAlert.sortPriority);

-  const pageOfAlerts = getPage(alerts, pagination);
+  const pageOfAlerts = getPage<AlertListItem>(alerts, pagination);


Is there a reason to keep the alert table pagination in this component instead of moving it into <RuleAlertList?

Good point, I'll move it to the alert list component

ymao1 · 2022-03-24T14:41:49Z

x-pack/plugins/triggers_actions_ui/public/application/sections/rule_details/components/rule.tsx

+  ];
+
+  const renderTabs = () => {
+    const isEnabled = getIsExperimentalFeatureEnabled('rulesListDatagrid');


Should this be rulesDetailLogs instead of rulesListDatagrid?

ymao1 · 2022-03-24T14:45:44Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+  return date;
+};
+
+const SORTABLE_COLUMNS = [


We have this const and also SortFields in x-pack/plugins/triggers_actions_ui/public/application/lib/rule_api/load_execution_log_aggregations.ts. Can we collapse to just one definition to make sure we're keeping them in sync?

Something like

const SortableColumns =[ 'timestamp', 'execution_duration', 'total_search_duration', 'es_search_duration', 'schedule_delay', 'num_triggered_actions', ] as const; type SortFields = typeof SortableColumns[number];

ymao1 · 2022-03-24T14:54:09Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+  });
+
+  // Date related states
+  const [isLoading, setIsLoading] = useState<boolean>(false);


Is there some sort of loading indicator for the EuiDataGrid that we can set? I added a delay to the API response for testing and I just see a table with an empty header while waiting for the response. Would be nice to have some sort of loading indicator.

I didn't find one inherently while working on the alerts table, but I found that adding a <EuiProgress> on the top of the grid worked well. See what I did here

It also made it much easier to functionally test, as I could properly wait for server requests to finish

Yea right now the only indication is the refresh button, but yes I can add what chris did to the execution log list

ymao1 · 2022-03-24T15:30:52Z

I just remembered we were planning to show something similar to Discover if the total returned in the API response was greater than 1000. I did not see that included in this PR. Will that be a followup?

chrisronline

Looking great so far! Made a first pass and had some questions/comments

chrisronline · 2022-03-24T15:24:19Z

x-pack/plugins/triggers_actions_ui/public/application/sections/rule_details/components/rule.tsx

+    {
+      id: ALERT_LIST_TAB,
+      name: i18n.translate('xpack.triggersActionsUI.sections.ruleDetails.rule.alertsTabText', {
+        defaultMessage: 'Alerts',


Is this an attempt to normalize the terminology here? We aren't technically showing alerts, right?

So this tab is where the previous alert list in the rule summary would go, it technically would be alerts here

Yea I'm just saying we aren't querying alerts-as-data indices here, but rather getting this information from the event log I think?

I think you're right, these alerts come from the rule summary, which queries event logs

Yea I'm not sure if it's a big deal or not, cc @XavierM

chrisronline · 2022-03-24T15:25:17Z

x-pack/plugins/triggers_actions_ui/public/application/sections/rule_details/components/rule.tsx

+  ];
+
+  const renderTabs = () => {
+    const isEnabled = getIsExperimentalFeatureEnabled('rulesListDatagrid');


Did you run into this bug while developing this? Or maybe not because it's on by default?

It's on by default, I recently noticed the issue when poking at the config values for integration testing

I should merge the fix by EOD so don't worry about fixing it yourself

chrisronline · 2022-03-24T15:27:33Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+const columns = [
+  {
+    id: 'id',
+    displayAsText: i18n.translate(


Can we use the initialWidth property to make the default columns more user friendly? We can easily shrink the Status column while making the Message column wider

chrisronline · 2022-03-24T15:29:16Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+  });
+
+  // Date related states
+  const [isLoading, setIsLoading] = useState<boolean>(false);


I didn't find one inherently while working on the alerts table, but I found that adding a <EuiProgress> on the top of the grid worked well. See what I did here

It also made it much easier to functionally test, as I could properly wait for server requests to finish

chrisronline · 2022-03-24T15:30:19Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+  // Main cell renderer, renders durations, statuses, etc.
+  const renderCell = ({ rowIndex, columnId }: EuiDataGridCellValueElementProps) => {
+    const { pageIndex, pageSize } = pagination;
+    const pagedRowIndex = rowIndex - pageIndex * pageSize;


I found a visibleRowIndex property available to use instead of needing to do this.

unfortunately I think theres a bug with this. visibleRowIndex doesn't seem to exist when its in the "popover" mode. It becomes undefined :(

chrisronline · 2022-03-24T15:55:39Z

...ui/public/application/sections/rule_details/components/rule_event_log_list_cell_renderer.tsx

+    return null;
+  }
+
+  if (columnId === 'status') {


Is there a way to use an enum or something here? Seems brittle that if the column id changes, this will silently fail

.../triggers_actions_ui/public/application/sections/rule_details/components/rule_alert_list.tsx

chrisronline · 2022-03-24T16:03:43Z

...ugins/triggers_actions_ui/public/application/lib/rule_api/load_execution_log_aggregations.ts

+  | 'schedule_delay'
+  | 'num_triggered_actions';
+
+export type SortOrder = 'asc' | 'desc';


We shouldn't need to explicitly define this - maybe we can use estypes.SortOrder?

chrisronline · 2022-03-24T16:05:47Z

...ugins/triggers_actions_ui/public/application/lib/rule_api/load_execution_log_aggregations.ts

+        date_end: dateEnd,
+        filter: getFilter(filter),
+        per_page: perPage,
+        page: page + 1,


Probably worth adding a comment explaining why we have to add 1 - honestly, I don't know why the API works this way when it just subtracts 1 from the page variable later. I'd add something here to remind us about this.

x-pack/plugins/triggers_actions_ui/public/application/lib/monitoring_utils.ts

JiaweiWu · 2022-03-24T16:46:28Z

I just remembered we were planning to show something similar to Discover if the total returned in the API response was greater than 1000. I did not see that included in this PR. Will that be a followup?

@ymao1 I wasn't aware of this, do you have more context for what it looks like or where I can find an example of its usage? I don't mind adding it with this PR

ymao1 · 2022-03-24T16:49:23Z

@ymao1 I wasn't aware of this, do you have more context for what it looks like or where I can find an example of its usage? I don't mind adding it with this PR

I thought it was in meta issue but it turns out it's in a discussion thread on my API PR. Sorry! Here is the link: #127339 (comment)

mdefazio

Awesome to see this in place! Some comments on the UI:

Move controls to the left: I know this will have a search bar in the future, but in the meantime, lets' just keep these left-aligned

No flex-grow on the inputs: Again, just to clean things up some more until the search bar is in.

Styling of EuiSuperDatePicker: Let's make the width set to auto and then change the refresh button to only be an icon and remove the fill on it as well.

Screenshot of these changes:

Duration should include milliseconds: I know we don't show milliseconds on the rule list page, but for these logs I feel it makes sense, particularly since its right next to a timestamp with milliseconds. Perhaps theres another opinion about this, but it felt odd to show timestamp with milliseconds and not the duration.

Right align columns with numbers: Columns where we are comparing numbers across rows, these should be right-aligned.

Filter popover Looks like there's extra padding getting applied to the filter popover. Not sure if the EuiSelectable is being used here. While the docs say a 'long list' is best to use EuiSelectable, I think it's worth considering so we can have this appear consistent next to other upcoming filters that may have longer lists (though we can omit the search for this filter group). The issue currently is the padding, but something to consider.

pmuellr · 2022-03-24T20:22:25Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+
+const RULE_EVENT_LOG_LIST_STORAGE_KEY = 'xpack.triggersActionsUI.ruleEventLogList.initialColumns';
+
+export const DEFAULT_INITIAL_VISIBLE_COLUMNS = [


This looks like a good list of columns to start with! Once this is live, we'll probably have a better sense if there's more we need.

JiaweiWu · 2022-03-24T21:34:32Z

@elasticmachine merge upstream

JiaweiWu · 2022-03-24T21:42:02Z

Right align columns with numbers: Columns where we are comparing numbers across rows, these should be right-aligned.

@mdefazio So it seems like the data grid should automatically format numeric values to be right aligned, I can't seem to achieve this functionality despite boiling it down to the simplest case possible (renderCellValue returns a static number). If you don't think this is a blocker I can open a bug after this is merge and dig deeper.

Filter popover Looks like there's extra padding getting applied to the filter popover

I'm currently using a div that wraps multiple EuiFilterSelectItems. The goal was to match the filter that exists in the rules page, the padding seems to be there by default, making space for the checkmark

JiaweiWu · 2022-03-26T06:07:33Z

@elasticmachine merge upstream

JiaweiWu · 2022-03-27T23:38:20Z

@elasticmachine merge upstream

jbudz

limits.yml LGTM

chrisronline

Looking really good! I commented about a few visual things but I think what we have is great!

chrisronline · 2022-03-28T17:34:29Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+        defaultMessage: 'Timestamp',
+      }
+    ),
+    isSortable: getIsColumnSortable('timestamp'),


Try adding initialWidth: 250,

chrisronline · 2022-03-28T17:34:40Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+        defaultMessage: 'Duration',
+      }
+    ),
+    isSortable: getIsColumnSortable('execution_duration'),


Try adding initialWidth: 100,

chrisronline · 2022-03-28T17:34:54Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+      }
+    ),
+    isSortable: getIsColumnSortable('message'),
+    initialWidth: 400,


Try removing this so the message takes up the rest of the available space

chrisronline · 2022-03-28T17:44:05Z

...ggers_actions_ui/public/application/sections/rule_details/components/rule_event_log_list.tsx

+        <EuiFlexItem grow={false}>
+          <RuleEventLogListStatusFilter selectedOptions={filter} onChange={onFilterChange} />
+        </EuiFlexItem>
+        <EuiFlexItem grow={false}>


Maybe grow={true} so it expands?

Addressed Michael's comments but he is OOO

kibana-ci · 2022-03-28T20:07:48Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`alerting`	25	26	+1
`triggersActionsUi`	338	345	+7
total			+8

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
`alerting`	294	315	+21

Async chunks

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

id	before	after	diff
`triggersActionsUi`	674.2KB	685.2KB	+11.0KB

Page load bundle

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

id	before	after	diff
`alerting`	38.2KB	38.4KB	+183.0B
`triggersActionsUi`	97.2KB	98.1KB	+1001.0B
total			+1.2KB

Unknown metric groups

API count

id	before	after	diff
`alerting`	302	323	+21

async chunk count

id	before	after	diff
`triggersActionsUi`	69	71	+2

ESLint disabled in files

id	before	after	diff
`triggersActionsUi`	5	6	+1

ESLint disabled line counts

id	before	after	diff
`triggersActionsUi`	155	158	+3

Total ESLint disabled count

id	before	after	diff
`triggersActionsUi`	160	164	+4

History

💚 Build #34070 succeeded dac8f66
💔 Build #33827 failed 20d5e8b
💔 Build #33823 failed 6c022d7
💔 Build #33800 failed 96bc5e9
💔 Build #33799 failed 726d79a
💔 Build #33715 failed 0cf9a23

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

JiaweiWu added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.2.0 labels Mar 21, 2022

JiaweiWu requested a review from a team as a code owner March 21, 2022 17:00

JiaweiWu changed the title ~~Execution log data grid~~ Execution log - data grid component and date picker Mar 21, 2022

JiaweiWu marked this pull request as draft March 21, 2022 17:01

This was linked to issues Mar 21, 2022

[RAM] Create data grid component to display logs in rule details page #126841

Closed

[RAM] Add date picker and refresh button to query logs in rule details page #126842

Closed

JiaweiWu added 4 commits March 22, 2022 19:50

Event log and alerts tab in rule summary

1cb6695

Fix tests

a4f80e7

Event log implementation in rule summary page

b9b0af7

Unit testing

344a0fa

JiaweiWu force-pushed the issue-126841-event-log-data-grid branch from 901bde0 to 344a0fa Compare March 23, 2022 20:14

JiaweiWu added 5 commits March 23, 2022 13:16

run lint

458b9d7

rule event log unit tests

ccc13d3

Load execution API unit test

d16d09f

With bulk rule api test update

b3674ef

Fix lint

598d49e

JiaweiWu changed the title ~~Execution log - data grid component and date picker~~ [ResponseOps] Execution log - data grid component, date picker, and status filter Mar 23, 2022

JiaweiWu marked this pull request as ready for review March 23, 2022 22:12

JiaweiWu requested review from XavierM, chrisronline, pmuellr and ymao1 March 23, 2022 22:13

ymao1 approved these changes Mar 24, 2022

View reviewed changes

chrisronline reviewed Mar 24, 2022

View reviewed changes

mdefazio previously requested changes Mar 24, 2022

View reviewed changes

pmuellr reviewed Mar 24, 2022

View reviewed changes

JiaweiWu added 2 commits March 24, 2022 14:24

Address comments

7a318ca

Fix feature flag test

9b5f0a2

Merge branch 'main' into issue-126841-event-log-data-grid

983c9be

JiaweiWu requested review from chrisronline and mdefazio March 24, 2022 21:45

JiaweiWu added 2 commits March 24, 2022 14:52

Fix import type linting

0cf9a23

Integration test and fixed lint

726d79a

kibanamachine and others added 2 commits March 26, 2022 02:07

Merge branch 'main' into issue-126841-event-log-data-grid

96bc5e9

Lazy load execution log list

6c022d7

kibanamachine and others added 2 commits March 27, 2022 19:38

Merge branch 'main' into issue-126841-event-log-data-grid

20d5e8b

Bump up triggers_actions_ui limits.yml size to 100kbs

dac8f66

JiaweiWu requested a review from a team as a code owner March 28, 2022 14:58

jbudz approved these changes Mar 28, 2022

View reviewed changes

chrisronline approved these changes Mar 28, 2022

View reviewed changes

JiaweiWu added 2 commits March 28, 2022 11:35

Fix merge conflicts

7ba91c3

Address comments

1c26f51

JiaweiWu merged commit 4235f81 into elastic:main Mar 28, 2022

JiaweiWu linked an issue Mar 28, 2022 that may be closed by this pull request

[RAM] Create the "Alerts" and "Logs" tabs in the rules details page #126845

Open


		const RULE_EVENT_LOG_LIST_STORAGE_KEY = 'xpack.triggersActionsUI.ruleEventLogList.initialColumns';

		export const DEFAULT_INITIAL_VISIBLE_COLUMNS = [

Conversation

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

Uh oh!

Summary

Uh oh!

elasticmachine commented Mar 21, 2022

Uh oh!

ymao1 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!

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!

ymao1 commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisronline 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!

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!

JiaweiWu Mar 24, 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JiaweiWu commented Mar 24, 2022

Uh oh!

ymao1 commented Mar 24, 2022

Uh oh!

mdefazio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JiaweiWu commented Mar 24, 2022

Uh oh!

JiaweiWu commented Mar 24, 2022

Uh oh!

JiaweiWu commented Mar 26, 2022

JiaweiWu commented Mar 21, 2022 •

edited

Loading

ymao1 commented Mar 24, 2022 •

edited

Loading

JiaweiWu Mar 24, 2022 •

edited

Loading