Skip to content

Commit

Permalink
[Threat Hunting Investigations] Show the correct stats in unified fie…
Browse files Browse the repository at this point in the history
…ld list (#180850)

## Summary

Fixes elastic/security-team#9077

Enables correct rendering of top values in the unified field list
through a new `dataService` for timeline. This `timelineDataService` is
synced with the current timeline's range filter, the selected
`indexPattern` and the special `combinedQuery` that timeline is based
on.

### Example 
Two values for `event.category` are present in the current timeline's
alerts (`behavior` and `process`):

<img width="930" alt="Screenshot 2024-04-15 at 22 06 33"
src="https://github.com/elastic/kibana/assets/68591/501ea66e-aaf8-4bc1-b79c-59101eee7ddb">

When filtering for a specific `_id`, only the `process` value is showing
and the counts are also correct (`1`):

<img width="932" alt="Screenshot 2024-04-15 at 22 06 56"
src="https://github.com/elastic/kibana/assets/68591/0871914e-e1b1-4a13-9126-c2523d273009">

### A wild bugfix appeared

While working on this change, I noticed some irregularities that stemmed
from a race condition between updates of the filters in redux and in
filter manager. In a worst case scenario, this race condition could lead
to marking unchanged timelines as changed when opened.

Before this fix/refactoring, `filterManager` updates where propagated
from a subscription in `QueryBarTimeline`, which is quite deep in the
component tree. This resulted in prop drilling (`setFilter`) and
bubbling up of changes to where they are handled (`QueryBarTimeline` ->
`SearchOrFilter` -> `StatefulSearchOrFilter`).

Before:
```mermaid
flowchart TD
  subgraph Redux
    Store
  end

  Store -- filters,setFilters --> StatefulSearchOrFilter
  StatefulSearchOrFilter -. setFilters(filters) .-> Store

  StatefulSearchOrFilter -- prop:filters,onFiltersUpdated --> FilterItems --> FilterItem
  FilterItem -. onRemove .-> FilterItems
  FilterItems -. onFiltersUpdated(filters) .-> StatefulSearchOrFilter
  
  StatefulSearchOrFilter -- prop:setFilters,filters --> SearchOrFilter
  SearchOrFilter -. setFilters(filters) .-> StatefulSearchOrFilter
  SearchOrFilter -- prop:setFilters,filters --> QueryBarTimeline
  QueryBarTimeline -. setFilters(filters) .-> SearchOrFilter

  QueryBarTimeline ==> |subscribe| TimelineFilterManager
  TimelineFilterManager -.-> |next| QueryBarTimeline

  FilterIn ~~~ TimelineFilterManager
  FilterOut ~~~ TimelineFilterManager
  DisableFilter ~~~  TimelineFilterManager
  FilterIn -.-> TimelineFilterManager
  FilterOut -.-> TimelineFilterManager
  DisableFilter -.->  TimelineFilterManager
```

In this refactoring, `StatefulSearchOrFilter` is directly subscribing to
filter manager changes and `setFilters` has been removed, making
`QueryBarTimeline` a proper leaf node. `StatefulSearchOrFilter` now also
contains all of the synchronization code between filter manager and
redux.

After:
```mermaid
flowchart TD
  subgraph Redux
    Store
  end

  Store -- filters,setFilters --> StatefulSearchOrFilter
  StatefulSearchOrFilter -. setFilters(filters) .-> Store

  StatefulSearchOrFilter -- prop:filters,onFiltersUpdated --> FilterItems
  FilterItems -. onFiltersUpdated(filters) .-> StatefulSearchOrFilter
  
  StatefulSearchOrFilter -- prop:filters --> SearchOrFilter
  SearchOrFilter -- prop:setFilters,filters --> QueryBarTimeline

  StatefulSearchOrFilter ==> |subscribe| TimelineFilterManager
  TimelineFilterManager -.-> |next| StatefulSearchOrFilter

  subgraph FilterManager
  FilterIn ~~~ TimelineFilterManager
  FilterOut ~~~ TimelineFilterManager
  DisableFilter ~~~  TimelineFilterManager
  FilterIn -.-> TimelineFilterManager
  FilterOut -.-> TimelineFilterManager
  DisableFilter -.->  TimelineFilterManager
  end
```

###
- make sure `unifiedComponentsInTimelineEnabled` is enabled before
testing this PR

### Checklist

- [x] memoize `combinedQueries`
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <[email protected]>
  • Loading branch information
janmonschke and kibanamachine authored May 8, 2024
1 parent 216ecd7 commit aa1df6d
Show file tree
Hide file tree
Showing 26 changed files with 244 additions and 129 deletions.
1 change: 1 addition & 0 deletions x-pack/plugins/security_solution/kibana.jsonc
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
"notifications",
"savedSearch",
"unifiedDocViewer",
"charts"
],
"optionalPlugins": [
"cloudExperiments",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ describe('createFilterInCellActionFactory', () => {
it('should execute using generic filterManager', async () => {
await filterInAction.execute(dataTableContext);
expect(mockGlobalFilterManager.addFilters).toHaveBeenCalled();
expect(services.timelineFilterManager.addFilters).not.toHaveBeenCalled();
expect(services.timelineDataService.query.filterManager.addFilters).not.toHaveBeenCalled();
});

it('should show warning if value type is unsupported', async () => {
Expand All @@ -122,7 +122,7 @@ describe('createFilterInCellActionFactory', () => {
],
});
expect(mockGlobalFilterManager.addFilters).not.toHaveBeenCalled();
expect(services.timelineFilterManager.addFilters).not.toHaveBeenCalled();
expect(services.timelineDataService.query.filterManager.addFilters).not.toHaveBeenCalled();
expect(mockWarningToast).toHaveBeenCalled();
});
});
Expand All @@ -135,7 +135,7 @@ describe('createFilterInCellActionFactory', () => {

it('should execute using timeline filterManager', async () => {
await filterInAction.execute(timelineContext);
expect(services.timelineFilterManager.addFilters).toHaveBeenCalled();
expect(services.timelineDataService.query.filterManager.addFilters).toHaveBeenCalled();
expect(mockGlobalFilterManager.addFilters).not.toHaveBeenCalled();
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export const createFilterInCellActionFactory = ({
}) => {
const { filterManager } = services.data.query;
const { notifications } = services;
const {
query: { filterManager: timelineFilterManager },
} = services.timelineDataService;
const genericFilterInActionFactory = createFilterInActionFactory({
filterManager,
notifications,
Expand Down Expand Up @@ -65,7 +68,7 @@ export const createFilterInCellActionFactory = ({
const addFilter = metadata?.negateFilters === true ? addFilterOut : addFilterIn;

if (metadata?.scopeId && isTimelineScope(metadata.scopeId)) {
addFilter({ filterManager: services.timelineFilterManager, fieldName, value, dataViewId });
addFilter({ filterManager: timelineFilterManager, fieldName, value, dataViewId });
} else {
addFilter({ filterManager, fieldName, value, dataViewId });
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ describe('createFilterOutCellActionFactory', () => {
it('should execute using generic filterManager', async () => {
await filterOutAction.execute(dataTableContext);
expect(mockGlobalFilterManager.addFilters).toHaveBeenCalled();
expect(mockServices.timelineFilterManager.addFilters).not.toHaveBeenCalled();
expect(
mockServices.timelineDataService.query.filterManager.addFilters
).not.toHaveBeenCalled();
});

it('should show warning if value type is unsupported', async () => {
Expand All @@ -119,7 +121,9 @@ describe('createFilterOutCellActionFactory', () => {
],
});
expect(mockGlobalFilterManager.addFilters).not.toHaveBeenCalled();
expect(mockServices.timelineFilterManager.addFilters).not.toHaveBeenCalled();
expect(
mockServices.timelineDataService.query.filterManager.addFilters
).not.toHaveBeenCalled();
expect(mockWarningToast).toHaveBeenCalled();
});
});
Expand All @@ -132,7 +136,7 @@ describe('createFilterOutCellActionFactory', () => {

it('should execute using timeline filterManager', async () => {
await filterOutAction.execute(timelineContext);
expect(mockServices.timelineFilterManager.addFilters).toHaveBeenCalled();
expect(mockServices.timelineDataService.query.filterManager.addFilters).toHaveBeenCalled();
expect(mockGlobalFilterManager.addFilters).not.toHaveBeenCalled();
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ export const createFilterOutCellActionFactory = ({
}) => {
const { filterManager } = services.data.query;
const { notifications } = services;
const {
query: { filterManager: timelineFilterManager },
} = services.timelineDataService;

const genericFilterOutActionFactory = createFilterOutActionFactory({
filterManager,
Expand Down Expand Up @@ -65,7 +68,7 @@ export const createFilterOutCellActionFactory = ({
const addFilter = metadata?.negateFilters === true ? addFilterIn : addFilterOut;

if (metadata?.scopeId && isTimelineScope(metadata.scopeId)) {
addFilter({ filterManager: services.timelineFilterManager, fieldName, value, dataViewId });
addFilter({ filterManager: timelineFilterManager, fieldName, value, dataViewId });
} else {
addFilter({ filterManager, fieldName, value, dataViewId });
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jest.mock('../../../timelines/store', () => ({

describe('createFilterLensAction', () => {
const mockServices = {
timelineFilterManager: 'mockTimelineFilterManager',
timelineDataService: { query: { filterManager: 'mockTimelineFilterManager' } },
data: { query: { filterManager: 'mockFilterManager' } },
application: { currentAppId$: of('appId') },
topValuesPopover: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,16 @@ export const createFilterLensAction = ({
services,
negate,
}: CreateFilterLensActionParams) => {
const { application, notifications, data: dataService, topValuesPopover } = services;
const {
application,
notifications,
data: dataService,
topValuesPopover,
timelineDataService,
} = services;
const {
query: { filterManager: timelineFilterManager },
} = timelineDataService;

let currentAppId: string | undefined;
application.currentAppId$.subscribe((appId) => {
Expand Down Expand Up @@ -93,7 +102,7 @@ export const createFilterLensAction = ({
const timeline = getTimelineById(store.getState(), TimelineId.active);
// timeline is open add the filter to timeline, otherwise add filter to global filters
const filterManager = timeline?.show
? services.timelineFilterManager
? timelineFilterManager
: dataService.query.filterManager;

// If value type is value_count, we want to filter an `Exists` filter instead of a `Term` filter
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ export const useHoverActionItems = ({
}: UseHoverActionItemsProps): UseHoverActionItems => {
const kibana = useKibana();
const dispatch = useDispatch();
const { timelines, timelineFilterManager, analytics, i18n, theme } = kibana.services;
const { timelines, timelineDataService, analytics, i18n, theme } = kibana.services;
const {
query: { filterManager: timelineFilterManager },
} = timelineDataService;
const dataViewId = useDataViewId(getSourcererScopeId(scopeId ?? ''));

// Common actions used by the alert table and alert flyout
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,34 @@ describe('Sourcerer Hooks', () => {
expect(result.current.indexPattern).toHaveProperty('getName');
});
});

it('should update the title of the data view according to the selected patterns', async () => {
const { result, rerender } = renderHook<SourcererScopeName, SelectedDataView>(
() => useSourcererDataView(),
{
wrapper: ({ children }) => <Provider store={store}>{children}</Provider>,
}
);

expect(result.current.sourcererDataView?.title).toBe(
'apm-*-transaction*,auditbeat-*,endgame-*,filebeat-*,logs-*,packetbeat-*,traces-apm*,winlogbeat-*,-*elastic-cloud-logs-*'
);

const testPatterns = ['anotherTestPattern', 'justATestPattern'];
await act(async () => {
store.dispatch(
sourcererActions.setSelectedDataView({
id: SourcererScopeName.default,
selectedDataViewId: 'security-solution-default',
selectedPatterns: testPatterns,
})
);

await rerender();

expect(result.current.sourcererDataView?.title).toBe(testPatterns.join(','));
});
});
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -445,11 +445,18 @@ export const useSourcererDataView = (
}
}, [missingPatterns, selectedDataView, selectedPatterns]);

const sourcererDataView = useMemo(
() =>
selectedDataView == null || missingPatterns.length > 0 ? legacyDataView : selectedDataView,
[legacyDataView, missingPatterns.length, selectedDataView]
);
const sourcererDataView = useMemo(() => {
const _dv =
selectedDataView == null || missingPatterns.length > 0 ? legacyDataView : selectedDataView;
// Make sure the title is up to date, so that the correct index patterns are used everywhere
return {
..._dv,
dataView: {
..._dv.dataView,
title: selectedPatterns.join(','),
},
};
}, [legacyDataView, missingPatterns.length, selectedDataView, selectedPatterns]);

const indicesExist = useMemo(() => {
if (loading || sourcererDataView.loading) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import { coreMock, themeServiceMock } from '@kbn/core/public/mocks';
import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
import { dataPluginMock } from '@kbn/data-plugin/public/mocks';
import { securityMock } from '@kbn/security-plugin/public/mocks';
import { createFilterManagerMock } from '@kbn/data-plugin/public/query/filter_manager/filter_manager.mock';

import {
DEFAULT_APP_REFRESH_INTERVAL,
Expand Down Expand Up @@ -128,25 +127,25 @@ export const createStartServicesMock = (
const guidedOnboarding = guidedOnboardingMock.createStart();
const cloud = cloudMock.createStart();
const mockSetHeaderActionMenu = jest.fn();
const mockTimelineFilterManager = createFilterManagerMock();
const timelineDataService = dataPluginMock.createStartContract();
const alerting = alertingPluginMock.createStartContract();

/*
* Below mocks are needed by unified field list
* when data service is passed through as a prop
*
* */
data.query.timefilter.timefilter.getAbsoluteTime = jest.fn(() => ({
timelineDataService.query.timefilter.timefilter.getAbsoluteTime = jest.fn(() => ({
from: '2021-08-31T22:00:00.000Z',
to: '2022-09-01T09:16:29.553Z',
}));
data.query.timefilter.timefilter.getTime = jest.fn(() => {
timelineDataService.query.timefilter.timefilter.getTime = jest.fn(() => {
return { from: 'now-15m', to: 'now' };
});
data.query.timefilter.timefilter.getRefreshInterval = jest.fn(() => {
timelineDataService.query.timefilter.timefilter.getRefreshInterval = jest.fn(() => {
return { pause: true, value: 1000 };
});
data.query.timefilter.timefilter.calculateBounds = jest.fn(calculateBounds);
timelineDataService.query.timefilter.timefilter.calculateBounds = jest.fn(calculateBounds);
/** ************************************************* */

return {
Expand Down Expand Up @@ -251,7 +250,7 @@ export const createStartServicesMock = (
fieldFormats: fieldFormatsMock,
dataViewFieldEditor: indexPatternFieldEditorPluginMock.createStartContract(),
upselling: new UpsellingService(),
timelineFilterManager: mockTimelineFilterManager,
timelineDataService,
alerting,
} as unknown as StartServices;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,12 @@ export const convertToBuildEsQuery = ({
}
};

export interface CombinedQuery {
filterQuery: string | undefined;
kqlError: Error | undefined;
baseKqlQuery: Query;
}

export const combineQueries = ({
config,
dataProviders = [],
Expand All @@ -239,7 +245,7 @@ export const combineQueries = ({
filters = [],
kqlQuery,
kqlMode,
}: CombineQueries): { filterQuery: string | undefined; kqlError: Error | undefined } | null => {
}: CombineQueries): CombinedQuery | null => {
const kuery: Query = { query: '', language: kqlQuery.language };
if (isDataProviderEmpty(dataProviders) && isEmpty(kqlQuery.query) && isEmpty(filters)) {
return null;
Expand All @@ -254,6 +260,7 @@ export const combineQueries = ({
return {
filterQuery,
kqlError,
baseKqlQuery: kuery,
};
}

Expand Down Expand Up @@ -281,5 +288,6 @@ export const combineQueries = ({
return {
filterQuery,
kqlError,
baseKqlQuery: kuery,
};
};
45 changes: 36 additions & 9 deletions x-pack/plugins/security_solution/public/plugin_services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import type { AppMountParameters, CoreSetup, CoreStart, PackageInfo } from '@kbn/core/public';
import { FilterManager, NowProvider, QueryService } from '@kbn/data-plugin/public';
import { NowProvider, QueryService } from '@kbn/data-plugin/public';
import type { DataPublicPluginStart, QueryStart } from '@kbn/data-plugin/public';
import { Storage } from '@kbn/kibana-utils-plugin/public';
import { initTelemetry, TelemetryService } from './common/lib/telemetry';
Expand All @@ -31,6 +31,7 @@ import type {
export class PluginServices {
private readonly telemetry: TelemetryService = new TelemetryService();
private readonly queryService: QueryService = new QueryService();
private readonly timelineQueryService: QueryService = new QueryService();
private readonly storage = new Storage(localStorage);
private readonly sessionStorage = new Storage(sessionStorage);

Expand Down Expand Up @@ -69,7 +70,13 @@ export class PluginServices {
{ prebuiltRulesPackageVersion: this.prebuiltRulesPackageVersion }
);

this.queryService?.setup({
this.queryService.setup({
uiSettings: coreSetup.uiSettings,
storage: this.storage,
nowProvider: new NowProvider(),
});

this.timelineQueryService.setup({
uiSettings: coreSetup.uiSettings,
storage: this.storage,
nowProvider: new NowProvider(),
Expand All @@ -92,6 +99,7 @@ export class PluginServices {

public stop() {
this.queryService.stop();
this.timelineQueryService.stop();
licenseService.stop();
}

Expand All @@ -105,12 +113,23 @@ export class PluginServices {

const { savedObjectsTaggingOss, ...plugins } = startPlugins;

const query = this.queryService.start({
uiSettings: coreStart.uiSettings,
storage: this.storage,
http: coreStart.http,
});
const customDataService = this.startCustomDataService(query, startPlugins.data);
const customDataService = this.startCustomDataService(
this.queryService.start({
uiSettings: coreStart.uiSettings,
storage: this.storage,
http: coreStart.http,
}),
startPlugins.data
);

const timelineDataService = this.startTimelineDataService(
this.timelineQueryService.start({
uiSettings: coreStart.uiSettings,
storage: this.storage,
http: coreStart.http,
}),
startPlugins.data
);

return {
...coreStart,
Expand All @@ -128,8 +147,8 @@ export class PluginServices {
contentManagement: startPlugins.contentManagement,
telemetry: this.telemetry.start(),
customDataService,
timelineDataService,
topValuesPopover: new TopValuesPopoverService(),
timelineFilterManager: new FilterManager(coreStart.uiSettings),
};
}

Expand All @@ -150,4 +169,12 @@ export class PluginServices {
customDataService.query.filterManager._name = 'customFilterManager';
return customDataService;
};

private startTimelineDataService = (query: QueryStart, data: DataPublicPluginStart) => {
// Used in the unified timeline
return {
...data,
query,
};
};
}
Loading

0 comments on commit aa1df6d

Please sign in to comment.