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

[Security Solution] Decompose Timelines TGrid component and moved to security_solution #140151

Merged

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Sep 7, 2022

Resolves #143152

Observability changes

This changes is a result of removal some types from timelines plugin:

  • cleaned up timelines plugin related types,

  • replaced Pick<ActionProps,'data' | 'eventId' | 'ecsData' | 'setEventsDeleted' > with the props which were actually used:

    data: TimelineNonEcsData[];
    ecsData: Ecs;
    eventId: string;
    

In this PR we still have references to @kbn/timelines-plugin, which needs to be changed later.
Threat Hunting team are going to think about replacing TimelineNonEcsData with the other type definition (maybe NonEcsData?) and moving Ecs type to the non timelines related plugin/package.

Security Solution changes

Before the current PR changes the components dependencies around TGrid looked like the image below:
Screen Shot 2022-11-29 at 6 16 14 AM
After decomposition the timelines plugin hosted TGrid HOC and moving all the data tables related sub-components to security_solution plugin the new components architecture got the next shape:
Screen Shot 2022-11-29 at 6 14 41 AM
security_solution plugin changes includes the next things:

  • Moved some data table and actions types to x-pack/plugins/security_solution/common/types, which is widely used across the related components.
  • Due to the movement of the data table with the store to from timeline plugin to security_solution many test files which had the reference to tGridReducer now cleaned up from the unnecessary logic:
- import { tGridReducer } from '@kbn/timelines-plugin/public';

and TableState references was replaced with the next changes:

- import type { TableState } from '@kbn/timelines-plugin/public';
+ import type { TableState } from '../common/store/data_table/types';
  • Replaced tGridActions with dataTableActions name.
  • Moved control_columns to security_solution common plugin components: RowCheckBox, HeaderCheckBox and transformControlColumns:
    RowActionComponent moved from timelines plugin to x-pack/plugins/security_solution/public/common/components/control_columns/row_action without changes.
    transformControlColumns moved from timelines plugin to x-pack/plugins/security_solution/public/common/components/control_columns/transform_control_columns.tsx. Removed not used property hasAlertsCrudPermissions, added unit test.

Screen Shot 2022-11-29 at 8 59 42 PM

  • Many translation changes as a result of the owner plugin change:
- i18n.translate('xpack.timelines....', {
+ i18n.translate('xpack.securitySolution....', {
  • Moved useDraggableKeyboardWrapper to security_solution, added reference to useAddToTimeline, by using timelines plugin with kibana services. Added unit tests.

Screen Shot 2022-11-30 at 9 06 42 AM

  • Replaced the next references:
- type: 'x-pack/timelines/t-grid/UPDATE_COLUMN_WIDTH',
+ type: 'x-pack/security_solution/data-table/UPDATE_COLUMN_WIDTH',
- type: 'x-pack/timelines/t-grid/REMOVE_COLUMN',
+ type: 'x-pack/security_solution/data-table/REMOVE_COLUMN',
  • moved TGrid store previously hosted in timeline plugin to security_solution as data_table store:

Screen Shot 2022-11-29 at 9 24 08 PM

  • Migrated TGrid BodyComponent to DataTableComponent
    x-pack/plugins/security_solution/public/common/components/data_table/index.tsx
    Removed some unused properties: hasAlertsCrudPermissions, appId, getRowRenderer, isEventViewer, tableView, totalSelectAllAlerts, trailingControlColumns. Current DataTableComponent is a subset of the previous BodyComponent, which includes only table related functionality:

Screen Shot 2022-11-30 at 10 44 35 AM

  • Renamed TimelineExpandedDetail to ExpandedDetail to make the type more generic for usage.
  • BulkActions related changes includes:

Screen Shot 2022-11-29 at 9 13 32 PM

AlertBulkActionsComponent moved from timelines plugin to x-pack/plugins/security_solution/public/common/components/toolbar/bulk_actions/alert_bulk_actions.tsx, just renaming changes.

Added x-pack/plugins/security_solution/public/common/components/toolbar/bulk_actions/types.ts to consolidate types
useBulkActionItems moved from timelines plugin to x-pack/plugins/security_solution/public/common/components/toolbar/bulk_actions/use_bulk_action_items.tsx. Changed links, renamed AlertsStatus to AlertWorkflowStatus, removed in-progress case handling.

useUpdateAlertsStatus moved from timelines plugin to x-pack/plugins/security_solution/public/common/components/toolbar/bulk_actions/use_update_alerts.ts. Cleaned up the code from handling Observability API.

  • Updated x-pack/plugins/security_solution/public/common/lib/kuery/index.ts with the actual implementations of
  convertKueryToDslFilter,
  convertKueryToElasticSearchQuery,
  convertToBuildEsQuery,
  escapeKuery,
  escapeQueryValue,
  combineQueries,

instead of referencing timelines plugin.

  • Moved EventRenderedView component to security_solution common components. Later planning to make it as a package.
  • EventsViewer component became the stateful component which is responsible for the data representation managing. Some part from TGridIntegratedComponent and BodyComponent was merged under its logic:

Screen Shot 2022-11-30 at 6 22 22 PM

Screen Shot 2022-11-30 at 6 24 06 PM

  • Moved header actions to common components x-pack/plugins/security_solution/public/common/components/header_actions
  • Renamed component AlertCount to UnitCount.
  • Moved to security_solution configuration for APM_USER_INTERACTIONS
  • changes createStore interface by using the direct reference to dataTableReducer instead of passing down it's value through the params.

Timeline plugin changes

  • cleaned up timeline plugin interface by removing:
getTGrid: <T extends TGridType = 'embedded'>(
    props: GetTGridProps<T>
  ) => ReactElement<GetTGridProps<T>>;
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
getTGridReducer: () => any;
getUseDraggableKeyboardWrapper: () => (
    props: UseDraggableKeyboardWrapperProps
  ) => UseDraggableKeyboardWrapper;
  • renamed embedded store
- setTGridEmbeddedStore: (store: Store) => void;
+ setTimelineEmbeddedStore: (store: Store) => void;
  • removed dependency to triggers_actions_ui plugin
  • removed duplicated components and types with security_solution:
TruncatableText
SubtitleComponent
EventsCountComponent
PopoverRowItems
PagingControlComponent
FooterComponent
SortIndicator
SortNumber
RowRendererContainer
plainRowRenderer
getColumnRenderer
StatefulRowRenderer
getMappedNonEcsValue
InspectButtonComponent
TGridCellAction
useMountAppended
tgrid store

YulNaumenko and others added 7 commits August 26, 2022 15:07
…ompose-layers-part1

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsx
#	x-pack/plugins/timelines/public/components/t_grid/integrated/index.tsx
…ompose-layers-part1

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/event_rendered_view/index.tsx
#	x-pack/plugins/security_solution/public/common/components/event_rendered_view/selector/index.tsx
#	x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsx
#	x-pack/plugins/timelines/public/components/t_grid/body/index.tsx
#	x-pack/plugins/timelines/public/components/t_grid/integrated/index.tsx
#	x-pack/plugins/timelines/public/components/t_grid/standalone/index.tsx
#	x-pack/plugins/timelines/public/components/t_grid/styles.tsx
@YulNaumenko YulNaumenko self-assigned this Oct 19, 2022
@YulNaumenko YulNaumenko added v8.6.0 technical debt Improvement of the software architecture and operational architecture Team:Threat Hunting:Investigations Security Solution Investigations Team release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Oct 19, 2022
Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

The Show top <field> popover action in the alerts page appears to be ignoring the Additional filters > Include building block alerts setting.

As a result (for example), the Top host.name popover will (incorrectly) not count building block alerts, even when they are shown in the table.

To reproduce:

  1. Filter the alerts page such that all the alerts are from an EQL sequence rule

Expected results

  • The entire alerts page is filtered to only include EQL sequence alerts, per screenshot below:

filter_by_EQL_sequence

  1. Add an "exists" filter for the host.name field, to make testing easier

Expected results

  • A host.name: exists filter is included at the top of the alerts page, per the screenshot below:

host_name_exists

  1. Now that all the page-level filters are in place, note the count of alerts shown in the table

Expected results

  • Per the screenshot below, the alert count does NOT include building block alerts
    alert_count_no_building_block_alerts
  • There are no building block alerts shown in the table above
  1. Inspect the table query

Expected result

  • The table query includes the following clause:
            "must_not": [
              {
                "exists": {
                  "field": "kibana.alert.building_block_type"
                }
              }
  1. Close the table query inspector

  2. Open the host.name popover for any row, and click Show top values

Expected result

  • When hovering over the values in the Top host.name graph, the sum of the values is equal to the count of alerts in the table, per the screenshot below:

show_top_host_name_no_building_block_alerts

  1. Inspect the Top host.name query

Expected result

  • The Inspect Top host.name query request includes the following clause:
            "must_not": [
              {
                "exists": {
                  "field": "kibana.alert.building_block_type"
                }
              }
  1. Close the Inspect Top host.name

  2. Close the Top host.name popover

  3. Open the Additional filters drop down, and select the Include building block alerts option, per the screenshot below:

include_building_blocks

Expected results

  • The alerts table is updated to display building block alerts
  • The alerts table count now includes building block alerts, per the screenshot below:

table_and_count_with_building_block_alerts

  1. Once again, inspect the table query

Expected result

  • The table query does NOT include the following clause:
            "must_not": [
              {
                "exists": {
                  "field": "kibana.alert.building_block_type"
                }
              }
  1. Once again, open the host.name popover for any row, and click Show top values

Expected result

  • When hovering over the values in the Top host.name graph, the sum of the values is equal to the count of alerts in the table

Actual result

  • The sum in the Top host.name does NOT equal the alert table count, per the screenshot below:

does_not_add_up

  1. Once again, inspect the Top host.name query

Expected result

  • The Inspect Top host.name query does NOT include the following clause:
            "must_not": [
              {
                "exists": {
                  "field": "kibana.alert.building_block_type"
                }
              }

Actual result

  • The Inspect Top host.name query includes the (unexpected) clause, per the screenshot below:

unexpected

@andrew-goldstein
Copy link
Contributor

The Event rendered view appears to be falling back to the plain-text rendered version of the alert.

I've tried to reproduce the issue by following the steps, but I cannot or I maybe misunderstood how the Reason should behave:

Screen.Recording.2022-12-02.at.5.04.59.PM.mov

The issue is only sometimes-reproducible. It's easier to reproduce with alerts that don't have an event.category. (I can reproduce it locally when testing with alerts from EQL sequence rules, and with threshold alerts.)

After some debugging, it looks like the issue is the EventRenderedView isn't being passed the getRowRenderer prop in the PR branch.

As a result of the getRowRenderer prop being undefined, the branching logic in x-pack/plugins/security_solution/public/common/components/event_rendered_view/index.tsx in the following code:

          const rowRenderer =
            getRowRenderer != null
              ? getRowRenderer({ data: ecsData, rowRenderers })
              : rowRenderers.find((x) => x.isInstance(ecsData)) ?? null;

was defaulting to the rowRenderers.find path, because getRowRenderer was undefined.

The fix (in my local checkout of the PR branch): is to add the following import:

import { getRowRenderer } from '../../../timelines/components/timeline/body/renderers/get_row_renderer';

to x-pack/plugins/security_solution/public/common/components/events_viewer/index.tsx

and then pass it to the getRowRenderer of the EventRenderedView view, per the example code below:

                          {tableView === 'eventRenderedView' && (
                            <EventRenderedView
                              events={nonDeletedEvents}
                              getRowRenderer={getRowRenderer}
                              leadingControlColumns={transformedLeadingControlColumns}
                              pagination={{
                                pageIndex: pageInfo.activePage,
                                pageSize: itemsPerPage,
                                totalItemCount: totalCountMinusDeleted,
                                pageSizeOptions: itemsPerPageOptions,
                                showPerPageOptions: true,
                              }}
                              rowRenderers={rowRenderers}
                              scopeId={tableId}
                              onChangePage={onChangePage}
                              onChangeItemsPerPage={onChangeItemsPerPage}
                              additionalControls={alertBulkActions}
                              unitCountText={unitCountText}
                            />
                          )}

Before the fix above, the Event rendered view in the PR branch looks like this:

before

after the fix, it renders as-expected:

after

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

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

Thanks @YulNaumenko for all the improvements to the code in this PR 🙏
LGTM 🚀

type: 'embedded',
unit,
})}
<StyledEuiPanel
Copy link
Contributor

Choose a reason for hiding this comment

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

And it's back! Thanks for doing this work 💪🏾 🎉

setSelected: dataTableActions.setSelected,
};

const connector = connect(undefined, mapDispatchToProps);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to remove the connected and just use useDispatch for the onRowSelected and onSelectPage? I don't think we get any benefit from connect over hooks when were not actually pulling in any state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need that connect for making checkboxes working. I didn't dig into details of its implementation, but we can revisit existing code in the follow up issue.

setStore: (store: Store) => void;
};

export const TGrid = (props: TGridComponent) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👌🏾

Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

Thanks again for doing this work! Just had some nits. 🚀 LGTM!

@stephmilovic
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

Thanks for this cleanup work, LGTM!

Copy link
Contributor

@mitodrummer mitodrummer left a comment

Choose a reason for hiding this comment

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

LGT-AWP!

Copy link
Contributor

@dplumlee dplumlee left a comment

Choose a reason for hiding this comment

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

Looks good from alerts area code 🚀

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #2 / Alerts detection rules table auto-refresh should disable auto refresh when any rule selected and enable it after rules unselected
  • [job] [logs] Security Solution Tests #1 / Alerts timeline Add a non-empty property to default timeline
  • [job] [logs] FTR Configs #4 / lens app - TSVB Open in Lens Table should not allow converting series with different aggregation function or aggregation by

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3334 3378 +44
timelines 192 123 -69
total -25

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 350 214 -136

Async chunks

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

id before after diff
observability 497.9KB 497.9KB -28.0B
securitySolution 9.9MB 10.1MB ⚠️ +260.9KB
timelines 74.8KB 30.0KB -44.7KB
total +216.1KB

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
securitySolution 27 28 +1
timelines 33 21 -12
total -11

Page load bundle

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

id before after diff
securitySolution 51.0KB 51.0KB +3.0B
timelines 139.8KB 87.4KB -52.4KB
total -52.4KB
Unknown metric groups

API count

id before after diff
timelines 462 257 -205

async chunk count

id before after diff
securitySolution 36 37 +1
timelines 10 8 -2
total -1

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 443 451 +8
timelines 32 23 -9
total +13

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 520 528 +8
timelines 32 23 -9
total +14

History

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

cc @YulNaumenko

@CoenWarmer CoenWarmer requested review from CoenWarmer and removed request for CoenWarmer December 6, 2022 08:45
Copy link
Contributor

@CoenWarmer CoenWarmer left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.7 candidate backport:skip This commit does not require backporting 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:Investigations Security Solution Investigations Team technical debt Improvement of the software architecture and operational architecture v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Decouple TGrid layers and dependencies and move the components to security_solution plugin