[Resolver] aria-level and aria-flowto support enhancements by oatkiller · Pull Request #71777 · elastic/kibana

oatkiller · 2020-07-14T21:51:31Z

Summary

IndexedProcessTree now owns the concern of defining the order of siblings
IsometricTaxiLayout now owns the concept of ariaLevels
added datetime method to process_event model which returns a time in ms since unix epoch for the event
renamed some resolver selectors
added resolver selector: ariaLevel
added 'data' selector: followingSibling (used for aria-flowto)
added resolver selector ariaFlowtoNodeID which takes a nodeID, and returns its following sibling's node id (if that sibling is visible.) By only returning visible siblings, we ensure that aria-flowto will point to an html ID that is in the dom.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

This reverts commit 24c27ee.

oatkiller · 2020-07-14T21:59:45Z

x-pack/plugins/security_solution/public/resolver/store/data/selectors.ts

-    };
-  }
-);
+export const nodesAndEdgelines: (


oatkiller · 2020-07-14T22:00:24Z

x-pack/plugins/security_solution/public/resolver/store/selectors.ts

-    /* eslint-enable no-shadow */
-  ) {
-    return (time: number) => indexedProcessNodesAndEdgeLineSegments(boundingBox(time));
+export const visibleNodesAndEdgeLines = createSelector(nodesAndEdgelines, boundingBox, function (


oatkiller · 2020-07-14T22:01:45Z

x-pack/plugins/security_solution/public/resolver/view/map.tsx

                }
                isProcessTerminated={terminatedProcesses.has(processEntityId)}
                isProcessOrigin={false}
+                timeAtRender={timeAtRender}


when timestamp is used to get the state of things during a (possible) animation, we should use the same value for all calls in the frame (regardless of how much time has passed.) this is a hacky way to get around the issue. we should file a ticket for it.

oatkiller · 2020-07-14T22:02:04Z

x-pack/plugins/security_solution/public/resolver/view/process_event_dot.tsx

+    timeAtRender: number;
  }) => {
+    // This should be unique to each instance of Resolver
+    const htmlIDPrefix = 'resolver';


@kqualters-elastic we need use the documentLocationID here. that way we can have unique IDs.

oatkiller · 2020-07-14T22:04:03Z

x-pack/plugins/security_solution/public/resolver/view/process_event_dot.tsx

-    const selfId = adjacentNodeMap.self;
    const activeDescendantId = useSelector(selectors.uiActiveDescendantId);
    const selectedDescendantId = useSelector(selectors.uiSelectedDescendantId);
+    const nodeID = processEventModel.uniquePidForProcess(event);


slowly changing things over to use a string, which i'm calling nodeID, to define nodes.
process events aren't good because several process events could define a node (start event, terminate event, already running event, different events from other vendors.) also, strings are interned and primitives which is nice

oatkiller · 2020-07-14T22:04:49Z

x-pack/plugins/security_solution/public/resolver/view/process_event_dot.tsx

        payload: {
-          nodeId,
-          selectedProcessId: selfId,
+          nodeId: nodeHTMLID(nodeID),


we should be able to pass just the nodeID. the HTML id can be calculated from that.

oatkiller · 2020-07-14T22:05:33Z

x-pack/plugins/security_solution/public/resolver/view/process_event_dot.tsx

              color={labelButtonFill}
-              data-test-subject="nodeLabel"
              fill={isLabelFilled}
-              id={labelId}


same id and data-test-subj were being used for this button and its parent. the data-test-subj wasn't being used anyway.

kibanamachine · 2020-07-14T23:30:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dfe495f

Build metrics

✅ unchanged

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

x-pack/plugins/security_solution/public/resolver/models/indexed_process_tree/index.ts

michaelolo24 · 2020-07-15T14:25:09Z

x-pack/plugins/security_solution/public/resolver/models/indexed_process_tree/index.ts

-  for (const treeRoot of roots) {
-    traverseLevels(idToAdjacent.get(uniquePidForProcess(treeRoot))!);
+      // if either value is NaN, compare them differently
+      if (isNaN(first)) {


Out of curiosity, why would these ever be NaN or is this a "should never happen" scenario?

datetime tries to parse a Date from @timestamp (or somewhere else for legacy events.)
It then returns the getTime from that Date. If a date is 'Invalid Date', then getTime will return NaN.

maybe i should check for NaN in the body of datetime and return null instead.

oatkiller · 2020-07-15T15:29:44Z

x-pack/plugins/security_solution/common/endpoint/models/event.ts

 }

-export function eventId(event: ResolverEvent): string {
+export function eventId(event: ResolverEvent): number | undefined | string {


@jonathan-buttner this function was converting the serial event id to a string. if it was undefined or 0 it was being converted to ''. i don't know the reasoning, but i undid it. some BE code was effected. let me know if this is OK

@bkimmel or @kqualters-elastic may have thoughts here.

I think this is fine I left some comments about checking for undefined and not displaying the string 'undefined'.

oatkiller · 2020-07-15T15:30:33Z

x-pack/plugins/security_solution/public/resolver/models/process_event.ts

 * may return NaN if the timestamp wasn't present or was invalid.
 */
-export function datetime(passedEvent: ResolverEvent): number {
+export function datetime(passedEvent: ResolverEvent): number | null {


@michaelolo24 when the value isn't defined or can't be parsed this now returns null.

oatkiller · 2020-07-15T15:31:13Z

x-pack/plugins/security_solution/public/resolver/models/process_event.ts

+/**
+ * used to sort events
+ */
+export function orderByTime(first: ResolverEvent, second: ResolverEvent): number {


@jonathan-buttner this now does a tie breaker with an arbitrary comparison of event ID.

Nice! Thanks!

oatkiller · 2020-07-15T15:32:42Z

x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_related_list.tsx

          eventType: `${event.ecsEventType(resolverEvent)}`,
          name: event.descriptiveName(resolverEvent),
-          entityId,
+          entityId: String(entityId),


the behavior here is changed. Before, if the event was a legacy event and the id was undefined or 0this would be ''. Now it will be '0' or 'undefined'. I'm not sure that either behavior is correct.

If it's undefined I wonder if we should mark it as an empty string here. I would think '0' would be fine if that is what was sent by the sensor.

Hmm also the field name here is a bit misleading, we're setting entityId with eventId. Is that intentional?

Went to track down what its used for. its not used. removing it.

oatkiller · 2020-07-15T15:32:46Z

x-pack/plugins/security_solution/server/endpoint/routes/resolver/utils/pagination.ts

    const cursor = {
      timestamp: lastResult['@timestamp'],
-      eventID: eventId(lastResult),
+      eventID: String(eventId(lastResult)),


the behavior here is changed. Before, if the event was a legacy event and the id was undefined or 0this would be ''. Now it will be '0' or 'undefined'. I'm not sure that either behavior is correct.

Thanks for point this out. I think we'll want to check if it is undefined and set it to an empty string here. If 0 is returned we'll likely want to keep it as 0 because I think that's a valid value for the legacy events.

oatkiller · 2020-07-15T16:08:21Z

github isn't recognizing my pushes unless i close and open?

oatkiller · 2020-07-15T16:11:56Z

closing in favor of this: #71887

oatkiller added 11 commits July 14, 2020 14:16

WIP

f59409f

buttner test

4a1be8d

revert me, too ambitious

24c27ee

Revert "revert me, too ambitious"

f72c8e8

This reverts commit 24c27ee.

draft?

64173db

fix and add tests

3d2b411

test

e028c67

tests

e1dd2c2

fixup

b7c6246

fixup

bcafbec

tests

dfe495f

oatkiller requested review from bkimmel and kqualters-elastic July 14, 2020 21:51

oatkiller requested review from a team as code owners July 14, 2020 21:51

oatkiller added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 14, 2020

oatkiller changed the title ~~Resolver traverse levels~~ [Resolver] aria-level and aria-flowto support Jul 14, 2020

oatkiller changed the title ~~[Resolver] aria-level and aria-flowto support~~ [Resolver] aria-level and aria-flowto support enhancements Jul 14, 2020

oatkiller commented Jul 14, 2020

View reviewed changes

oatkiller added 7.10.0 v7.9.0 and removed v7.9.0 7.10.0 labels Jul 14, 2020

jonathan-buttner reviewed Jul 15, 2020

View reviewed changes

x-pack/plugins/security_solution/public/resolver/models/indexed_process_tree/index.ts Show resolved Hide resolved

x-pack/plugins/security_solution/public/resolver/models/indexed_process_tree/index.ts Outdated Show resolved Hide resolved

michaelolo24 reviewed Jul 15, 2020

View reviewed changes

fix sort

9662220

oatkiller closed this Jul 15, 2020

oatkiller reopened this Jul 15, 2020

oatkiller commented Jul 15, 2020

View reviewed changes

oatkiller closed this Jul 15, 2020

oatkiller mentioned this pull request Jul 15, 2020

Resolver traverse levels #71887

Merged

2 tasks

Conversation

oatkiller commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 14, 2020

💚 Build Succeeded

Build metrics

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oatkiller commented Jul 15, 2020

Uh oh!

oatkiller commented Jul 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oatkiller commented Jul 14, 2020 •

edited

Loading