Skip to content

Performance improvements to the ACL sync - #1255

Merged
seanstory merged 4 commits into
mainfrom
seanstory/5162-speed-up-acl-sync
Jul 13, 2023
Merged

Performance improvements to the ACL sync#1255
seanstory merged 4 commits into
mainfrom
seanstory/5162-speed-up-acl-sync

Conversation

@seanstory

@seanstory seanstory commented Jul 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Part of https://github.com/elastic/enterprise-search-team/issues/5162

Speeding up the ACL sync.

  • fixes an id-vs-username bug
  • Utilizes $expand=transitiveMemberOf in a bulk request, to reduce request volume
  • Utilizes $top to cap out page size
  • Utilizes $filter to fetch only active users, and not disabled users.
  • Utilizes $select to fetch only the fields we need, which speeds up response time
  • Removes fetching users from UserInformationList because that was broken, and we could not extract User Ids (to then fetch transitive group membership)

Checklists

Pre-Review Checklist

  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)
  • Considered corresponding documentation changes (follow-up PR)
  • Contributed any configuration settings changes to the configuration reference

Related Pull Requests

@artem-shelkovnikov
artem-shelkovnikov force-pushed the seanstory/5162-speed-up-acl-sync branch from fc6a089 to 51ae0e4 Compare July 12, 2023 12:49
@seanstory
seanstory force-pushed the seanstory/5162-speed-up-acl-sync branch from 51ae0e4 to b52708f Compare July 12, 2023 18:17
@seanstory
seanstory marked this pull request as ready for review July 12, 2023 18:17
@seanstory
seanstory requested a review from a team July 12, 2023 18:17
@seanstory seanstory changed the title Trying out some improvements to the ACL sync Performance improvements to the ACL sync Jul 12, 2023
@seanstory seanstory mentioned this pull request Jul 12, 2023
Merged
1 task
@seanstory

seanstory commented Jul 12, 2023

Copy link
Copy Markdown
Member Author
Screenshot 2023-07-12 at 2 34 22 PM

woo, we making progress!

  • The first success was just using the List Users API.
  • The next speedup was filtering on active users only (good idea @artem-shelkovnikov, why index deactivated users? 🤷 )
  • The next speedup was explicitly providing $select clauses for only the fields we consume. I would have expected this to be slower, requiring extra processing on MS's side. But a StackOverflow post said this is how you make it go fast, and it was right!

artem-shelkovnikov
artem-shelkovnikov previously approved these changes Jul 13, 2023
View reviewed changes

@artem-shelkovnikov artem-shelkovnikov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great improvements, Sean!

@artem-shelkovnikov
artem-shelkovnikov force-pushed the seanstory/5162-speed-up-acl-sync branch from 542abcd to c1be74d Compare July 13, 2023 12:08
This was referenced Jul 13, 2023
Merged
Merged
@artem-shelkovnikov

artem-shelkovnikov commented Jul 13, 2023

Copy link
Copy Markdown
Member

@seanstory I've rebased the changeset and things got weird :/ You might wanna force push your branch in and do the change to connector.json manually :/

seanstory added a commit to elastic/kibana that referenced this pull request Jul 13, 2023
@seanstory
remove native config for selecting fetch strategy (#161798)
8175c8a 
## Summary

Part of elastic/search-team#5162

Effectively reverting #161546, as
the new strategy has completely replaced the old.

See also: elastic/connectors#1255

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
@seanstory
seanstory force-pushed the seanstory/5162-speed-up-acl-sync branch from c1be74d to 52661c1 Compare July 13, 2023 13:28
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 13, 2023
@seanstory
remove native config for selecting fetch strategy (elastic#161798)
33ecb62 
## Summary

Part of elastic/search-team#5162

Effectively reverting elastic#161546, as
the new strategy has completely replaced the old.

See also: elastic/connectors#1255

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

(cherry picked from commit 8175c8a)
@seanstory
seanstory enabled auto-merge (squash) July 13, 2023 14:23
@seanstory
seanstory merged commit 24249c1 into main Jul 13, 2023
@seanstory
seanstory deleted the seanstory/5162-speed-up-acl-sync branch July 13, 2023 14:26
@github-actions

github-actions Bot commented Jul 13, 2023

Copy link
Copy Markdown

💔 Failed to create backport PR(s)

Status Branch Result
❌ 8.9 Commit could not be cherrypicked due to conflicts

To backport manually run:
backport --pr 1255 --autoMerge --autoMergeMethod squash

kibanamachine referenced this pull request in elastic/kibana Jul 13, 2023
@kibanamachine @seanstory
[8.9] remove native config for selecting fetch strategy (#161798) (#1…
49a4eee 
…61866)

# Backport

This will backport the following commits from `main` to `8.9`:
- [remove native config for selecting fetch strategy
(#161798)](#161798)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Sean
Story","email":"sean.j.story@gmail.com"},"sourceCommit":{"committedDate":"2023-07-13T13:25:10Z","message":"remove
native config for selecting fetch strategy (#161798)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/enterprise-search-team/issues/5162\r\n\r\nEffectively
reverting #161546, as\r\nthe new
strategy has completely replaced the old.\r\n\r\nSee also:
https://github.com/elastic/connectors-python/pull/1255\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"8175c8a95d053cdcad6c8cf61dd767f90b797c58","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:EnterpriseSearch","v8.9.0","v8.10.0"],"number":161798,"url":"https://github.com/elastic/kibana/pull/161798","mergeCommit":{"message":"remove
native config for selecting fetch strategy (#161798)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/enterprise-search-team/issues/5162\r\n\r\nEffectively
reverting #161546, as\r\nthe new
strategy has completely replaced the old.\r\n\r\nSee also:
https://github.com/elastic/connectors-python/pull/1255\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"8175c8a95d053cdcad6c8cf61dd767f90b797c58"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/161798","number":161798,"mergeCommit":{"message":"remove
native config for selecting fetch strategy (#161798)\n\n##
Summary\r\n\r\nPart of
https://github.com/elastic/enterprise-search-team/issues/5162\r\n\r\nEffectively
reverting #161546, as\r\nthe new
strategy has completely replaced the old.\r\n\r\nSee also:
https://github.com/elastic/connectors-python/pull/1255\r\n\r\n### For
maintainers\r\n\r\n- [ ] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"8175c8a95d053cdcad6c8cf61dd767f90b797c58"}}]}]
BACKPORT-->

Co-authored-by: Sean Story <sean.j.story@gmail.com>
@seanstory seanstory mentioned this pull request Jul 13, 2023
Merged
seanstory added a commit that referenced this pull request Jul 13, 2023
@seanstory
Performance improvements to the ACL sync (#1255)
39fa082 
* Trying out some improvements to the ACL sync

* Remove fetching users by site, fetch active users only

* Add explicit select clauses when fetching users

* removing RCF from spo ftest

(cherry picked from commit 24249c1)

# Conflicts:
#	connectors/sources/sharepoint_online.py
@seanstory

seanstory commented Jul 13, 2023

Copy link
Copy Markdown
Member Author

💚 All backports created successfully

Status Branch Result
✅ 8.9

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

seanstory added a commit that referenced this pull request Jul 13, 2023
@seanstory
[8.9] Performance improvements to the ACL sync (#1255) (#1267)
465dce9 
* Performance improvements to the ACL sync (#1255)

* Trying out some improvements to the ACL sync

* Remove fetching users by site, fetch active users only

* Add explicit select clauses when fetching users

* removing RCF from spo ftest

(cherry picked from commit 24249c1)

# Conflicts:
#	connectors/sources/sharepoint_online.py

* autoformat merge conflict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artem-shelkovnikov artem-shelkovnikov artem-shelkovnikov approved these changes

Assignees

No one assigned

Labels

auto-backport v8.9.0 v8.10.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@seanstory @artem-shelkovnikov