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][Tech Debt] Decoupled TGrid state part from Timelines under the security_solution store #141010

Merged
merged 129 commits into from
Oct 18, 2022

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Sep 19, 2022

Resolves #143153
This was not initially in the scope, but this PR contains the changes, which allows to reduce the bundle size of the security_solution plugin:

Screen Shot 2022-09-30 at 6 02 48 PM

Before changes Screen Shot 2022-09-30 at 12 13 16 PM
After Screen Shot 2022-09-30 at 12 12 53 PM

The bundle size reduce was met by moving getSubPluginRoutesByCapabilities and manageOldSiemRoutes to the lazy loaded chunk.
Both functions are used after the application mounted, so shouldn't stop the security_solution plugin to load.

The actual theme of the PR is about the changes in the security_solution store related to splitting the logic for timelines and data tables.

New security solution redux store structure. Screen Shot 2022-10-02 at 5 04 11 PM
New TGrid redux store structure. Screen Shot 2022-10-02 at 5 34 01 PM Removed fields which is not used by TGrid:
documentType: string;
excludedRowRendererIds: RowRendererId[];
filterManager?: FilterManager;
footerText?: string | React.ReactNode;
dataProviders: DataProvider[];
dateRange: {
  start: string;
  end: string;
};
kqlQuery: {
  // TODO convert to nodebuilder
  filterQuery: SerializedFilterQuery | null;
};
timelineType: 'default' | 'template';
version: string | null;
To meet the requirements, the next changes was done: 1. Extended `security_solution` redux store to have separately state for the timelines and the data table:

Screen Shot 2022-09-26 at 8 40 27 AM

  1. Moved data table ids to own TableId enum and reduce TimelineId to keep only timelines:

Screen Shot 2022-09-26 at 8 42 39 AM

  1. Renamed storageTimelines?: Pick<TimelineState, 'timelineById'>; to storageDataTables?: Pick<TableState, 'tableById'>;, because we store only the data tables info about the user settings changes.
New local storage structure for the data table Screen Shot 2022-10-02 at 5 00 29 PM
4. For the components which are common for Timelines and TGrid and also contains the selectors or actions to fetch/update the store data, the temporary approach is to pick the proper action or selector based on the `scopeId`. Not perfect, because components should vary the logic based on the input, but to reduce the scope of the changes I suggest accept it as a first iteration and then follow up on modifying the components to use more extensible and stateless approach. For example, this is the code to pick the right selector based on the `scopeId`:
const getScope = useMemo(() => {
      if (isTimelineScope(scopeId)) {
        return timelineSelectors.getTimelineByIdSelector();
      } else if (isInTableScope(scopeId)) {
        return dataTableSelectors.getTableByIdSelector();
      }
    }, [scopeId]);

Fo actions:

if (isTimelineScope(scopeId)) {
            dispatch(
              timelineActions.removeColumn({
                columnId: column.id,
                id: scopeId,
              })
            );
          } else if (isInTableScope(scopeId)) {
            dispatch(
              dataTableActions.removeColumn({
                columnId: column.id,
                id: scopeId,
              })
            );
          }

This changes is applied to the next list of the hooks and components:
EventFieldsBrowser, useAlertsActions, AddressLinksItemComponent, GraphOverlayComponent, UseDetailPanelReturn, DetailsPanel, HeaderActionsComponent, ActionsComponent

Only data table selector and actions is used for PreviewHistogram, HostDetailsComponent, HostsComponent, HostsTabs, SessionsView, NetworkComponent

  1. ❗ ❗ ❗ This PR still has some temporary 👎 - need to have reduceReducers to manage addProviderToTimeline logic which is not movable for now to security_solution plugin. This is the specific part of the timeline data store management in security_solution plugin from the timelines plugin, which is used by other plugins as
timelines.getHoverActions().getAddToTimelineButton

to avoid the circular dependancies (particularly for osquery plugin, because this is not a sub plugin for security_solution and no way to use the store and security context like we did for threat_intelligence plugin).
To make it at least better organized, moved addProviderToTimeline action to the new timeline folder under the x-pack/plugins/timelines/public/store.
Exposed the reducer from the timeline plugin interface:

getTimelineReducer: () => {
        return timelineReducer;
      },

and combined this to manage the timeline part of the security_solution store.

  1. Changed the action for EventsQueryTabBodyComponent to update the dataTable store:
    from
timelineActions.initializeTGridSettings({
        id: timelineId,

to

dataTableActions.initializeTGridSettings({
     id: tableId,
  1. Exposed tableDefaults for the dataTable
  2. Cleanup work. Removed some not used functionality:
  • removed not used props for TGridModel
  • removed not used actions/helpers and reducers from TGrid store in x-pack/plugins/timelines/public/store/t_grid
  • renamed actions to correspond to the TGrid context
  • removed not used props for integrated TGrid
  • removed not used actions/helpers and reducers from Timelines store in x-pack/plugins/security_solution/public/timelines/store/timeline
  1. Renamed timelineId to scopeId for the next
interfaces

CellValueElementProps
EnrichedFieldInfo

and
components

DetailsPanel
BarChartComponent
DraggableLegendItemComponent
DraggableWrapperComponent
DefaultDraggable
AlertSummaryView
EnrichmentDescription
EnrichmentSummary
Overview
EventDetailsComponent
RelatedAlertsByProcessAncestry
Insights
RelatedAlertsBySession
RelatedAlertsBySourceEvent
Overview
StatusPopoverButton
ActionCell
PrevalenceCell
SummaryValueCell
EventsQueryTabBody
ShowTopNButton
HoverActions
MatrixHistogramComponent
SessionsViewComponent
StatefulTopNComponent
TopNComponent
UseAlertPrevalence
ExpandedCellValueActionsComponent
FieldValueCell
AlertsTableComponent
PreviewTableCellRenderer
PreviewRenderCellValue
TakeActionDropdown
RenderCellValue
DefaultCellRenderer
DetectionEnginePageComponent
RuleDetailsPageComponent
EventsQueryTabBody
EventsByDatasetComponent
DraggableWrapper
ExpandableEvent
FlyoutBodyComponent
FlyoutFooterComponent
EventDetailsPanelComponent

10. Modified `StatefulEventsViewerComponent` to manage data tables actions and selectors, removed not used properties. Use `defaultModel: SubsetTGridModel;` instead of `defaultModel: SubsetTimelineModel;` 11. Changed `eventsViewerSelector` to use data table selector `getTableByIdSelector` instead of timelines `getTimelineByIdSelector`. 12. Changed all usage of `timelineSelectors.getManageTimelineById()` to `timelineSelectors.getTimelineByIdSelector()` 13. Extended `x-pack/plugins/security_solution/public/common/mock/global_state.ts` with dataTable mock. 14. Exposed all data table related store functionality under the folder `x-pack/plugins/security_solution/public/common/store/data_table`: model, defaults, selectors and actions imported from timelines plugin.

@YulNaumenko YulNaumenko self-assigned this Sep 19, 2022
@YulNaumenko YulNaumenko added 8.6 candidate Team:Threat Hunting Security Solution Threat Hunting Team Team:Threat Hunting:Investigations Security Solution Investigations Team technical debt Improvement of the software architecture and operational architecture labels Sep 19, 2022
kibanamachine and others added 15 commits September 19, 2022 22:22
…m/YulNaumenko/kibana into security-solution-tgrid-own-state

# Conflicts:
#	x-pack/plugins/security_solution/public/timelines/store/data_table/selectors.ts
…m/YulNaumenko/kibana into security-solution-tgrid-own-state

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/hover_actions/use_hover_action_items.tsx
…grid-own-state

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/event_details/cti_details/threat_summary_view.tsx
#	x-pack/plugins/security_solution/public/common/components/event_details/event_details.tsx
#	x-pack/plugins/security_solution/public/common/lib/triggers_actions_ui/register_alerts_table_configuration.tsx
#	x-pack/plugins/security_solution/public/detections/components/alerts_table/default_config.tsx
#	x-pack/plugins/security_solution/public/detections/components/alerts_table/index.tsx
#	x-pack/plugins/security_solution/public/detections/components/osquery/osquery_flyout.tsx
#	x-pack/plugins/security_solution/public/detections/components/rules/rule_preview/preview_histogram.tsx
#	x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx
#	x-pack/plugins/security_solution/public/hosts/pages/details/index.tsx
#	x-pack/plugins/security_solution/public/hosts/pages/hosts.tsx
#	x-pack/plugins/security_solution/public/timelines/components/fields_browser/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/expandable_event.tsx
#	x-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/flyout/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/side_panel/event_details/index.tsx
#	x-pack/plugins/security_solution/public/timelines/components/timeline/body/renderers/reason_column_renderer.tsx
#	x-pack/plugins/security_solution/public/users/pages/details/index.tsx
#	x-pack/plugins/timelines/public/components/t_grid/helpers.tsx
#	x-pack/plugins/timelines/public/components/t_grid/integrated/index.tsx
#	x-pack/plugins/timelines/public/components/t_grid/standalone/index.tsx
@YulNaumenko YulNaumenko added the release_note:skip Skip the PR/issue when compiling release notes label Sep 23, 2022
…grid-own-state

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/sessions_viewer/index.tsx
@YulNaumenko
Copy link
Contributor Author

Sync load times look great, thank you for improving it. Do we know what's going on with the async size doubling?

@jbudz I've reduced twice the async chunks under the current PR and opened a follow up issue to revisit in general how we approach the bundle size.

};

/**
* Migrates the values of the data table from the legacy timelines key to the securityDataTable key
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
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

sync size changes/limits.yml LGTM. I was advised to leave the async bundles out of scope for the operations review.

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 for making these changes! Did clickthrough testing for the tables including the detail flyouts, hover action functionality, and column persistence. It all looks to be working as expected! Thanks, LGTM!

@YulNaumenko YulNaumenko enabled auto-merge (squash) October 18, 2022 15:59
…grid-own-state

# Conflicts:
#	x-pack/plugins/security_solution/public/common/components/event_details/event_details.tsx
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3165 3169 +4

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
securitySolution 54 75 +21
timelines 346 348 +2
total +23

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.6MB 9.3MB ⚠️ +2.8MB
timelines 26.4KB 26.3KB -92.0B
total ⚠️ +2.8MB

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 23 26 +3
timelines 33 32 -1
total +2

Page load bundle

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

id before after diff
securitySolution 265.3KB 51.1KB -214.3KB
timelines 269.3KB 222.2KB -47.1KB
total -261.4KB
Unknown metric groups

API count

id before after diff
securitySolution 55 112 +57
timelines 452 457 +5
total +62

async chunk count

id before after diff
securitySolution 32 39 +7

ESLint disabled in files

id before after diff
timelines 2 0 -2

ESLint disabled line counts

id before after diff
timelines 36 34 -2

Total ESLint disabled count

id before after diff
timelines 38 34 -4

History

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

cc @YulNaumenko

@YulNaumenko YulNaumenko merged commit ade016b into elastic:main Oct 18, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 18, 2022
kc13greiner pushed a commit to kc13greiner/kibana that referenced this pull request Oct 19, 2022
…nes under the security_solution store (elastic#141010)

* [Security Solution][Tech Debt] Decoupled TGrid state part from Timelines under the security_solution store

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Unified usage of data table get by id selector

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Cleanup - removed not used code

* -

* -

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fixed add to timeline

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fixed filter manager for useHoverActions by proper context usage for defining the scopeId

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fixed es lint

* -

* TableIds to TableId

* Fixed unit tests

* Fixed tests

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* -

* fixed garphevent component

* FIxed details tests

* Added mock for cases test

* Fixed store tests

* fixed mocks

* fixed mocks

* Cleaned up tgrid store from the timeline actions

* Set back reduceReducers to handle ability addToTimelineButton, need to change this later when timelines data will live in the timeline plugin

* fixed merge

* fixed check types

* Fixed type checks

* Fixed tests

* Added snapshot

* Fixed toggleDetails for user and host

* fixed tests

* Fixed timelines tests

* FIxed tests

* Fixed tests

* Fixed tests

* Fixed Jest tests

* Fixed resolver bug

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* FIxed miissing filterManager

* moved tgrid store

* Reduced bundle size!

* Fixed names

* Fixed tests

* Removed test

* New securitySolution bundle size

* Cleanup the store

* More cleanup

* Removed footer

* removed excludedRowRendererIds

* Fixed typecheck

* remove tests changes

* Cleaned up unused selectors

* Removed savedObjectId from tgrid state

* fixed type check

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Resolved the comments

* Fixed due to comments

* Fixed type checks

* Fixed tests

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* fixed merge issue

* Move suricata-sid-db to lazy loaded modules

* Fixed test

* moved mitre helpers to async chunk

* Fixed due to comments

* Fixed tests

* Renamed TableId.detectionsRulesDetailsPage -> TableId.alertsOnRuleDetailsPage
TableId.detectionsPage -> TableId.alertsOnAlertsPage

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fixed typecheck

* Fixed test

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
guskovaue pushed a commit to guskovaue/kibana that referenced this pull request Oct 22, 2022
…nes under the security_solution store (elastic#141010)

* [Security Solution][Tech Debt] Decoupled TGrid state part from Timelines under the security_solution store

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Unified usage of data table get by id selector

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Cleanup - removed not used code

* -

* -

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fixed add to timeline

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fixed filter manager for useHoverActions by proper context usage for defining the scopeId

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fixed es lint

* -

* TableIds to TableId

* Fixed unit tests

* Fixed tests

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* -

* fixed garphevent component

* FIxed details tests

* Added mock for cases test

* Fixed store tests

* fixed mocks

* fixed mocks

* Cleaned up tgrid store from the timeline actions

* Set back reduceReducers to handle ability addToTimelineButton, need to change this later when timelines data will live in the timeline plugin

* fixed merge

* fixed check types

* Fixed type checks

* Fixed tests

* Added snapshot

* Fixed toggleDetails for user and host

* fixed tests

* Fixed timelines tests

* FIxed tests

* Fixed tests

* Fixed tests

* Fixed Jest tests

* Fixed resolver bug

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* FIxed miissing filterManager

* moved tgrid store

* Reduced bundle size!

* Fixed names

* Fixed tests

* Removed test

* New securitySolution bundle size

* Cleanup the store

* More cleanup

* Removed footer

* removed excludedRowRendererIds

* Fixed typecheck

* remove tests changes

* Cleaned up unused selectors

* Removed savedObjectId from tgrid state

* fixed type check

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Resolved the comments

* Fixed due to comments

* Fixed type checks

* Fixed tests

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* fixed merge issue

* Move suricata-sid-db to lazy loaded modules

* Fixed test

* moved mitre helpers to async chunk

* Fixed due to comments

* Fixed tests

* Renamed TableId.detectionsRulesDetailsPage -> TableId.alertsOnRuleDetailsPage
TableId.detectionsPage -> TableId.alertsOnAlertsPage

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* Fixed typecheck

* Fixed test

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.6 candidate backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team Team:Threat Hunting Security Solution Threat Hunting Team technical debt Improvement of the software architecture and operational architecture v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution]Separating the data store management for Timelines and TGrid data tables.