[RAC] EuiDataGrid pagination #109269

machadoum · 2021-08-19T15:23:25Z

Summary

Remove T-grid Footer
Use DataGrid pagination
Update everywhere using rowIndex to use rowIndex % itemsPerPage because we only load the current page in memory
Improve the loading state during query time
DataGrid pagination makes sure that we display the grid with the proper height.

Add a full table screenshot

* It also improves the Gtid loading state * DataGrid pagination makes sure that we display the grid with the proper height.

machadoum · 2021-08-24T13:06:25Z

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

@@ -311,81 +301,56 @@ const TGridIntegratedComponent: React.FC<TGridIntegratedProps> = ({
                )}
              </UpdatedFlexGroup>

-              <FullWidthFlexGroup
-                $visible={!graphEventId && graphOverlay == null}


How to test graphOverlay and graphEventId? It was always undefined when I implemented this feature. But I see that it was recently introduced on master.

@stephmilovic is that correct?

graphOverlay you can do analyze event from alerts, not sure the other?

this is broken now, need to hide tgrid when analyzer is opened

@semd we need to add this back, can you do your magic here?

machadoum · 2021-08-24T13:07:25Z

x-pack/plugins/security_solution/public/common/lib/cell_actions/default_cell_actions.tsx

+ * Be careful with array out of bounds. `pageRowIndex` can be bigger or equal to `data.length`
+ *  in the scenario where the user changes the event status (Open, Acknowledged, Closed).
+ */
+export const getPageRowIndex = (rowIndex: number, itemsPerPage: number) => rowIndex % itemsPerPage;


This function is defined twice (timelines and security solutions). Where can we move it to reuse on both places?

since this is already importing from the timeslines plugin, i think we can import it from there

okay I exported it from timeline public, it is imported by both o11y and securitySol.

machadoum · 2021-08-24T13:10:24Z

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

@@ -86,16 +80,6 @@ const EventsContainerLoading = styled.div.attrs(({ className = '' }) => ({
  flex-direction: column;
 `;

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


It was useless. should we reintroduce it because $visible could be defined?

HUGE HACK!!! DataGrtid height isn't properly calculated when the grid has horizontal scroll. elastic/eui#5030 In order to get around this bug we are calculating `DataGrid` height here and setting it as a prop. Please revert this commit and allow DataGrid to calculate its height when the bug is fixed.

elasticmachine · 2021-08-24T15:24:15Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

elasticmachine · 2021-08-24T15:24:15Z

Pinging @elastic/security-solution (Team: SecuritySolution)

machadoum · 2021-08-24T16:17:30Z

I've found an unrelated issue with the checkbox while testing this branch:

When clicking on one checkbox, two are selected:

When selecting all checkboxes, the total is wrong:

Seams like two events have the same id. Is it a bug or some problem with the testing data?

XavierM · 2021-08-25T16:47:48Z

@semd and @spong and I investigate your first problem, we know why. It is because now we are using an alias .alerts-securitySolution.alerts for .siem-signals-{spaceId} and every developer in kibana-siem-dev can creates their own signals index but since we are using an alias to groupe all these indexes in the cluster, it is possible to have duplicate id from siem-signals-xavier-default-00001 and siem-signals-pablo-default-00001.
We do not think that this behavior can happen in production because we do not allow user to modify the signal index name in production.

mdefazio

I didn't get a chance to get this running locally to test, but I've see you run through and it LGTM. My only question was whether we should default to more alerts by default in the view.

mdefazio · 2021-08-25T18:04:33Z

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

    itemsPerPageOptions: [],
    loadingEventIds: [],
    loadPage: jest.fn(),
-    querySize: 25,
+    pageSize: 25,


Is this the initial amount shown in the table? Would it make sense to have this be at least 50?

Hi @mdefazio , Yes the default size is 50 items in security solutions and observability. This is just a test, we are good 👍

stephmilovic · 2021-08-25T19:27:31Z

x-pack/plugins/timelines/public/components/t_grid/body/height_hack.ts

+const TIME_INTERVAL = 50;
+
+/**
+ * You are probably asking yourself "Why 3?". But that is the wrong mindset. You should be asking yourself "why not 3?".


spong · 2021-08-25T20:01:10Z

We do not think that this behavior can happen in production because we do not allow user to modify the signal index name in production.

Note, this technically is possible via the xpack.securitySolution.signalsIndex kibana.yml setting, however this isn't an officially supported configuration, isn't in our documentation, and has only come up in one community issue: https://discuss.elastic.co/t/detection-result-in-new-index/270347, so @XavierM's point stands.

stephmilovic · 2021-08-26T13:30:07Z

small bug here that the right arrow is enabled on the last page of results

stephmilovic

Looks like that last bug is Eui related. Code looks good and I didn't surface any other issues testing locally. LGTM thank you @semd 🐎 🏁 🏎️

semd · 2021-08-26T16:04:03Z

Looks like that last bug is Eui related. Code looks good and I didn't surface any other issues testing locally. LGTM thank you @semd 🐎 🏁 🏎️

Thanks @stephmilovic for testing it. And @machadoum! he did the hardest part 📐

* Update T-Grid to use DataGrid pagination * It also improves the Gtid loading state * DataGrid pagination makes sure that we display the grid with the proper height. * Add DataGrid height hack to t-grid HUGE HACK!!! DataGrtid height isn't properly calculated when the grid has horizontal scroll. elastic/eui#5030 In order to get around this bug we are calculating `DataGrid` height here and setting it as a prop. Please revert this commit and allow DataGrid to calculate its height when the bug is fixed. * Apply DataGrid laoding and pagination changes to observability * Fix cypress tests * Fix t-grid page render bug on Observability * some pagination fixes * hide table when analyzer active * isolate exported function Co-authored-by: semd <sergi.massaneda@elastic.co>

kibanamachine · 2021-08-26T18:44:59Z

💔 Backport failed

Status	Branch	Result
✅	7.15
❌	7.x	Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 109269

* Update T-Grid to use DataGrid pagination * It also improves the Gtid loading state * DataGrid pagination makes sure that we display the grid with the proper height. * Add DataGrid height hack to t-grid HUGE HACK!!! DataGrtid height isn't properly calculated when the grid has horizontal scroll. elastic/eui#5030 In order to get around this bug we are calculating `DataGrid` height here and setting it as a prop. Please revert this commit and allow DataGrid to calculate its height when the bug is fixed. * Apply DataGrid laoding and pagination changes to observability * Fix cypress tests * Fix t-grid page render bug on Observability * some pagination fixes * hide table when analyzer active * isolate exported function Co-authored-by: semd <sergi.massaneda@elastic.co> Co-authored-by: Pablo Machado <pablo.nevesmachado@elastic.co> Co-authored-by: semd <sergi.massaneda@elastic.co>

* Update T-Grid to use DataGrid pagination * It also improves the Gtid loading state * DataGrid pagination makes sure that we display the grid with the proper height. * Add DataGrid height hack to t-grid HUGE HACK!!! DataGrtid height isn't properly calculated when the grid has horizontal scroll. elastic/eui#5030 In order to get around this bug we are calculating `DataGrid` height here and setting it as a prop. Please revert this commit and allow DataGrid to calculate its height when the bug is fixed. * Apply DataGrid laoding and pagination changes to observability * Fix cypress tests * Fix t-grid page render bug on Observability * some pagination fixes * hide table when analyzer active * isolate exported function Co-authored-by: semd <sergi.massaneda@elastic.co> # Conflicts: # x-pack/plugins/timelines/public/components/t_grid/body/index.tsx

kibanamachine · 2021-08-30T09:35:19Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: 372abfe
Storybooks Preview
Flaky suites:
- xpack-accessibility

Metrics [docs]

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
`timelines`	843	845	+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`	554.2KB	554.3KB	+141.0B
`securitySolution`	6.5MB	6.5MB	+885.0B
`timelines`	426.8KB	419.2KB	-7.6KB
total			-6.6KB

Page load bundle

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

id	before	after	diff
`timelines`	307.4KB	307.9KB	+496.0B

Unknown metric groups

API count

id	before	after	diff
`timelines`	963	966	+3

History

💛 Build #148993 was flaky abd30d3
💚 Build #149004 succeeded 372abfe
💚 Build #148880 succeeded 55d6707
💔 Build #148854 failed 567afb4
💚 Build #148645 succeeded dd2cc6f

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

cc @machadoum

* Update T-Grid to use DataGrid pagination * It also improves the Gtid loading state * DataGrid pagination makes sure that we display the grid with the proper height. * Add DataGrid height hack to t-grid HUGE HACK!!! DataGrtid height isn't properly calculated when the grid has horizontal scroll. elastic/eui#5030 In order to get around this bug we are calculating `DataGrid` height here and setting it as a prop. Please revert this commit and allow DataGrid to calculate its height when the bug is fixed. * Apply DataGrid laoding and pagination changes to observability * Fix cypress tests * Fix t-grid page render bug on Observability * some pagination fixes * hide table when analyzer active * isolate exported function Co-authored-by: semd <sergi.massaneda@elastic.co>

* Update T-Grid to use DataGrid pagination * It also improves the Gtid loading state * DataGrid pagination makes sure that we display the grid with the proper height. * Add DataGrid height hack to t-grid HUGE HACK!!! DataGrtid height isn't properly calculated when the grid has horizontal scroll. elastic/eui#5030 In order to get around this bug we are calculating `DataGrid` height here and setting it as a prop. Please revert this commit and allow DataGrid to calculate its height when the bug is fixed. * Apply DataGrid laoding and pagination changes to observability * Fix cypress tests * Fix t-grid page render bug on Observability * some pagination fixes * hide table when analyzer active * isolate exported function Co-authored-by: semd <sergi.massaneda@elastic.co> Co-authored-by: Pablo Machado <pablo.nevesmachado@elastic.co>

machadoum requested a review from XavierM August 19, 2021 15:23

machadoum self-assigned this Aug 19, 2021

XavierM mentioned this pull request Aug 19, 2021

Alert Table loading state for change and switching pagination #108432

Closed

machadoum force-pushed the siem-alerts-t-grid-pagination branch 4 times, most recently from d7222cc to c38b421 Compare August 24, 2021 11:47

Update T-Grid to use DataGrid pagination

d66f5c2

* It also improves the Gtid loading state * DataGrid pagination makes sure that we display the grid with the proper height.

machadoum force-pushed the siem-alerts-t-grid-pagination branch from c38b421 to f8aa22e Compare August 24, 2021 12:49

machadoum commented Aug 24, 2021

View reviewed changes

machadoum added 2 commits August 24, 2021 16:51

Apply DataGrid laoding and pagination changes to observability

091b592

machadoum force-pushed the siem-alerts-t-grid-pagination branch from f8aa22e to 091b592 Compare August 24, 2021 14:51

machadoum marked this pull request as ready for review August 24, 2021 15:22

machadoum requested review from a team as code owners August 24, 2021 15:22

machadoum added the bug Fixes for quality problems that affect the customer experience label Aug 24, 2021

Fix cypress tests

06d112c

XavierM added the v7.16.0 label Aug 24, 2021

semd changed the title ~~POC for T-Grid pagination~~ [RAC] EuiDataGrid pagination Aug 25, 2021

mdefazio approved these changes Aug 25, 2021

View reviewed changes

stephmilovic reviewed Aug 25, 2021

View reviewed changes

semd added 2 commits August 26, 2021 12:41

hide table when analyzer active

567afb4

isolate exported function

55d6707

snide mentioned this pull request Aug 26, 2021

[Security Solution] [RAC] Possible to break the grid when switching between tabs in RAC #110267

Closed

stephmilovic approved these changes Aug 26, 2021

View reviewed changes

conflicts with event context solved

abd30d3

expand fix conflicts solved

372abfe

semd enabled auto-merge (squash) August 26, 2021 16:21

semd merged commit 3854d3a into elastic:master Aug 26, 2021

kibanamachine mentioned this pull request Aug 26, 2021

[7.15] [RAC] EuiDataGrid pagination (#109269) #110316

Merged

This was referenced Aug 30, 2021

[RAC][Observability]Display Alerts table with proper height #110077

Closed

[RAC] 7.15 Observability Checklist #107933

Closed

semd mentioned this pull request Aug 30, 2021

[7.x] [RAC] EuiDataGrid pagination (#109269) #110442

Closed

semd mentioned this pull request Aug 30, 2021

[7.x] [RAC] EuiDataGrid pagination (#109269) #110471

Merged

machadoum mentioned this pull request Sep 1, 2021

[Security Solution] Alert Table collide when there are less alerts in open state on alert table. #109421

Closed

afgomez mentioned this pull request Sep 1, 2021

[RAC][Observability] Loading indicator shouldn't appear at the bottom #109375

Closed

janmonschke mentioned this pull request Jan 10, 2022

Upgrade EUI to v44.0.0 #122386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RAC] EuiDataGrid pagination #109269

[RAC] EuiDataGrid pagination #109269

machadoum commented Aug 19, 2021 •

edited by mgiota

Loading

machadoum Aug 24, 2021

XavierM Aug 25, 2021

stephmilovic Aug 25, 2021

stephmilovic Aug 25, 2021

XavierM Aug 26, 2021

machadoum Aug 24, 2021

stephmilovic Aug 25, 2021 •

edited

Loading

semd Aug 26, 2021

machadoum Aug 24, 2021 •

edited

Loading

elasticmachine commented Aug 24, 2021

elasticmachine commented Aug 24, 2021

machadoum commented Aug 24, 2021

XavierM commented Aug 25, 2021

mdefazio left a comment

mdefazio Aug 25, 2021

semd Aug 26, 2021

stephmilovic Aug 25, 2021

spong commented Aug 25, 2021

stephmilovic commented Aug 26, 2021

stephmilovic left a comment

semd commented Aug 26, 2021

kibanamachine commented Aug 26, 2021

kibanamachine commented Aug 30, 2021

API count

[RAC] EuiDataGrid pagination #109269

[RAC] EuiDataGrid pagination #109269

Conversation

machadoum commented Aug 19, 2021 • edited by mgiota Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephmilovic Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machadoum Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Aug 24, 2021

elasticmachine commented Aug 24, 2021

machadoum commented Aug 24, 2021

XavierM commented Aug 25, 2021

mdefazio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spong commented Aug 25, 2021

stephmilovic commented Aug 26, 2021

stephmilovic left a comment

Choose a reason for hiding this comment

semd commented Aug 26, 2021

kibanamachine commented Aug 26, 2021

💔 Backport failed

kibanamachine commented Aug 30, 2021

💛 Build succeeded, but was flaky

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

machadoum commented Aug 19, 2021 •

edited by mgiota

Loading

stephmilovic Aug 25, 2021 •

edited

Loading

machadoum Aug 24, 2021 •

edited

Loading