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

[Endpoint] use rbush to only render to DOM resolver nodes that are in view #68957

Merged

Conversation

kqualters-elastic
Copy link
Contributor

@kqualters-elastic kqualters-elastic commented Jun 11, 2020

Summary

This pr uses the rbush library to spatially index process nodes and lines within resolver. This is mostly implemented in 2 new selectors, indexedProcessNodePositionsAndEdgeLineSegments which calculates the bounding box for each process node and edge line segment and bulk inserts them into the tree, and visibleProcessNodePositionsAndEdgeLineSegments, which uses the current size of the resolver element and the current coordinates of the camera to render any items that are in the users view. This pr also adds a prop to the process event dot component to show if the process is terminated or still running.
Summarize your PR. If it involves visual changes include a screenshot or gif.
resolver_spatial_new

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@kqualters-elastic kqualters-elastic requested review from a team as code owners June 11, 2020 20:31
@kqualters-elastic kqualters-elastic added the Feature:Endpoint Elastic Endpoint feature label Jun 11, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

@kqualters-elastic kqualters-elastic added Feature:Resolver Security Solution Resolver feature release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels Jun 11, 2020
const processesToIndex: IndexedProcessNode[] = [];
const edgeLineSegmentsToIndex: IndexedEdgeLineSegment[] = [];
for (const [processEvent, position] of positions) {
const transformedPosition = applyMatrix3(position, dataSelectors.isometricTransformMatrix);
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ This is entirely for your amusement and not really a serious a suggestion of any kind, but I thought you would enjoy it: https://gist.github.com/bkimmel/054eb12d9f13ce8bece83d31de20a333

const indexedProcessTree = indexedProcessTreeFactory(
dataSelectors.graphableProcesses(state.data)
);
const processNodeViewWidth = 360;
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 Let's find some way to share these with the node, maybe export from here and import there.

function (results: DataState['results']) {
return new Set(
results.filter(isTerminatedProcess).map((terminatedEvent) => {
if (event.isLegacyEvent(terminatedEvent)) {

This comment was marked as resolved.

This comment was marked as resolved.

@@ -60,7 +64,7 @@ export function hasError(state: DataState) {
* We can multiply both of these matrices to get the final transformation below.
*/
/* prettier-ignore */
const isometricTransformMatrix: Matrix3 = [
export const isometricTransformMatrix: Matrix3 = [
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO, why is this exported. what else needs to know about the graph layout logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kqualters-elastic can export be removed?

};
processesToIndex.push(indexedEvent);
}
for (const edgeLineSegment of edgeLineSegments) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like most of the logic in this function is duplicated in processNodePositionsAndEdgeLineSegments.
Maybe instead, processNodePositionsAndEdgeLineSegments should return a function that takes an AABB and returns a the matching nodes and edge line segments. Then visibleProcessNodePositionsAndEdgeLineSegments (in this file) can get the AABB like it already does, and call processNodePositionsAndEdgeLineSegments.

Reasoninig:

  1. Only selectors/data will need to know about the graph layout. This is good because the graph layout will change, and possibly end up being dynamic (in the future.)
  2. No code duplication. This code in data/selectors seems to be duplicated here

@@ -122,3 +132,93 @@ function composeSelectors<OuterState, InnerState, ReturnValue>(
): (state: OuterState) => ReturnValue {
return (state) => secondSelector(selector(state));
}

const spatiallyIndexedEntities = createSelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this only needs stuff from the data selectors, I think this should be moved to the data selectors. And processNodePositionsAndEdgeLineSegments can probably be made private. Nothing outside of data needs to be exposed to the underlying structure of the graph. The interface can just be

(query: AABB) => Entities[]

maxX,
maxY,
});
const visibleProcessNodePositions = visibleEntities.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not return the the entire array and let the react code deal with it. in react

visibleEntities.map(entity => {
  if (entity.type === 'edge') {
    // components
  } else {
    // different components
  }
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on past PR feedback I've gotten, I think the indication here would be to keep it as it is:
#64121 (comment)

);

export const visibleProcessNodePositionsAndEdgeLineSegments = createSelector(
spatiallyIndexedEntities,
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ I think we talked about this before, but we might need to think about selecting in the "active" and "selected" nodes from the UI (they may or may not be the same) and also possibly the ones "adjacent" to them as designated by the processAdjacency (sp?) selector. The reason is, I'm not sure how it would play out to have those things point (via aria-flowto and other mechanics) to nodes that "don't exist".

Would it be possible/good to make this like spatiallyIndexedAndActiveUIEntities?

Even though this is kind of a big issue, I'm marking it with a ❔ because I don't think it should stand in the way of merging this PR (and we have an issue on the board to deal with a11y stuff for 7.9 that we could cover this under. I'd ask that if you decide not to look into this, you add a comment to ( https://github.com/elastic/endpoint-app-team/issues/289 ) just so we know to track it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd note that in most cases this shouldn't add any more nodes to render. The active one should always be centered and when you're zoomed out enough, at least the up,left, and right ones should already be visible. Sometimes the selected one might be outside the viewbox (after we implement more a11y fixes) so you could be looking at maybe a worst case of 5 (selected + 4 directions) + 5 (active + maybe 4 directions) == 10 extras.

adjacentNodeMap={adjacentNodeMap}
relatedEvents={relatedEvents.get(processEvent)}
isProcessTerminated={terminatedProcesses.has(entity_id)}
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ very 🆒

@@ -340,7 +349,9 @@ const ProcessEventDotComponents = React.memo(
})
| null;
} = React.createRef();
const { cubeSymbol, labelBackground, descriptionText } = nodeAssets[nodeType(event)];
const { cubeSymbol, labelBackground, descriptionText } = nodeAssets[
nodeType(isProcessTerminated, isProcessOrigin)
Copy link
Contributor

Choose a reason for hiding this comment

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

ℹ️ nice

adjacentNodeMap={adjacentNodeMap}
relatedEvents={relatedEvents.get(processEvent)}
isProcessTerminated={terminatedProcesses.has(entity_id)}
isProcessOrigin={false}
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Where/how does it get the kind where isProcessOrigin={true} ?

Copy link
Contributor

@bkimmel bkimmel left a comment

Choose a reason for hiding this comment

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

I have a few questions, but nothing that should stand in the way of merging this great work.

return new Set(
results.filter(isTerminatedProcess).map((terminatedEvent) => {
if (event.isLegacyEvent(terminatedEvent)) {
return terminatedEvent.endgame.unique_pid;
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

Choose a reason for hiding this comment

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

@kqualters-elastic kqualters-elastic marked this pull request as draft June 17, 2020 07:22
},
index
) => (
<EdgeLine
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the metadata for the elapsed time was removed on the rebase probably

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, did not intend to do that will add back.

@@ -60,7 +64,7 @@ export function hasError(state: DataState) {
* We can multiply both of these matrices to get the final transformation below.
*/
/* prettier-ignore */
const isometricTransformMatrix: Matrix3 = [
export const isometricTransformMatrix: Matrix3 = [
Copy link
Contributor

Choose a reason for hiding this comment

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

@kqualters-elastic can export be removed?

@@ -119,7 +146,7 @@ export const graphableProcesses = createSelector(
* H
*
*/
function widthsOfProcessSubtrees(indexedProcessTree: IndexedProcessTree): ProcessWidths {
export function widthsOfProcessSubtrees(indexedProcessTree: IndexedProcessTree): ProcessWidths {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kqualters-elastic can export be removed?

@@ -150,7 +177,7 @@ function widthsOfProcessSubtrees(indexedProcessTree: IndexedProcessTree): Proces
return widths;
}

function processEdgeLineSegments(
export function processEdgeLineSegments(
Copy link
Contributor

Choose a reason for hiding this comment

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

@kqualters-elastic can export be removed?

@@ -330,7 +358,7 @@ function* levelOrderWithWidths(
}
}

function processPositions(
export function processPositions(
Copy link
Contributor

Choose a reason for hiding this comment

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

can export be removed?

processNodePositions: visibleProcessNodePositions,
connectingEdgeLineSegments,
};
}, isEqual);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:
In models/vector2.ts add:

export function isEqual([x1, y1]: Vector2, [x2, y2]: Vector2): boolean {
  return x1 === x2 && y1 === y2
}

In 'models/aabb.ts` add:

import * as vector2 from './vector2';
export function isEqual(a: AABB, b: AABB): boolean {
  return vector2.isEqual(a.minimum, b.minimum) && vector2.isEqual(a.maximum, b.maximum);
}

Then replace isEqual with aabb.isEqual.

{processNodePositions.map(({ entity, position }, index) => {
const adjacentNodeMap = processToAdjacencyMap.get(entity);
const {
process: { entity_id },
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the entityID method here instead

@@ -0,0 +1,145 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add animation tests to this?

Comment on lines 185 to 196
export const visibleProcessNodePositionsAndEdgeLineSegments = createSelector(
indexedProcessNodesAndEdgeLineSegments,
currentBoundingBox,
function (
/* eslint-disable no-shadow */
indexedProcessNodesAndEdgeLineSegments,
currentBoundingBox
/* eslint-enable no-shadow */
) {
return indexedProcessNodesAndEdgeLineSegments(currentBoundingBox);
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const visibleProcessNodePositionsAndEdgeLineSegments = createSelector(
indexedProcessNodesAndEdgeLineSegments,
currentBoundingBox,
function (
/* eslint-disable no-shadow */
indexedProcessNodesAndEdgeLineSegments,
currentBoundingBox
/* eslint-enable no-shadow */
) {
return indexedProcessNodesAndEdgeLineSegments(currentBoundingBox);
}
);
export const visibleProcessNodePositionsAndEdgeLineSegments = createSelector(
indexedProcessNodesAndEdgeLineSegments,
boundingBox,
function (
/* eslint-disable no-shadow */
indexedProcessNodesAndEdgeLineSegments,
boundingBox
/* eslint-enable no-shadow */
) {
return (time: number) => indexedProcessNodesAndEdgeLineSegments(boundingBox(time));
}
);

store.dispatch(action);
store.dispatch(cameraAction);
});
it('the visibleProcessNodePositions list should include all lines', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ I think you intended list should include all lines to be list should include all _processNodePositions

@@ -704,3 +717,22 @@ export const ProcessEventDot = styled(ProcessEventDotComponents)`
color: white;
}
`;

const processTypeToCube: Record<ResolverProcessType, keyof NodeStyleMap> = {
Copy link
Contributor

@bkimmel bkimmel Jun 24, 2020

Choose a reason for hiding this comment

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

❔ This and nodeType below got moved to /view/assets on master. Did you intend to duplicate them here?

Copy link
Contributor

@bkimmel bkimmel left a comment

Choose a reason for hiding this comment

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

Couple more questions after recent changes, but my thumb stands.

import { AABB } from '../types';

/**
* Return a boolean indicating if 2 vector objects are equal.
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ Some explanation of what AABB represents in the comments could be of help to a reader.

Copy link
Contributor

Choose a reason for hiding this comment

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

So far i've tried to document the type:

This way, you can browse the types file and see what every concept we model is.
Also, your editor can show you those comments on hover usually:
e.g. in mine:
image

Comment on lines +573 to +575
const processNodeViewWidth = 720;
const processNodeViewHeight = 240;
const lineSegmentPadding = 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const processNodeViewWidth = 720;
const processNodeViewHeight = 240;
const lineSegmentPadding = 30;
// Make sure these numbers are big enough to cover the process nodes at all zoom levels.
// NB: the process nodes don't extend equally in all directions from their center point.
const processNodeViewWidth = 720;
const processNodeViewHeight = 240;
const lineSegmentPadding = 30;

it('the visibleProcessNodePositions list should only include 2 nodes', () => {
const { processNodePositions } = visibleProcessNodePositionsAndEdgeLineSegments(
store.getState()
)(Date.now());
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't an animation running, you can just use 0. that eliminates the code smell of side effects

it('the visibleProcessNodePositions list should only include 2 nodes', () => {
const { processNodePositions } = visibleProcessNodePositionsAndEdgeLineSegments(
store.getState()
)(Date.now());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
)(Date.now());
)(0);

it('the visibleEdgeLineSegments list should only include 1 lines', () => {
const { connectingEdgeLineSegments } = visibleProcessNodePositionsAndEdgeLineSegments(
store.getState()
)(Date.now());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
)(Date.now());
)(0);

store.dispatch(action);
store.dispatch(cameraAction);
});
it('the visibleProcessNodePositions list should include all lines', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO, kq to do something better

it('the visibleProcessNodePositions list should include all lines', () => {
const { processNodePositions } = visibleProcessNodePositionsAndEdgeLineSegments(
store.getState()
)(Date.now());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
)(Date.now());
)(0);

});
store = createStore(resolverReducer, undefined);
});
describe('when rendering a large tree with a small viewport', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe('when rendering a large tree with a small viewport', () => {
describe('when rendering a tree with 2 events and with a small viewport', () => {

)(Date.now());
expect(processNodePositions.length).toEqual(2);
});
it('the visibleEdgeLineSegments list should only include 1 lines', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('the visibleEdgeLineSegments list should only include 1 lines', () => {
it('the visibleEdgeLineSegments list should only include one edge line', () => {

expect(connectingEdgeLineSegments.length).toEqual(1);
});
});
describe('when rendering a large tree with a large viewport', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe('when rendering a large tree with a large viewport', () => {
describe('when rendering a tree with seven events and with a large viewport', () => {

dataSelectors.visibleProcessNodePositionsAndEdgeLineSegments
);

export const visibleProcessNodePositionsAndEdgeLineSegments = createSelector(
Copy link
Contributor

@oatkiller oatkiller Jun 24, 2020

Choose a reason for hiding this comment

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

Suggested change
export const visibleProcessNodePositionsAndEdgeLineSegments = createSelector(
/**
* 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 animated. So in order to get the currently visible entities, we need
to pass in time.
*/
export const visibleProcessNodePositionsAndEdgeLineSegments = createSelector(

@@ -142,6 +142,33 @@ export type CameraState = {
}
);

export type IndexedEntity = IndexedEdgeLineSegment | IndexedProcessNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export type IndexedEntity = IndexedEdgeLineSegment | IndexedProcessNode;
/**
* Wrappers around our internal types that make them compatible with `rbush`.
*/
export type IndexedEntity = IndexedEdgeLineSegment | IndexedProcessNode;

export interface IndexedProcessNode extends BBox {
type: 'processNode';
entity: ResolverEvent;
position: Vector2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
position: Vector2;

maxX,
maxY,
});
const visibleProcessNodePositions = entities.filter(
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO, map these back to the non-indexed versions. This encapsulates the internal indexed structure. This is good because we don't actually need that structure, rbush can be configured to work w/ our AABB type.

) => (
<EdgeLine
edgeLineMetadata={metadata}
key={index}
Copy link
Contributor

Choose a reason for hiding this comment

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

Because rbush might return these entities in a non-stable order, we should use a stable key. This will help react reconcile old and new components. Since this runs every frame.... :)

@@ -51,6 +51,7 @@ const PanelContent = memo(function PanelContent() {
const urlSearch = history.location.search;
const dispatch = useResolverDispatch();

const { timestamp } = useContext(SideEffectContext);
Copy link
Contributor

Choose a reason for hiding this comment

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

solid change

@@ -364,14 +376,15 @@ const ProcessEventDotComponents = React.memo(
| null;
} = React.createRef();
const { colorMap, nodeAssets } = useResolverTheme();
const processType = nodeType(isProcessTerminated, isProcessOrigin);
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO, delete old nodeType from assets

Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

fired

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
securitySolution 777 +1 776

History

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

@kqualters-elastic kqualters-elastic merged commit 9ebf41c into elastic:master Jun 26, 2020
@kqualters-elastic kqualters-elastic deleted the spatial-index-resolver-nodes branch June 26, 2020 13:42
kqualters-elastic added a commit that referenced this pull request Jun 29, 2020
…are in view (#68957) (#70072)

* [Endpoint] use rbush to only render resolver nodes that are in view in the DOM

* Add related events code back

* Change processNodePositionsAndEdgeLineSegments selector to return a function that takes optional bounding box

* Refactor selectors to not break original, and not run as often

* Memoize rtree search selector, fix tests

* Update node styles to use style hook, update jest tests

* Fix type change issue in jest test

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Endpoint Elastic Endpoint feature Feature:Resolver Security Solution Resolver feature release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants