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

[Cases] Performance and RBAC improvements #101465

Merged
merged 9 commits into from
Jun 8, 2021
Merged

[Cases] Performance and RBAC improvements #101465

merged 9 commits into from
Jun 8, 2021

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Jun 7, 2021 •
edited
Loading

Summary

Resolves: #100804

For maintainers

@cnasikas cnasikas added v8.0.0 Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.14.0 Theme: rac label obsolete Feature:Cases-RAC-RBAC labels Jun 7, 2021
@cnasikas cnasikas self-assigned this Jun 7, 2021
@cnasikas cnasikas changed the base branch from master to cases-rbac-poc June 7, 2021 09:38
This was referenced Jun 7, 2021
Merged
Closed
@cnasikas cnasikas marked this pull request as ready for review June 7, 2021 13:16
@cnasikas cnasikas requested a review from a team as a code owner June 7, 2021 13:16
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@jonathan-buttner jonathan-buttner deleted the branch elastic:master June 7, 2021 13:37
@jonathan-buttner jonathan-buttner changed the base branch from cases-rbac-poc to master June 7, 2021 13:47
@jonathan-buttner jonathan-buttner requested review from a team as code owners June 7, 2021 13:47
@cnasikas cnasikas force-pushed the cases_rbac_improv branch from fda09f4 to 44bfa52 Compare June 7, 2021 13:51
@mshustov mshustov removed the request for review from a team June 7, 2021 15:53
@mshustov
Copy link
Contributor

mshustov commented Jun 7, 2021

Don't see any changes related to Core, but the approach LGTM.

@cnasikas cnasikas removed the request for review from a team June 8, 2021 06:55
@cnasikas cnasikas added the release_note:skip Skip the PR/issue when compiling release notes label Jun 8, 2021
@cnasikas
Copy link
Member Author

cnasikas commented Jun 8, 2021

Don't see any changes related to Core, but the approach LGTM.

Thank you @mshustov! This PR was based on another PR (the RBAC PR) that touched some of your files. The PR got merged and I switched the base to master and your team left as a reviewer. Sorry for the trouble.

@cnasikas
Copy link
Member Author

cnasikas commented Jun 8, 2021

@elasticmachine merge upstream

@cnasikas cnasikas force-pushed the cases_rbac_improv branch from 676403c to b06d2b6 Compare June 8, 2021 13:03
@cnasikas cnasikas force-pushed the cases_rbac_improv branch from b06d2b6 to 075364c Compare June 8, 2021 13:08
jonathan-buttner
jonathan-buttner reviewed Jun 8, 2021
View reviewed changes
Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Looking good, just two more places I think we need the 10k perPage:
https://github.com/elastic/kibana/blob/master/x-pack/plugins/cases/server/services/cases/index.ts#L402

https://github.com/elastic/kibana/blob/master/x-pack/plugins/cases/server/common/models/commentable_case.ts#L328

x-pack/plugins/cases/server/client/cases/delete.ts
});

// Ensuring we don't too many concurrent get running.
const comments = await pMap(ids, getCommentsMapper, {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This isn't a change you made but if I understand the previous code correctly comments will be an array of saved object find responses (essentially an array of arrays [[1,2,3],[4,5,6]]). If we flattened it I wonder if it would make the deleteCommentsMapper a little easier to understand 🤷‍♂️ . That way we could avoid the pMap inside of the mapper.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's smart! As we gonna use search_after to iterate over all objects I will leave it for another PR and create a ticket to track it.

x-pack/plugins/cases/server/client/cases/find.ts
@@ -77,6 +77,7 @@ export const find = async (

ensureSavedObjectsAreAuthorized([...cases.casesMap.values()]);

// casesStatuses are bounded by us. No need to limit concurrent calls.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
cases 382 381 -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
cases 133.5KB 133.7KB +228.0B
Unknown metric groups

API count

id before after diff
cases 407 406 -1

History

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

cc @cnasikas

@cnasikas cnasikas merged commit 13e20e7 into elastic:master Jun 8, 2021
@cnasikas cnasikas deleted the cases_rbac_improv branch June 8, 2021 16:01
@cnasikas cnasikas mentioned this pull request Jun 8, 2021
Merged
cnasikas added a commit that referenced this pull request Jun 8, 2021
@cnasikas @kibanamachine
[Cases] Performance and RBAC improvements (#101465) (#101652)
f2a5c47 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathan-buttner jonathan-buttner jonathan-buttner approved these changes

Assignees

@cnasikas cnasikas

Labels
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 Security Solution Threat Hunting Team Theme: rac label obsolete v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Cases] Performance and RBAC improvements
5 participants
@cnasikas @elasticmachine @mshustov @kibanamachine @jonathan-buttner