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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Remove fields from sourcerer response #130917

Merged
merged 8 commits into from
May 11, 2022
Merged

[Security Solution] Remove fields from sourcerer response #130917

merged 8 commits into from
May 11, 2022

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Apr 25, 2022 •
edited
Loading

Summary

We were unnecessarily returning fields with sourcerer. This PR removes unnecessary calls to dataViews.get and ensures only id, patternList, and title are returned

Brings down the response time for the sourcerer call from about 3.5 seconds to .7 seconds

@stephmilovic stephmilovic added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed Team:Threat Hunting:Explore v8.3.0 labels Apr 25, 2022
@stephmilovic stephmilovic requested a review from a team as a code owner April 25, 2022 17:18
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

stephmilovic
stephmilovic commented Apr 25, 2022
View reviewed changes
x-pack/plugins/security_solution/server/lib/sourcerer/routes/index.test.ts
@@ -26,6 +26,13 @@ jest.mock('./helpers', () => {
});
const mockPattern = {
id: 'security-solution',
fields: [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding fields to mock response to ensure they are not returned

@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

YulNaumenko
YulNaumenko reviewed Apr 28, 2022
View reviewed changes
x-pack/plugins/security_solution/server/lib/sourcerer/routes/index.ts
@@ -90,6 +82,7 @@ export const createSourcererDataViewRoute = (
}
}
} else if (patternListAsTitle !== siemDataViewTitle) {
siemDataView = await dataViewService.get(dataViewId);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can get to this line only if siemDataView is not null, so I didn't understand the logic, why we need to reassign the new value fetched from dataViewService to siemDataView, is there any purpose for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, unfortunately to make the dataViewService.updateSavedObject call we need to pass the entire data view object including fields 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

@stephmilovic is this still true? From the docs it says Update part of an data view. Only the specified fields are updated in the data view. Unspecified fields stay as they are persisted.. Are we just looking to update title here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yctercero unfortunately, if we do not do the GET call we do not get the saved object methods. It's kind of silly, they should really fetch it on their end. But alas, sending only id and title to dataViewService.updateSavedObject results in

error TypeError: indexPattern.getAsSavedObjectBody is not a function
    at DataViewsService.updateSavedObject (/Users/stephmilovic/dev/kibana/src/plugins/data_views/common/data_views/data_views.ts:665:31)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the follow-up Steph! That stinks...

YulNaumenko
YulNaumenko approved these changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

@stephmilovic stephmilovic mentioned this pull request May 2, 2022
Merged
@michaelolo24 michaelolo24 mentioned this pull request May 3, 2022
Closed
9 tasks
@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@stephmilovic stephmilovic merged commit 5f5a6cf into elastic:main May 11, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 11, 2022
@kibanamachine
Copy link
Contributor

⚪ 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 130917

Questions ?

Please refer to the Backport tool documentation

@stephmilovic stephmilovic added v8.2.1 and removed backport:skip This commit does not require backporting labels May 11, 2022
@stephmilovic stephmilovic mentioned this pull request May 11, 2022
Merged
stephmilovic added a commit to stephmilovic/kibana that referenced this pull request May 11, 2022
@stephmilovic
[Security Solution] Remove fields from sourcerer response (elastic#…
7d42450 
…130917)

(cherry picked from commit 5f5a6cf)
stephmilovic added a commit that referenced this pull request May 11, 2022
@stephmilovic
[Security Solution] Remove fields from sourcerer response (#130917) (…
20c2f86 
…#132058)

(cherry picked from commit 5f5a6cf)
academo pushed a commit to academo/kibana that referenced this pull request May 12, 2022
@stephmilovic
[Security Solution] Remove fields from sourcerer response (elastic#…
b728903 
…130917)
@yctercero yctercero mentioned this pull request Aug 2, 2022
Open
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
@stephmilovic stephmilovic mentioned this pull request Nov 21, 2022
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yctercero yctercero yctercero approved these changes

@YulNaumenko YulNaumenko YulNaumenko approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed ci:cloud-deploy Create or update a Cloud deployment release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.2.1 v8.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@stephmilovic @elasticmachine @kibana-ci @kibanamachine @yctercero @YulNaumenko @tylersmalley @michaelolo24