Skip to content
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

[RAC] [TGrid] Field browser implemented in EuiDataGrid toolbar #105207

Merged
merged 25 commits into from
Jul 20, 2021

Conversation

semd
Copy link
Contributor

@semd semd commented Jul 12, 2021

Summary

Migrate header columns to use EuiDataGrid and implement custom columns utility (aka Field Browser).

The code has been moved from the Security Solutions plugin, to the tGrid in timelines plugin, cleaning all the Drag & Drop functionalities.

In the Observability Alerts tGrid (still under experimental flag) it is not used yet waiting for #106199 to be merged.

Security Solutions now uses the timeline plugin Field Browser for rendering the functionality, using a lazy load.
It can be tested in Alerts table, Hosts/Events tab table, and Timelines page table:

Gravacio.de.pantalla.2021-07-20.a.les.11.13.50.mov

Checklist

Delete any items that are not applicable to this PR.

@semd semd added v8.0.0 Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.14.0 v7.15.0 labels Jul 12, 2021
@semd semd changed the title [Security][tGrid] Custom columns header to EuiDataGrid [TGrid] Field browser implemented in EuiDataGrid toolbar Jul 12, 2021
@semd semd added the release_note:skip Skip the PR/issue when compiling release notes label Jul 14, 2021
@semd semd added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 14, 2021
@semd semd marked this pull request as ready for review July 14, 2021 16:02
@semd semd requested a review from a team as a code owner July 14, 2021 16:02
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@semd
Copy link
Contributor Author

semd commented Jul 14, 2021

@elasticmachine merge upstream

@snide snide requested a review from a team July 15, 2021 07:05
@semd semd requested a review from a team as a code owner July 16, 2021 17:13
@semd
Copy link
Contributor Author

semd commented Jul 19, 2021

@elastic/kibana-operations I had to increase the limit of the timelines plugin file size. This is because I moved a component source code from securitySolution plugin to timelines, the numbers of the two plugins:

master:

  • timelines.plugin.js: 231 K
  • timelines all files: 707 K
  • securitySolution.plugin.js: 202 K
  • securitySolution all files: 9193 K

timeline-euidatagrid branch:

  • timelines.plugin.js: 321 K (+90 K)
  • timelines all files: 792 K (+85 K)
  • securitySolution.plugin.js: 202 K (==)
  • securitySolution all files: 9129 K (-64 K)

The total net size increase is around 21 K, which is caused by a couple of files that I had to duplicate, but they will be removed in following PRs.
This is part of the migration of the timelines tGrid table to use EuiDataGrid, there will be a small size overhead while we are developing, but once it's done we will be able to decrease the limit to a comfortable value again, as explained in #106199

@snide
Copy link
Contributor

snide commented Jul 19, 2021

Can design get some instructions on how to check this PR? The screen shows obs, so I'm guessing I need to turn on a feature flag somewhere?

Couple quick notes just from browsing the code / looking at the screens:

  • Lots of hard coded pixel values in here. Most of them can likely be moved to EUI tokens.
  • The added button is unlike the rest of the default DG ones (it's missing a label). Also, I assume you don't want both the EUI column manager, and this field browser both showing? They duplicate functionality a bit.
  • I know a lot of this came from the previous incarnation, but there's some fairly unique patterns being handled in this component. It's treated like a modal (fixed position) but displays like a popover. Suggestion would be to use one or the other.

@semd
Copy link
Contributor Author

semd commented Jul 20, 2021

Hello @snide,

I updated the description explaining how to test it in Security Solutions. In Observalibity it is not possible until this pr #106199 is merged.

Regarding the notes:

We can talk about a redesign to make it more aligned with the rest of popovers/modals, but right now it is just a port from one plugin to the other, I tried to keep the same UI as there was no design enhancement planned.
I removed all the D&D functionality and cleaned up the code a little bit. I deleted some hardcoded pixels indeed, but some others remained. I will check EUI tokens as I am not very familiar with them.

About the EUI column manager. It will be used only for column order, which is the only action that Field Browser does not implement right now. The column manager "hide/show" action is disabled by config, so there's no functionality overlap using both of them.

Thanks for the observations.

@@ -257,6 +257,7 @@ export const defaultTimelineToTimelineModel = (
const timelineEntries = {
...timeline,
columns: timeline.columns != null ? timeline.columns.map(setTimelineColumn) : defaultHeaders,
defaultColumns: defaultHeaders,
Copy link
Contributor Author

@semd semd Jul 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed in order to reset the columns in the timeline page property, if not defined the reset action updates to empty columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also resolves @andrew-goldstein comment #105207 (review)

@@ -23,17 +23,19 @@ export const getColumnHeaders = (
headers: ColumnHeaderOptions[],
browserFields: BrowserFields
): ColumnHeaderOptions[] => {
return headers.map((header) => {
const splitHeader = header.id.split('.'); // source.geo.city_name -> [source, geo, city_name]
return headers
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fix for this error:
Captura de Pantalla 2021-07-20 a les 11 20 41
to reproduce:

  • Open any saved timeline
  • Open field browser
  • Click reset fields

I replicated the same fix there is x-pack/plugins/timelines/public/components/t_grid/body/column_headers/helpers.ts

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2208 2198 -10
timelines 223 246 +23
total +13

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 759 763 +4

Async chunks

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

id before after diff
securitySolution 6.3MB 6.2MB -50.0KB
timelines 261.4KB 234.1KB -27.3KB
total -77.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
timelines 23 25 +2

Page load bundle

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

id before after diff
timelines 231.3KB 320.7KB +89.4KB
Unknown metric groups

API count

id before after diff
timelines 878 882 +4

async chunk count

id before after diff
timelines 3 4 +1

References to deprecated APIs

id before after diff
timelines 28 34 +6

History

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

@semd semd changed the title [TGrid] Field browser implemented in EuiDataGrid toolbar [RAC] [TGrid] Field browser implemented in EuiDataGrid toolbar Jul 20, 2021
@@ -107,7 +107,7 @@ pageLoadAssetSize:
dataVisualizer: 27530
banners: 17946
mapsEms: 26072
timelines: 251886
timelines: 330000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like how big this is getting, but between #105941 and #105207 (comment) I understand this is just some debt we're taking on temporarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, thanks

@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.14 Commit could not be cherrypicked due to conflicts
7.x

Successful backport PRs will be merged automatically after passing CI.

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

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 20, 2021
…ic#105207)

* tGid header using EuiDataGrid

* useFetchIndex migrated and column_headers refactor

* removed useless mock

* add badges translations

* i18n translations keys fixed

* code format

* filter default columns not present in field browser

* reset button to initial columns

* cleaning

* dependencies moved

* fix functional test with missing data service

* remove unused code (unrelated)

* fieldBrowser integration with security solutions timeline

* lint and translations cleaned

* timeline toolbar removed for merge & some test fixes

* type fix

* type fixes

* timeline static default colums

* limit size temporary increase

* limit size temporary increase

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@semd semd removed the v7.14.0 label Jul 20, 2021
kibanamachine added a commit that referenced this pull request Jul 20, 2021
…) (#106278)

* tGid header using EuiDataGrid

* useFetchIndex migrated and column_headers refactor

* removed useless mock

* add badges translations

* i18n translations keys fixed

* code format

* filter default columns not present in field browser

* reset button to initial columns

* cleaning

* dependencies moved

* fix functional test with missing data service

* remove unused code (unrelated)

* fieldBrowser integration with security solutions timeline

* lint and translations cleaned

* timeline toolbar removed for merge & some test fixes

* type fix

* type fixes

* timeline static default colums

* limit size temporary increase

* limit size temporary increase

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Sergi Massaneda <sergi.massaneda@elastic.co>
andrew-goldstein added a commit to andrew-goldstein/kibana that referenced this pull request Jul 21, 2021
…enshots below:

![o11y_alerts](https://user-images.githubusercontent.com/4459398/126273572-7e02e3b1-c075-4b1a-9b77-03a6843d6b72.png)

![security_solution_alerts](https://user-images.githubusercontent.com/4459398/126279321-60d8c118-a97f-4c3f-b997-a966f7755d33.png)

Related RAC Issue: elastic/security-team#1299

To reduce the size of the `timelines` and `security_solution` plugins, legacy TGrid code and the dependency on `react-beautiful-dnd` will be removed in a follow-up PR.

- Related issue: elastic#105941

The legacy code and dependencies will be deleted when the following tasks are completed (in follow-up PRs):

- Sorting: Map `redux` sort state to `EuiDataGrid`'s `sorting` prop
- Actions: Migrate draggable hover actions to `EuiDataGrid` `cellActions`
  - related PR: <elastic#105500>
- Integrate with the Field Browser for adding / removing columns
  - related PR: <elastic#105207>
- Use `BrowserFields` to display field metadata when hovering over a column
  - related PR: <elastic#105207>
- Migrate Security Solution's actions column config from a single column to multiple columns

To desk test this PR, you must enable feature flags in the Observability and Security Solution:

- To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`:

```
xpack.observability.unsafe.cases.enabled: true
xpack.observability.unsafe.alertingExperience.enabled: true
xpack.ruleRegistry.write.enabled: true
```

- To desk test the TGrid in the following Security Solution, edit `x-pack/plugins/security_solution/common/experimental_features.ts` and in the `allowedExperimentalValues` section set:

```typescript
tGridEnabled: true,
```
andrew-goldstein added a commit that referenced this pull request Jul 21, 2021
## [RAC] [TGrid] Migrate the TGrid's rendering to `EuiDataGrid`

This PR migrates TGrid's rendering to use `EuiDataGrid`, per the screenshots below:

![o11y_alerts](https://user-images.githubusercontent.com/4459398/126413504-e825a5a2-1cb5-475e-b514-01fb819793e1.png)

![security_solution_alerts](https://user-images.githubusercontent.com/4459398/126413546-28df8f28-fa81-4b97-91c6-667589ea683c.png)

Related RAC Issue: elastic/security-team#1299

### Prerequisites to deleting legacy code (reducing bundle sizes)

To reduce the size of the `timelines` and `security_solution` plugins, legacy TGrid code and the dependency on `react-beautiful-dnd` will be removed in a follow-up PR.

- Related issue: #105941

The legacy code and dependencies will be deleted when the following tasks are completed (in follow-up PRs):

- Sorting: Map `redux` sort state to `EuiDataGrid`'s `sorting` prop
- Actions: Migrate draggable hover actions to `EuiDataGrid` `cellActions`
  - related PR: <#105500>
- Use `BrowserFields` to display field metadata when hovering over a column
  - related PR: <#105207>
- Migrate Security Solution's actions column config from a single column to multiple columns

### Desk testing

To desk test this PR, you must enable feature flags in the Observability and Security Solution:

- To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`:

```
xpack.observability.unsafe.cases.enabled: true
xpack.observability.unsafe.alertingExperience.enabled: true
xpack.ruleRegistry.write.enabled: true
```

- To desk test the TGrid in the following Security Solution, edit `x-pack/plugins/security_solution/common/experimental_features.ts` and in the `allowedExperimentalValues` section set:

```typescript
tGridEnabled: true,
```
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 21, 2021
…#106199)

## [RAC] [TGrid] Migrate the TGrid's rendering to `EuiDataGrid`

This PR migrates TGrid's rendering to use `EuiDataGrid`, per the screenshots below:

![o11y_alerts](https://user-images.githubusercontent.com/4459398/126413504-e825a5a2-1cb5-475e-b514-01fb819793e1.png)

![security_solution_alerts](https://user-images.githubusercontent.com/4459398/126413546-28df8f28-fa81-4b97-91c6-667589ea683c.png)

Related RAC Issue: elastic/security-team#1299

### Prerequisites to deleting legacy code (reducing bundle sizes)

To reduce the size of the `timelines` and `security_solution` plugins, legacy TGrid code and the dependency on `react-beautiful-dnd` will be removed in a follow-up PR.

- Related issue: elastic#105941

The legacy code and dependencies will be deleted when the following tasks are completed (in follow-up PRs):

- Sorting: Map `redux` sort state to `EuiDataGrid`'s `sorting` prop
- Actions: Migrate draggable hover actions to `EuiDataGrid` `cellActions`
  - related PR: <elastic#105500>
- Use `BrowserFields` to display field metadata when hovering over a column
  - related PR: <elastic#105207>
- Migrate Security Solution's actions column config from a single column to multiple columns

### Desk testing

To desk test this PR, you must enable feature flags in the Observability and Security Solution:

- To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`:

```
xpack.observability.unsafe.cases.enabled: true
xpack.observability.unsafe.alertingExperience.enabled: true
xpack.ruleRegistry.write.enabled: true
```

- To desk test the TGrid in the following Security Solution, edit `x-pack/plugins/security_solution/common/experimental_features.ts` and in the `allowedExperimentalValues` section set:

```typescript
tGridEnabled: true,
```
kibanamachine added a commit that referenced this pull request Jul 21, 2021
#106348)

## [RAC] [TGrid] Migrate the TGrid's rendering to `EuiDataGrid`

This PR migrates TGrid's rendering to use `EuiDataGrid`, per the screenshots below:

![o11y_alerts](https://user-images.githubusercontent.com/4459398/126413504-e825a5a2-1cb5-475e-b514-01fb819793e1.png)

![security_solution_alerts](https://user-images.githubusercontent.com/4459398/126413546-28df8f28-fa81-4b97-91c6-667589ea683c.png)

Related RAC Issue: elastic/security-team#1299

### Prerequisites to deleting legacy code (reducing bundle sizes)

To reduce the size of the `timelines` and `security_solution` plugins, legacy TGrid code and the dependency on `react-beautiful-dnd` will be removed in a follow-up PR.

- Related issue: #105941

The legacy code and dependencies will be deleted when the following tasks are completed (in follow-up PRs):

- Sorting: Map `redux` sort state to `EuiDataGrid`'s `sorting` prop
- Actions: Migrate draggable hover actions to `EuiDataGrid` `cellActions`
  - related PR: <#105500>
- Use `BrowserFields` to display field metadata when hovering over a column
  - related PR: <#105207>
- Migrate Security Solution's actions column config from a single column to multiple columns

### Desk testing

To desk test this PR, you must enable feature flags in the Observability and Security Solution:

- To desk test the `Observability > Alerts` page, add the following settings to `config/kibana.dev.yml`:

```
xpack.observability.unsafe.cases.enabled: true
xpack.observability.unsafe.alertingExperience.enabled: true
xpack.ruleRegistry.write.enabled: true
```

- To desk test the TGrid in the following Security Solution, edit `x-pack/plugins/security_solution/common/experimental_features.ts` and in the `allowedExperimentalValues` section set:

```typescript
tGridEnabled: true,
```

Co-authored-by: Andrew Goldstein <andrew-goldstein@users.noreply.github.com>
@semd
Copy link
Contributor Author

semd commented Jul 21, 2021

Hi @snide,
After talking with the team about your observations, we will change the component to a modal.
Before it was implemented using this (kind of) popover because the user was able to drag & drop fields from the popover browser to the actual table columns. But now this capability is not available anymore as we are removing all d&d from the tGrid, so now we can change the field browser to live inside a modal.
I will create another PR for this change.
Thanks.

@snide
Copy link
Contributor

snide commented Jul 21, 2021

@semd Sounds good and I think that's OK to do a separate PR as this one is mostly a port. I'll leave the review to a member of the @elastic/security-design team.

@semd
Copy link
Contributor Author

semd commented Jul 22, 2021

@snide Field Browser to EuiModal: #106541

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting Security Solution Threat Hunting Team v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants