[Security Solution] Optimize field formatting server side #130915

stephmilovic · 2022-04-25T17:15:21Z

Summary

Server side field formatting optimization

Combines the two iterative functions that added category, description, and indexes (index alias) to each field. Optimization tested on 15k fields, async function runtime goes from ~700ms to ~300ms

elasticmachine · 2022-04-25T17:15:24Z

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

elasticmachine · 2022-04-25T17:15:24Z

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

stephmilovic · 2022-04-25T17:28:16Z

@elasticmachine merge upstream

kqualters-elastic · 2022-04-25T23:42:14Z

@stephmilovic can you add more context about what you are optimizing and why? Maybe also describe other places in this bit of code that could be potentially slow and improved a la #129861 ?

stephmilovic · 2022-04-26T13:45:22Z

@kqualters-elastic Basically we were mapping over the fields twice when it can be done in one go. I combined the logic from formatFirstFields and formatSecondFields so now we are only mapping over each field once instead of twice. It's not a huge performance boost, but does cut down iteration in this part of the code by half

stephmilovic · 2022-04-27T19:19:38Z

@elasticmachine merge upstream

michaelolo24 · 2022-04-28T14:24:11Z

x-pack/plugins/timelines/server/search_strategy/index_fields/index.ts

-  const fields = await formatFirstFields(beatFields, responsesFieldSpec, indexesAlias);
-  const secondFields = await formatSecondFields(fields);
-  return secondFields;
+  return formatFirstFields(beatFields, responsesFieldSpec, indexesAlias);


Quick question, what's the benefit of combining both of theses into one call?

Ignore this. Saw the comment from Kevin above late. My bad ✋🏾

x-pack/plugins/timelines/server/search_strategy/index_fields/index.ts

stephmilovic · 2022-05-02T14:50:12Z

@elasticmachine merge upstream

yctercero · 2022-05-02T19:10:40Z

x-pack/plugins/timelines/server/search_strategy/index_fields/index.ts

-  const fields = await formatFirstFields(beatFields, responsesFieldSpec, indexesAlias);
-  const secondFields = await formatSecondFields(fields);
-  return secondFields;
+  return formatFirstFields(beatFields, responsesFieldSpec, indexesAlias);


I think you can get rid of this and just call formatFirstFields directly where needed. Only seems to be used in this file right now anyways.

I don't think this is under the platform codeowners, but should it be under both? Would you consider it a part of sourcerer overall? If not, happy to leave it to @michaelolo24 :)

@stephmilovic, could you please include the codeowners file update to this PR?

I actually don't see this directory referenced in CODEOWNERS. Looking at the PR, it only pinged @elastic/security-threat-hunting-investigations. I think @yctercero might be confusing the ping from my other PR which is still awaiting platform review

Testing that other one now! I'd just been going through the both of your open PRs. It seems like this deals with fetching the data views/fields, so I wondered if it was part of the larger data views architecture. If it's purely timeline and not an area of code platform needs to worry about, then all good 👍🏽

yctercero

Thanks so much for jumping into the performance stuff here, Steph! Pulled down and tested before and after with 27K fields. Like you already mentioned in your comments,
I didn't see a huge performance change but see the code is cleaner. Had a few notes going through it.

Could we add some background to the PR description as to why we made this change, performance before/after? In case we somehow find we're slower we can go back and remember what our theory was in making a performance related change.
I don't think this is under the platform codeowners, but should it be under both? Would you consider it a part of sourcerer overall? If not, happy to leave it to @michaelolo24 :)
Since we're in this file, could we add docs to each of the functions in the file, giving a high level description of what each accomplishes? That could help in understanding which parts of the app they may affect.

YulNaumenko · 2022-05-02T20:01:42Z

I don't think this is under the platform codeowners, but should it be under both? Would you consider it a part of sourcerer overall? If not, happy to leave it to @michaelolo24 :)

@stephmilovic, could you please include the codeowners file update to this PR?

cla-checker-service · 2022-05-02T20:58:25Z

💚 CLA has been signed

stephmilovic · 2022-05-02T21:02:01Z

cla/check

stephmilovic · 2022-05-02T21:16:02Z

Thanks so much for jumping into the performance stuff here, Steph! Pulled down and tested before and after with 27K fields. Like you already mentioned in your comments, I didn't see a huge performance change but see the code is cleaner. Had a few notes going through it.

Could we add some background to the PR description as to why we made this change, performance before/after? In case we somehow find we're slower we can go back and remember what our theory was in making a performance related change.

I don't think this is under the platform codeowners, but should it be under both? Would you consider it a part of sourcerer overall? If not, happy to leave it to @michaelolo24 :)

Since we're in this file, could we add docs to each of the functions in the file, giving a high level description of what each accomplishes? That could help in understanding which parts of the app they may affect.

@yctercero done!

…-fix'

michaelolo24

LGTM - #131452 (Confirming we're merging this through the integration branch)

stephmilovic · 2022-05-10T17:22:42Z

@elasticmachine merge upstream

stephmilovic · 2022-05-11T14:34:52Z

@elasticmachine merge upstream

kibana-ci · 2022-05-11T16:48:08Z

💛 Build succeeded, but was flaky

Failed CI Steps

Security Solution Tests

Test Failures

[job] [logs] Security Solution Tests / Inspect Hosts stats and tables "before all" hook for "inspects the All Hosts Table"

Metrics [docs]

✅ unchanged

History

💔 Build #43801 failed 1e1253b
💚 Build #41934 succeeded 9eae382
💔 Build #41922 failed 676c6f4
💚 Build #41802 succeeded 964e9845f6f1cdac756cc58bada030fc3df9187b
💚 Build #41454 succeeded d68ab96

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

kibanamachine · 2022-05-11T18:20:33Z

⚪ Backport skipped

The pull request was not backported as there were no branches to backport to. If this is a mistake, please apply the desired version labels or run the backport tool manually.

Manual backport

To create the backport manually run:

node scripts/backport --pr 130915

Questions ?

Please refer to the Backport tool documentation

…0915)

get rid of formatSecondFields

b9cd5a8

stephmilovic requested a review from a team as a code owner April 25, 2022 17:15

kibanamachine and others added 2 commits April 25, 2022 12:28

Merge branch 'main' into first_second_fields

6b0d3be

test fixing

634bde7

Merge branch 'main' into first_second_fields

b578500

michaelolo24 reviewed Apr 28, 2022

View reviewed changes

x-pack/plugins/timelines/server/search_strategy/index_fields/index.ts Outdated Show resolved Hide resolved

michaelolo24 reviewed Apr 28, 2022

View reviewed changes

x-pack/plugins/timelines/server/search_strategy/index_fields/index.ts Outdated Show resolved Hide resolved

concat

d68ab96

yctercero reviewed May 2, 2022

View reviewed changes

stephmilovic force-pushed the first_second_fields branch from b133c79 to d68ab96 Compare May 2, 2022 21:13

stephmilovic added 2 commits May 2, 2022 15:14

Merge branch 'main' into first_second_fields

bd68a76

pr updates

676c6f4

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

9eae382

…-fix'

michaelolo24 added the ci:deploy-cloud label May 3, 2022

michaelolo24 approved these changes May 4, 2022

View reviewed changes

stephmilovic mentioned this pull request May 4, 2022

[WIP][Security Solution] #131452

Closed

9 tasks

Merge branch 'main' into first_second_fields

1e1253b

Merge branch 'main' into first_second_fields

cefe3ce

stephmilovic merged commit 9c8796a into elastic:main May 11, 2022

kibanamachine added the backport:skip This commit does not require backporting label May 11, 2022

stephmilovic added v8.2.1 and removed backport:skip This commit does not require backporting labels May 11, 2022

stephmilovic mentioned this pull request May 11, 2022

[8.2] [Security Solution] Optimize field formatting server side (#130915) #132059

Merged

academo pushed a commit to academo/kibana that referenced this pull request May 12, 2022

[Security Solution] Optimize field formatting server side (elastic#13…

8f6bd91

…0915)

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Optimize field formatting server side #130915

[Security Solution] Optimize field formatting server side #130915

stephmilovic commented Apr 25, 2022 •

edited

Loading

elasticmachine commented Apr 25, 2022

elasticmachine commented Apr 25, 2022

stephmilovic commented Apr 25, 2022

kqualters-elastic commented Apr 25, 2022 •

edited

Loading

stephmilovic commented Apr 26, 2022

stephmilovic commented Apr 27, 2022

michaelolo24 Apr 28, 2022

michaelolo24 Apr 28, 2022

stephmilovic commented May 2, 2022

yctercero May 2, 2022

stephmilovic May 2, 2022

yctercero May 2, 2022

yctercero left a comment

YulNaumenko commented May 2, 2022 •

edited

Loading

cla-checker-service bot commented May 2, 2022 •

edited

Loading

stephmilovic commented May 2, 2022

stephmilovic commented May 2, 2022

michaelolo24 left a comment

stephmilovic commented May 10, 2022

stephmilovic commented May 11, 2022

kibana-ci commented May 11, 2022

kibanamachine commented May 11, 2022

[Security Solution] Optimize field formatting server side #130915

[Security Solution] Optimize field formatting server side #130915

Conversation

stephmilovic commented Apr 25, 2022 • edited Loading

Summary

elasticmachine commented Apr 25, 2022

elasticmachine commented Apr 25, 2022

stephmilovic commented Apr 25, 2022

kqualters-elastic commented Apr 25, 2022 • edited Loading

stephmilovic commented Apr 26, 2022

stephmilovic commented Apr 27, 2022

michaelolo24 Apr 28, 2022

Choose a reason for hiding this comment

michaelolo24 Apr 28, 2022

Choose a reason for hiding this comment

stephmilovic commented May 2, 2022

yctercero May 2, 2022

Choose a reason for hiding this comment

stephmilovic May 2, 2022

Choose a reason for hiding this comment

yctercero May 2, 2022

Choose a reason for hiding this comment

yctercero left a comment

Choose a reason for hiding this comment

YulNaumenko commented May 2, 2022 • edited Loading

cla-checker-service bot commented May 2, 2022 • edited Loading

stephmilovic commented May 2, 2022

stephmilovic commented May 2, 2022

michaelolo24 left a comment

Choose a reason for hiding this comment

stephmilovic commented May 10, 2022

stephmilovic commented May 11, 2022

kibana-ci commented May 11, 2022

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

kibanamachine commented May 11, 2022

⚪ Backport skipped

Manual backport

Questions ?

stephmilovic commented Apr 25, 2022 •

edited

Loading

kqualters-elastic commented Apr 25, 2022 •

edited

Loading

YulNaumenko commented May 2, 2022 •

edited

Loading

cla-checker-service bot commented May 2, 2022 •

edited

Loading