[Security Solution][Timeline] Rebuild nested fields structure from fields response #96187

rylnd · 2021-04-03T03:22:20Z

Summary

Adds our threat indicator fields to TIMELINE_EVENTS_FIELDS so as to always be requested for events/all and eql queries
Adds parsing logic to build the original objects from a nested fields response
- specifically, each nested field builds up a "slice" of the full object for that particular field, and these are all merged together
- Recursive and works with any level of nesting
  - for ES storage/performance reasons, I would not expect that anyone has more than 1 level of nested objects, or maybe two, but as mentioned I don't think this will be a bottleneck

TODO

Add *Ecs types for threat fields?
- this work is actually done in the subsequent PR consuming this format, [Security Solution][Detections] Adds Nested CTI row renderer #96275
More 👀 as this data feeds timeline and alerts table
Verify performance; see section and comments below

Performance characteristics

TL;DR this path should not be an issue.

Rationale (please verify @XavierM @FrankHassanabad):

The function analogous to the one being introduced here (buildObjectForFieldPath), getValuesFromFields, was meant to mimic the performance improvements in formatIndexFields
formatFirstFields needs to be performant as it operates on a potentially huge list of index fields from all data sources
formatTimelineData (the caller of the code in this PR), however, operates only on requested fields; typically a subset of ECS and those we need for various SecSol pages
- I believe the user can add custom fields via e.g. the columns UI, but I do not believe there's a way to request fields on the same order of magnitude as formatIndexFields would receive

Regarding the specific performance of buildObjectFromFields, it's invoked once for each requested field; see cab30d4 for more details.

Why?

Given alerts with nested fields like threat.indicator:

{
  "threat":{
    "indicator":[
      {
        "matched":{
          "atomic":[
            "matched_atomic"
          ],
          "field":[
            "matched_field",
            "other_matched_field"
          ]
        }
      },
      {
        "matched":{
          "atomic":[
            "matched_atomic_2"
          ],
          "field":[
            "matched_field_2"
          ]
        }
      }
    ]
  }
}

we currently have two differing needs for nested fields in the UI:

flattened leaf values, for e.g. draggables in the Alert Details table
- e.g. threat.indicator.matched.field: ["matched_field", "other_matched_field", "matched_field_2"]
the original objects to preserve indicator independence
- necessary for row renderer to display each "threat match" correctly
- necessary for our new CTI tabs to display indicator fields correctly

#92025 did the work in 7.12 to give us 1. above. While there are some minor issues (only supports one level of nested fields, object values are stringified), I believe this is sufficient for our immediate needs, i.e.: answering "Which values populated this field?" and the ability to investigate those values in timeline.

However, search strategies were previously missing the structure described in 2. Our initial CTI tabs work solved this problem by JSON.parseing the stringified threat.indicator object returned as part of 1., but this is blocking work that should be done by the backend if possible, and is not feasible for a repeated task such as row rendering.

Thus, this PR: the logic of 1. was left relatively untouched, so the table view should continue to function as before. The new object structure is included as part of the .ecs property of a given node in the response, and can thus be consumed by e.g. row renderers without further changes. Previously, requesting nested fields in this section would result in an incorrect structure, with single objects/keys and empty array values.

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

* Always requests TIMELINE_CTI_FIELDS as part of request This only works for one level of nesting; will be extending tests to allow for multiple levels momentarily.

This is a recursive implementation, but recursion depth is limited to the number of levels of nesting, with arguments reducing in size as we go (i.e. logarithmic)

* Order short-circuiting conditions by cost, ascending * Simplify object building for non-nested objects from fields * The non-nested case is the same as the base recursive case, so always call our recursive function if building from .fields * Simplify getNestedParentPath * We can do a few simple string comparison rather than building up multiple strings/arrays * Don't call getNestedParentPath unnecessarily, only if we have a field

By definition, nestedParentFieldName can never be equal to fieldName, which means there are only two branches here.

Each top-level field value can be either an array of leaf values (unknown[]), or an array of nested fields.

If fieldName is null or undefined, there is no reason to search for it in dataFields. Looking through the git history this looks to be dead code as a result of refactoring, as opposed to a legitimate bugfix, so I'm removing it.

rylnd · 2021-04-03T03:23:38Z

...plugins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.test.ts

          },
          {
-            field: 'threat.indicator.matched.field',
-            value: ['matched_field', 'matched_field_2'],
+            field: 'source.geo.location',


The order here changed due to the addition of threat match fields to the ECS fields list, meaning it comes sooner than a requested field like this one (source.geo.location)

...plugins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.test.ts

rylnd · 2021-04-03T03:27:13Z

x-pack/plugins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.ts

+    return set({}, fieldPath, toStringArray(value));
+  }
+
+  return buildObjectRecursive(fieldPath, hit.fields);


@FrankHassanabad would you be able to pair on verifying the performance of this logic? I know this is a hot path and that we're doing setTimeouts and Promise.resolves elsewhere; I would appreciate a crash course in analzying/benchmarking/profiling this!

@rylnd and myself discussed this and the conclusion is:

I suggested for testing we can create some large index mappings and testing these functions manually to see if they need to do partitioning or not with a setTimeout.

We have had weird things in the past where once you add a partition through a setTimeout, team members from anywhere at any time can sometimes think it's a mistake (no matter how many docs and all CAPS! you put above it :-) ) and they will remove 'em and you have to watch them forever.

But also so we can see that everything functions as we expect today with a manual test as suggested. However right now we don't have an automated perf regression framework for injecting large index mappings, running a test against them and then seeing if we have de-graded performance of the regression of either NodeJS or the frontend UI.

There's more context about this in the Performance Characteristics section in the PR description 👍

I have PR who removed all the set from lodash in this function, if I remember correctly that show slowness in the past.

ecezalp · 2021-04-07T21:22:47Z

looks like I reviewed this change as a part of #96275, I was wondering if you were considering keeping this alive or just continuing the discussion under the row renderer change

* one was a test failure due to my modifying mock data * one may have been a legitimate bug where we don't handle a hit without a fields response; I need to follow up with Xavier to verify.

elasticmachine · 2021-04-08T03:20:23Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2021-04-08T03:20:25Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

rylnd · 2021-04-08T03:23:08Z

...plugins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.test.ts

@@ -407,6 +407,14 @@ describe('#formatTimelineData', () => {
      });
    });

+    it('builds an object with no fields response', () => {


@XavierM I added this test and the accompanying null check because the parseEqlResponse unit tests were feeding in events without a fields property and failing. It certainly seems plausible for a hit not to have that property, hence the changes here, but I wanted to double-check it with you as well.

rylnd · 2021-04-08T03:24:33Z

@elasticmachine merge upstream

rylnd · 2021-04-12T15:50:09Z

@elasticmachine merge upstream

stephmilovic · 2021-04-12T16:04:49Z

.../security_solution/common/search_strategy/security_solution/matrix_histogram/events/index.ts

@@ -26,10 +26,12 @@ export interface EventsActionGroupData {
  doc_count: number;
 }

+export type Fields = Record<string, unknown[] | Fields[]>;


this type is referring to itself is that kosher? does it recurse? broke my brain a little bit

Yep, it's recursive! This was the most accurate representation of nested fields (and thus "fields" in general) I could think of. It appears to work with all our existing uses of the property without any changes because unknown[] is greedier than Fields[], but e.g. I needed to narrow in cases where we know it's gong to be the recursive form.

stephmilovic · 2021-04-12T16:53:55Z

...plugins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.test.ts

+      let nestedHit: EventHit;
+
+      beforeEach(() => {
+        // @ts-expect-error nestedHit is minimal
+        nestedHit = {
+          fields: {
+            'nested_1.foo': [
+              {
+                'nested_2.bar': [
+                  { leaf: ['leaf_value'], leaf_2: ['leaf_2_value'] },
+                  { leaf_2: ['leaf_2_value_2', 'leaf_2_value_3'] },
+                ],
+              },
+              {
+                'nested_2.bar': [
+                  { leaf: ['leaf_value_2'], leaf_2: ['leaf_2_value_4'] },
+                  { leaf: ['leaf_value_3'], leaf_2: ['leaf_2_value_5'] },
+                ],
+              },
+            ],
+          },
+        };
+      });


You do not need the let or beforeEach

Suggested change

let nestedHit: EventHit;

beforeEach(() => {

// @ts-expect-error nestedHit is minimal

nestedHit = {

fields: {

'nested_1.foo': [

{

'nested_2.bar': [

{ leaf: ['leaf_value'], leaf_2: ['leaf_2_value'] },

{ leaf_2: ['leaf_2_value_2', 'leaf_2_value_3'] },

],

},

{

'nested_2.bar': [

{ leaf: ['leaf_value_2'], leaf_2: ['leaf_2_value_4'] },

{ leaf: ['leaf_value_3'], leaf_2: ['leaf_2_value_5'] },

],

},

],

},

};

});

// @ts-expect-error nestedHit is minimal

const nestedHit: EventHit = {

fields: {

'nested_1.foo': [

{

'nested_2.bar': [

{ leaf: ['leaf_value'], leaf_2: ['leaf_2_value'] },

{ leaf_2: ['leaf_2_value_2', 'leaf_2_value_3'] },

],

},

{

'nested_2.bar': [

{ leaf: ['leaf_value_2'], leaf_2: ['leaf_2_value_4'] },

{ leaf: ['leaf_value_3'], leaf_2: ['leaf_2_value_5'] },

],

},

],

},

};

So this let/beforeEach is my paranoid "reset test data" pattern, meant to address the case where some function under test permutes its arguments. Consider the following scenario:

describe('permuting test data', () => { const a = ['foo']; it('works once', () => { expect(a).toEqual(['foo']); a.push('bar'); }); it('then breaks', () => { expect(a).toEqual(['foo']); }); });

Contrived example? Yes, we're not usually writing code with side effects like this, and should strive not to. However, it inevitably happens, and occasionally the solution is to "fix" the tests such that they work against the permuted data. And then we have both order-dependent and incorrect tests 😦 .

Given the fact that this suite was using that shared eventHit variable all over the place, and I had no idea how the code worked before writing these new tests, I opted to go this route and ensure that these new tests were "clean" and not subject to previous permutations.

stephmilovic

Thanks for cleaning this up, nested fields === 🤯 . LGTM 🚀

Conflicts: x-pack/plugins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.test.ts

rylnd · 2021-04-12T20:33:22Z

@elasticmachine merge upstream

kibanamachine · 2021-04-12T22:39:31Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #118964 failed f60a165
💛 Build #118798 was flaky bbbe168
💚 Build #118104 succeeded f9a598a
💔 Build #117147 failed 8143acd

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

cc @rylnd

…elds response (elastic#96187) * First pass at rebuilding nested object structure from fields response * Always requests TIMELINE_CTI_FIELDS as part of request This only works for one level of nesting; will be extending tests to allow for multiple levels momentarily. * Build objects from arbitrary levels of nesting This is a recursive implementation, but recursion depth is limited to the number of levels of nesting, with arguments reducing in size as we go (i.e. logarithmic) * Simplify parsing logic, perf improvements * Order short-circuiting conditions by cost, ascending * Simplify object building for non-nested objects from fields * The non-nested case is the same as the base recursive case, so always call our recursive function if building from .fields * Simplify getNestedParentPath * We can do a few simple string comparison rather than building up multiple strings/arrays * Don't call getNestedParentPath unnecessarily, only if we have a field * Simplify if branching By definition, nestedParentFieldName can never be equal to fieldName, which means there are only two branches here. * Declare/export a more accurate fields type Each top-level field value can be either an array of leaf values (unknown[]), or an array of nested fields. * Remove unnecessary condition If fieldName is null or undefined, there is no reason to search for it in dataFields. Looking through the git history this looks to be dead code as a result of refactoring, as opposed to a legitimate bugfix, so I'm removing it. * Fix failing tests * one was a test failure due to my modifying mock data * one may have been a legitimate bug where we don't handle a hit without a fields response; I need to follow up with Xavier to verify. Co-authored-by: Kibana Machine <[email protected]>

…elds response (#96187) (#96915) * First pass at rebuilding nested object structure from fields response * Always requests TIMELINE_CTI_FIELDS as part of request This only works for one level of nesting; will be extending tests to allow for multiple levels momentarily. * Build objects from arbitrary levels of nesting This is a recursive implementation, but recursion depth is limited to the number of levels of nesting, with arguments reducing in size as we go (i.e. logarithmic) * Simplify parsing logic, perf improvements * Order short-circuiting conditions by cost, ascending * Simplify object building for non-nested objects from fields * The non-nested case is the same as the base recursive case, so always call our recursive function if building from .fields * Simplify getNestedParentPath * We can do a few simple string comparison rather than building up multiple strings/arrays * Don't call getNestedParentPath unnecessarily, only if we have a field * Simplify if branching By definition, nestedParentFieldName can never be equal to fieldName, which means there are only two branches here. * Declare/export a more accurate fields type Each top-level field value can be either an array of leaf values (unknown[]), or an array of nested fields. * Remove unnecessary condition If fieldName is null or undefined, there is no reason to search for it in dataFields. Looking through the git history this looks to be dead code as a result of refactoring, as opposed to a legitimate bugfix, so I'm removing it. * Fix failing tests * one was a test failure due to my modifying mock data * one may have been a legitimate bug where we don't handle a hit without a fields response; I need to follow up with Xavier to verify. Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

rylnd added 6 commits April 2, 2021 16:15

First pass at rebuilding nested object structure from fields response

1cb213c

* Always requests TIMELINE_CTI_FIELDS as part of request This only works for one level of nesting; will be extending tests to allow for multiple levels momentarily.

Build objects from arbitrary levels of nesting

cab30d4

This is a recursive implementation, but recursion depth is limited to the number of levels of nesting, with arguments reducing in size as we go (i.e. logarithmic)

Simplify if branching

9f8bdad

By definition, nestedParentFieldName can never be equal to fieldName, which means there are only two branches here.

Declare/export a more accurate fields type

168612b

Each top-level field value can be either an array of leaf values (unknown[]), or an array of nested fields.

Remove unnecessary condition

8143acd

If fieldName is null or undefined, there is no reason to search for it in dataFields. Looking through the git history this looks to be dead code as a result of refactoring, as opposed to a legitimate bugfix, so I'm removing it.

rylnd added release_note:enhancement Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.13.0 Team: CTI labels Apr 3, 2021

rylnd self-assigned this Apr 3, 2021

rylnd commented Apr 3, 2021

View reviewed changes

...plugins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.test.ts Show resolved Hide resolved

rylnd commented Apr 3, 2021

View reviewed changes

rylnd mentioned this pull request Apr 6, 2021

[Security Solution][Detections] Adds Nested CTI row renderer #96275

Merged

14 tasks

Fix failing tests

e5aada2

* one was a test failure due to my modifying mock data * one may have been a legitimate bug where we don't handle a hit without a fields response; I need to follow up with Xavier to verify.

rylnd marked this pull request as ready for review April 8, 2021 03:20

rylnd requested a review from a team as a code owner April 8, 2021 03:20

rylnd requested review from XavierM, andrew-goldstein and stephmilovic April 8, 2021 03:20

rylnd commented Apr 8, 2021

View reviewed changes

Merge branch 'master' into rebuild_nested_fields_structure

f9a598a

Merge branch 'master' into rebuild_nested_fields_structure

bbbe168

stephmilovic reviewed Apr 12, 2021

View reviewed changes

stephmilovic approved these changes Apr 12, 2021

View reviewed changes

Merge branch 'master' into rebuild_nested_fields_structure

f60a165

Conflicts: x-pack/plugins/security_solution/server/search_strategy/timeline/factory/events/all/helpers.test.ts

Merge branch 'master' into rebuild_nested_fields_structure

9bc1ea6

rylnd added the v8.0.0 label Apr 12, 2021

rylnd merged commit 39f87f4 into elastic:master Apr 12, 2021

rylnd deleted the rebuild_nested_fields_structure branch April 12, 2021 22:52

rylnd mentioned this pull request Apr 13, 2021

[7.x] [Security Solution][Timeline] Rebuild nested fields structure from fields response (#96187) #96915

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Timeline] Rebuild nested fields structure from fields response #96187

[Security Solution][Timeline] Rebuild nested fields structure from fields response #96187

rylnd commented Apr 3, 2021 •

edited

Loading

rylnd Apr 3, 2021

rylnd Apr 3, 2021

FrankHassanabad Apr 12, 2021 •

edited

Loading

rylnd Apr 12, 2021

XavierM Apr 13, 2021

ecezalp commented Apr 7, 2021

elasticmachine commented Apr 8, 2021

elasticmachine commented Apr 8, 2021

rylnd Apr 8, 2021 •

edited

Loading

rylnd commented Apr 8, 2021

rylnd commented Apr 12, 2021

stephmilovic Apr 12, 2021

rylnd Apr 12, 2021

stephmilovic Apr 12, 2021

rylnd Apr 12, 2021

stephmilovic Apr 12, 2021

stephmilovic left a comment

rylnd commented Apr 12, 2021

kibanamachine commented Apr 12, 2021

[Security Solution][Timeline] Rebuild nested fields structure from fields response #96187

[Security Solution][Timeline] Rebuild nested fields structure from fields response #96187

Conversation

rylnd commented Apr 3, 2021 • edited Loading

Summary

TODO

Performance characteristics

Why?

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankHassanabad Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecezalp commented Apr 7, 2021

elasticmachine commented Apr 8, 2021

elasticmachine commented Apr 8, 2021

rylnd Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

rylnd commented Apr 8, 2021

rylnd commented Apr 12, 2021

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

stephmilovic left a comment

Choose a reason for hiding this comment

rylnd commented Apr 12, 2021

kibanamachine commented Apr 12, 2021

💚 Build Succeeded

Metrics [docs]

History

rylnd commented Apr 3, 2021 •

edited

Loading

FrankHassanabad Apr 12, 2021 •

edited

Loading

rylnd Apr 8, 2021 •

edited

Loading