Resolver refactoring #70312

oatkiller · 2020-06-30T12:51:46Z

Summary

Hey y'all.
There's a few subjective changes here. Feel free to say 'I liked it that way.'

In my last PR I removed all usages of a field in DataState called relatedEventStats. I forgot to remove the field itself. This PR removes the field.
Move the calculation of the 'total' related events to the selector. NB: this is still calculated in the view because it was duplicated to begin with. I'll try to fix this up in my next PR.
Replace magFactorX (the first element of the projection matrix) with xScale for consistency with naming elsewhere in resolver.
Refactor calculation of actionableButtonsTopOffset for readability
the middleware had a call to dispatch in the try...catch used for HTTP request control flow. If dispatch threw an error due to code in the reducer or middleware, the error would be treated the same as a HTTP error (e.g. returning 500 from the server.) Moved the dispatch out of try...catch so it can be handled in the same way as other runtime errors. NB: we should look to replace our standard try...catch approach with something better.
The component ProcessEventListNarrowedByType was in a module called panels/panel_content_related_list.tsx. This moves it to panels/process_event_list_narrowed_by_type for readability

Checklist

Just refatoring.

click around the app, stuff still seems to work

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-06-30T12:53:08Z

Pinging @elastic/endpoint-data-visibility-team (Team:Endpoint Data Visibility)

elasticmachine · 2020-06-30T12:53:09Z

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

oatkiller · 2020-06-30T13:03:45Z

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

+ * Return the 'total' stats (whatever that means) for a process by entity ID
+ * TODO, how does this work for legacy events?
+ */
+export const relatedEventTotalForProcess: (


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

Found it in the other copy of the code: https://github.com/oatkiller/kibana/blob/resolver-refactoring/x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_related_counts.tsx#L54

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?

@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.

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

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.

oatkiller · 2020-06-30T13:05:14Z

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

+ return null;
+ }
+ let total = 0;
+ for (const value of Object.values(stats.events.byCategory)) {


Is events.byCategory a structure we made up?

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, }

Also cleanup related events view code for readability

I find this more readable.

We use try...catch for HTTP control flow logic. We don't want to treat runtime errors the same as HTTP errors though. Move `dispatch` call out of that try catch.

michaelolo24

nice cleanup

michaelolo24 · 2020-06-30T13:20:22Z

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

- break;
- }
+ const actionableButtonsTopOffset =
+ (isShowingEventActions ? 3.5 : isShowingDescriptionText ? 1 : 21) * xScale + 5;


Nice, you don't have anything against nested ternary's from a readability standpoint?

wanted your thoughts on this (esp. since you wrote the original.)

I found the original a lot to read due to boilerplate. Specifically, reading actionableButtonsTopOffset vs actionalButtonsBaseTopOffset was hard.

The repeated part of actionalButtonsBaseTopOffset + and * magFactorX required me to visually compared the 3 versions to see that they were the same.

I didn't realize at first that this was using true in the switch statement.

I haven't read a switch statement since at least 2008 :p (but seriously)

part of my motivation to make this terse is because the function it is in is long, and has many variables that aren't related to each other. That isn't the best motivation to make something terse. We could move the logic out of the function instead.

So maybe we keep it less terse? Eventually the function its in will get cleaned up.
Or maybe we keep it less terse and move it to an external fn?

agreed, makes sense

also makes sense

I like to throw in the occasional switch every now and again 😂

^^

Even if it's terse, it's still readable, so no need to revert any of it. I like moving it into an external function though because it will be a better separation of concerns and eventually it can probably be removed altogether in favor of mostly pure css.

jonathan-buttner · 2020-06-30T14:19:28Z

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

+ return () => null;
+ }
+ return (event: ResolverEvent) => {
+ const nodeStats = statsMap.get(uniquePidForProcess(event));


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.

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

jonathan-buttner · 2020-06-30T14:22:03Z

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

+ * Return the 'total' stats (whatever that means) for a process by entity ID
+ * TODO, how does this work for legacy events?
+ */
+export const relatedEventTotalForProcess: (


@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.

jonathan-buttner · 2020-06-30T14:23:23Z

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

+ return null;
+ }
+ let total = 0;
+ for (const value of Object.values(stats.events.byCategory)) {


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, }

bkimmel

This ruptures a lot of things we're using in other places (counts, stats, etc.) I need a little time to understand this.

oatkiller · 2020-06-30T15:28:36Z

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

@@ -9,7 +9,6 @@ import { DataState } from '../../types';
 import { ResolverAction } from '../actions';

 const initialState: DataState = {
- relatedEventsStats: new Map(),


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

See: https://github.com/elastic/kibana/pull/69728/files#diff-b013c0d85f67056fb127d2247c99a538L78

kqualters-elastic · 2020-06-30T16:42:01Z

...lugins/security_solution/public/resolver/view/panels/process_event_list_narrowed_by_type.tsx

@@ -97,6 +97,7 @@ export const ProcessEventListNarrowedByType = memo(function ProcessEventListNarr
 }) {
 const processName = processEvent && event.eventName(processEvent);
 const processEntityId = event.entityId(processEvent);
+


I think more than just this file have a file name that doesn't line up with the component name. Would probably make sense to fix those as well

I'm changing them as I find 'em. I don't think we need a rule that says '1:1 relationship between components and filenames.' That said, if a file has just one export, which is a component, it would be nice to name the component and the file the same.

kqualters-elastic · 2020-06-30T16:42:40Z

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

@@ -24,7 +24,7 @@ import { useResolverDispatch } from './use_resolver_dispatch';
 import * as event from '../../../common/endpoint/models/event';
 import { ResolverEvent, ResolverNodeStats } from '../../../common/endpoint/types';
 import { SideEffectContext } from './side_effect_context';
-import { ProcessEventListNarrowedByType } from './panels/panel_content_related_list';
+import { ProcessEventListNarrowedByType } from './panels/process_event_list_narrowed_by_type';


this file should probably be panels/index.tsx instead of just panel.tsx

🔴 That's not a list of process events - it's a list of related events like (registry creation, file creation, etc.). Also, if we're changing the name of one of the panel content components, we should change the prefix on all of them.

I gave it the wrong name from the export -> The file name should stay the same, the export name should change,

Maybe something like relatedEventListNarrowedByType ?

I'll revert this part of the PR.

oatkiller · 2020-06-30T17:49:50Z

This ruptures a lot of things we're using in other places (counts, stats, etc.) I need a little time to understand this.

Per our discussion, I think I've addressed your concerns. Let me know if I can do anything else.

bkimmel · 2020-06-30T18:04:25Z

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

@@ -177,11 +179,11 @@ type EventDisplayName = typeof displayNameRecord[keyof typeof displayNameRecord]
 typeof unknownEventTypeMessage;

 /**
- * Take a gross `schemaName` and return a beautiful translated one.


ℹ️ No fun allowed 📏

Just trying to get clarity.

bkimmel

I saw a couple things that look a little "off" there, but I don't want to hold this up.

bkimmel · 2020-06-30T18:30:24Z

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

@@ -4,14 +4,16 @@
 * you may not use this file except in compliance with the Elastic License.


🆒 Thank you for not changing the name of process_event_dot . I have an attachment to the name. It's a nice reminder that everything starts as a dot.

To @kqualters-elastic point, we could probably improve it in the future by making it more like a dot and less like a giant thing with a bunch of code in it but 🤷

oatkiller · 2020-06-30T19:27:51Z

took out the part about changing file names. lets do it in a follow up w/ @bkimmel's help

oatkiller · 2020-06-30T19:32:51Z

@bkimmel lets follow up re: anything 'off'. Otherwise I'll just keep doing more of it.

kibanamachine · 2020-06-30T21:08:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 54043fd

Build metrics

✅ unchanged

History

💛 Build #57484 was flaky 662cef4

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

* remove unused piece of state * Move related event total calculation to selector * rename xScale * remove `let` * Move `dispatch` call out of HTTP try-catch

…-based-rbac * upstream/master: (38 commits) Move logger configuration integration test to jest (elastic#70378) Changes observability plugin codeowner (elastic#70439) update (elastic#70424) [Logs UI] Avoid CCS-incompatible index name resolution (elastic#70179) Enable "Explore underlying data" actions for Lens visualizations (elastic#70047) Initial work on uptime homepage API (elastic#70135) expressions indexPattern function (elastic#70315) [Discover] Deangularization context error message refactoring (elastic#70090) [Lens] Add "no data" popover (elastic#69147) [Lens] Move chart switcher over (elastic#70182) chore: add missing mjs extension (elastic#70326) [Lens] Multiple y axes (elastic#69911) skip flaky suite (elastic#70386) fix bug to add timeline to case (elastic#70343) [QA][Code Coverage] Drop catchError and use try / catch instead, (elastic#69198) [QA] [Code Coverage] Integrate with Team Assignment Pipeline and Add Research and Development Indexes and Cluster (elastic#69348) [Metrics UI] Add context.reason and alertOnNoData to Inventory alerts (elastic#70260) Resolver refactoring (elastic#70312) [Ingest Manager] Fix agent ack after input format change (elastic#70335) [eslint][ts] Enable prefer-ts-expect-error (elastic#70022) ...

* remove unused piece of state * Move related event total calculation to selector * rename xScale * remove `let` * Move `dispatch` call out of HTTP try-catch

remove unused piece of state

f584bd4

oatkiller requested review from a team as code owners June 30, 2020 12:51

oatkiller added Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Data Visibility Team managing the endpoint resolver v7.9.0 v8.0.0 labels Jun 30, 2020

oatkiller assigned bkimmel, michaelolo24 and kqualters-elastic Jun 30, 2020

oatkiller commented Jun 30, 2020

View reviewed changes

oatkiller force-pushed the resolver-refactoring branch from e899fed to d2cf840 Compare June 30, 2020 13:08

oatkiller added 4 commits June 30, 2020 09:10

Move related event total calculation to selector

80119f3

Also cleanup related events view code for readability

rename xscale

e1cfb70

remove let

f4a26da

I find this more readable.

Move dispatch call out of HTTP try-catch

54043fd

We use try...catch for HTTP control flow logic. We don't want to treat runtime errors the same as HTTP errors though. Move `dispatch` call out of that try catch.

oatkiller force-pushed the resolver-refactoring branch from d2cf840 to 662cef4 Compare June 30, 2020 13:10

michaelolo24 approved these changes Jun 30, 2020

View reviewed changes

kqualters-elastic approved these changes Jun 30, 2020

View reviewed changes

jonathan-buttner reviewed Jun 30, 2020

View reviewed changes

bkimmel suggested changes Jun 30, 2020

View reviewed changes

oatkiller commented Jun 30, 2020

View reviewed changes

kqualters-elastic reviewed Jun 30, 2020

View reviewed changes

bkimmel reviewed Jun 30, 2020

View reviewed changes

bkimmel approved these changes Jun 30, 2020

View reviewed changes

bkimmel reviewed Jun 30, 2020

View reviewed changes

oatkiller force-pushed the resolver-refactoring branch from 662cef4 to 54043fd Compare June 30, 2020 19:27

oatkiller merged commit 893525c into elastic:master Jun 30, 2020

oatkiller deleted the resolver-refactoring branch June 30, 2020 21:32

Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020

Resolver refactoring (elastic#70312)

2e2550a

* remove unused piece of state * Move related event total calculation to selector * rename xScale * remove `let` * Move `dispatch` call out of HTTP try-catch

oatkiller mentioned this pull request Jul 1, 2020

[7.x] Resolver refactoring (#70312) #70443

Merged

oatkiller pushed a commit that referenced this pull request Jul 1, 2020

Resolver refactoring (#70312) (#70443)

487f34d

* remove unused piece of state * Move related event total calculation to selector * rename xScale * remove `let` * Move `dispatch` call out of HTTP try-catch

		@@ -4,14 +4,16 @@
		* you may not use this file except in compliance with the Elastic License.

Resolver refactoring #70312

Resolver refactoring #70312

Conversation

oatkiller commented Jun 30, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jun 30, 2020

elasticmachine commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelolo24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatkiller Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

michaelolo24 Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkimmel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatkiller commented Jun 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkimmel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatkiller commented Jun 30, 2020

oatkiller commented Jun 30, 2020

kibanamachine commented Jun 30, 2020

💚 Build Succeeded

Build metrics

History

oatkiller commented Jun 30, 2020 •

edited

Loading

oatkiller Jun 30, 2020 •

edited

Loading

michaelolo24 Jun 30, 2020 •

edited

Loading