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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[match][sigh] speed up profiles fetching by adding name filter to the API calls #21016

Closed

Conversation

PaulTaykalo
Copy link
Contributor

@PaulTaykalo PaulTaykalo commented Jan 28, 2023

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I see several green ci/circleci builds in the "All checks have passed" section of my PR (connect CircleCI to GitHub if not)
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Speeding up some queries on AppStoreConnectAPI

Description

When running match and sigh commands, and there's a need for searching specific provisioning profiles, the current general pattern is to search for all profiles by calling Spaceship::ConnectAPI::Profile.all and filter later i.e.

all_profiles = Spaceship::ConnectAPI::Profile.all(includes: "devices")

In this PR we're adding additional filtering when querying ConnectAPI which greatly reduces request times.

all_profiles = Spaceship::ConnectAPI::Profile.all(filter: { name: name}, includes: "devices")

These queries are running multiple times during sigh/match tools so decreasing time even for a few seconds will get a great improvement in speed

Important notes

  • While it seems that profile can simply be renamed, and have the same uuid, it's not actually true. When the profile is renamed, its uuid changes as well.
  • There's no visible way to require exact name matching when querying, so we're always adding later checks and not relying on names only. For example, if "Profile" is searched, "Profile1" and "Profile2" will be returned.

Some numbers

Numbers greatly depend on the portal size, these are numbers for a real one (~200 profiles, 50 certs)
Querying all profiles (without specifying name) was taking from 60 to 150 seconds.
Adding name filtering decreased fetching time by almost 25x times 馃槷

INFO [2023-01-29 01:46:11.10]: Time for fetching all profiles Without names: 125.897404
INFO [2023-01-29 01:46:12.57]: Time for fetching all profiles with names: 1.467258
INFO [2023-01-29 01:48:43.37]: Time for fetching all profiles Without names: 150.727023
INFO [2023-01-29 01:48:49.80]: Time for fetching all profiles: 6.429851

Testing Steps

Basically, sigh and match should work exactly the same.

@PaulTaykalo PaulTaykalo force-pushed the fix/faster-profile-resolving branch 2 times, most recently from d8443b5 to c7f5288 Compare January 29, 2023 00:13
@PaulTaykalo PaulTaykalo changed the title Fetch profiles faster if name is known Speedup profiles fetching by adding name filter to the api calls Jan 29, 2023
@PaulTaykalo PaulTaykalo changed the title Speedup profiles fetching by adding name filter to the api calls Speedup profiles fetching by adding name filter to the API calls Jan 29, 2023
Copy link

@gelosi gelosi left a comment

Choose a reason for hiding this comment

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

Wow. Can鈥檛 wait to upgrade match 馃憦

@nekrich
Copy link
Contributor

nekrich commented Jul 18, 2023

@joshdholtz @rogerluan can you please merge this PR. We are using this improvement for 6 months already and are really happy with it.

Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

馃挴 Nice!

@rogerluan rogerluan changed the title Speedup profiles fetching by adding name filter to the API calls [match][sigh] speed up profiles fetching by adding name filter to the API calls Jul 28, 2023
@rogerluan rogerluan requested a review from revolter July 28, 2023 02:58
Copy link
Member

@rogerluan rogerluan left a comment

Choose a reason for hiding this comment

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

@PaulTaykalo , sorry that this PR didn't get merged yet 馃槵 it ended up developing merge conflicts, would you mind fixing them by merging master into this branch?

Also, since we've merged a PR that potentially improved match performance by up to 100x, would you mind running your tests again to check whether this PR still improves performance, and by how much? 馃檱

Thank you so much!

@PaulTaykalo
Copy link
Contributor Author

@rogerluan from what I see all these changes were added in #21694 this PR
So, I'll close current PR

@rogerluan
Copy link
Member

Thanks for verifying this, @PaulTaykalo 馃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants