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

Resolver refactoring #70312

Merged
merged 5 commits into from
Jun 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { DataState } from '../../types';
import { ResolverAction } from '../actions';

const initialState: DataState = {
relatedEventsStats: new Map(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The logic that used to populate this piece of state has been moved to the selector called relatedEventsStats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

relatedEvents: new Map(),
relatedEventsReady: new Map(),
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,3 +301,46 @@ export function databaseDocumentIDToAbort(state: DataState): string | null {
return null;
}
}

/**
* `ResolverNodeStats` for a process (`ResolverEvent`)
*/
const relatedEventStatsForProcess: (
state: DataState
) => (event: ResolverEvent) => ResolverNodeStats | null = createSelector(
relatedEventsStats,
(statsMap) => {
if (!statsMap) {
return () => null;
}
return (event: ResolverEvent) => {
const nodeStats = statsMap.get(uniquePidForProcess(event));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leaving the note here like we talked about before @oatkiller this function uniquePidForProcess will suffer from the same problem we talked about before where unique_pid will have collisions in the map just like the entityId function. We'll need to do the serialization or something for that function too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get something formally added to the team's backlog about this.

if (!nodeStats) {
return null;
}
return nodeStats;
};
}
);

/**
* The sum of all related event categories for a process.
*/
export const relatedEventTotalForProcess: (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What was the justification for this? I'd like to put it down in comments here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I understand what this is doing I see that this number doesn't match the number of events. Is there any point in showing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bkimmel you might want to comment here.

There is a difference between total events and the total amount of all the categories added up which is what is happening here. It really depends on what we want to communicate to the user. If we communicate that there are 5 total related events, when they look at the break down the values won't always added up to 5 because a single event could fall into multiple categories. So it could be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I'm not following is:
how does coming up w/ this number make things less confusing?

Copy link
Contributor

Choose a reason for hiding this comment

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

For a set of related events with the following ECS categories:
[DNS, File], [DNS, File], [Registry]
We break counts in submenus as:

DNS: 2
File: 2
Registry: 1

There are 2 choices then, for how to display the total: 5 or 3

If I'm not mistaken, I believe it was you, Robert, who suggested that 5 is less confusing (and I think everyone agreed). This is what counts to 5. There are implications in other places, but that's the crux of what's happening here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/shrug

state: DataState
) => (event: ResolverEvent) => number | null = createSelector(
relatedEventStatsForProcess,
(statsForProcess) => {
return (event: ResolverEvent) => {
const stats = statsForProcess(event);
if (!stats) {
return null;
}
let total = 0;
for (const value of Object.values(stats.events.byCategory)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is events.byCategory a structure we made up?

Copy link
Contributor

Choose a reason for hiding this comment

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

events.byCategory is what the backend returns. It's an object of number of events that fall in a particular category for example:

{
  'network': 3,
  'file': 2,
}

total += value;
}
return total;
};
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -43,24 +43,26 @@ export const resolverMiddlewareFactory: MiddlewareFactory = (context) => {
action.type === 'appDetectedMissingEventData'
) {
const entityIdToFetchFor = action.payload;
let result: ResolverRelatedEvents;
let result: ResolverRelatedEvents | undefined;
try {
result = await context.services.http.get(
`/api/endpoint/resolver/${entityIdToFetchFor}/events`,
{
query: { events: 100 },
}
);
} catch {
api.dispatch({
type: 'serverFailedToReturnRelatedEventData',
payload: action.payload,
});
}

if (result) {
api.dispatch({
type: 'serverReturnedRelatedEventData',
payload: result,
});
} catch (e) {
api.dispatch({
type: 'serverFailedToReturnRelatedEventData',
payload: action.payload,
});
}
}
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,14 @@ const indexedProcessNodesAndEdgeLineSegments = composeSelectors(
dataSelectors.visibleProcessNodePositionsAndEdgeLineSegments
);

/**
* Total count of related events for a process.
*/
export const relatedEventTotalForProcess = composeSelectors(
dataStateSelector,
dataSelectors.relatedEventTotalForProcess
);

/**
* Return the visible edge lines and process nodes based on the camera position at `time`.
* The bounding box represents what the camera can see. The camera position is a function of time because it can be
Expand Down
8 changes: 1 addition & 7 deletions x-pack/plugins/security_solution/public/resolver/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,7 @@
import { Store } from 'redux';
import { BBox } from 'rbush';
import { ResolverAction } from './store/actions';
import {
ResolverEvent,
ResolverNodeStats,
ResolverRelatedEvents,
ResolverTree,
} from '../../common/endpoint/types';
import { ResolverEvent, ResolverRelatedEvents, ResolverTree } from '../../common/endpoint/types';

/**
* Redux state for the Resolver feature. Properties on this interface are populated via multiple reducers using redux's `combineReducers`.
Expand Down Expand Up @@ -176,7 +171,6 @@ export interface VisibleEntites {
* State for `data` reducer which handles receiving Resolver data from the backend.
*/
export interface DataState {
readonly relatedEventsStats: Map<string, ResolverNodeStats>;
readonly relatedEvents: Map<string, ResolverRelatedEvents>;
readonly relatedEventsReady: Map<string, boolean>;
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export const ResolverMap = React.memo(function ({
projectionMatrix={projectionMatrix}
event={processEvent}
adjacentNodeMap={adjacentNodeMap}
relatedEventsStats={
relatedEventsStatsForProcess={
relatedEventsStats ? relatedEventsStats.get(entityId(processEvent)) : undefined
}
isProcessTerminated={terminatedProcesses.has(processEntityId)}
Expand Down
Loading