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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safer pagination and refresh for site cache #1239

Merged
merged 14 commits into from
Apr 24, 2024

Conversation

timotheeg
Copy link
Contributor

@timotheeg timotheeg commented Mar 22, 2024

Problem

The way isomercms retrieves and cache the list of repos is with the help of an environment variable ISOMERPAGES_REPO_PAGE_COUNT. That variable contains a number, which is used to create parallel requests to github.

In practice that works, but operationally this is not great:

  • It introduces a mental load to update the variables if the number of supported repo grows beyond the current page count (easy to forget and create a potential incident!)
  • The concurrency fetches optimize for time in all cases, while for the internal cache refreshes, optimizing for load distribution would be safer

Github APIs have a very robust pagination system, so we can derive the number of pages from that to make the system dynamic, and remove the (small) operational risk of forgetting to update the env var.

Additionally, the cache only has one exposed API, which is to retrieve the lastUpdated time by repoName. The list of repo is not stored in cache by name, but it is a list and so the lookup does a list search every time.

Solution

  1. split the getAllRepoData() calls into 2 distinct use cases:
    a. exported API optimized for time: we fetch the first page, assess how many pages there in total, and fetch all remaining pages in parallel
    b. optimized for load distribution: we fetch the pages sequentially by following github pagination system's next link
  2. populate the cache at bootstrap time with the time-optimized call
  3. cache refresh uses the load-optimized call
  4. Use a dictionary to store the repos indexes by name for O(1) lookup

Notes:

  1. The time-optimized fetch is still slower than the previous version, since the first page call is always made sequentially
  2. We could (to discuss) cache the number of pages, such that:
    a. only the first bootstrap call has the 2 step fetch
    b. every refresh call updates the number of pages
    c. when the number of pages is cached, the fast getAllRepoData() does all its calls in parallel again.
  3. We add a custom span to instrument the time the cache refresh process takes.

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Reference:

Results:
Sample trace for the new refresh process that shows fetches the pages sequentially works:

image

Post Deploy Work (after stability is observed)

  • delete the SSM entries 'PROD_ISOMERPAGES_REPO_PAGE_COUNT' and STAGING_ISOMERPAGES_REPO_PAGE_COUNT. This should ONLY be done when we are sure we will not need to roll back possibly wait one full release.

@mergify mergify bot requested a review from a team April 21, 2024 11:32
Copy link

mergify bot commented Apr 21, 2024

This pull request has been stale for more than 30 days! Could someone please take a look at it @isomerpages/iso-engineers

@timotheeg timotheeg marked this pull request as ready for review April 23, 2024 06:40
@timotheeg timotheeg force-pushed the safer-pagination-and-refresh-for-site-cache branch from 8e84614 to 475d999 Compare April 23, 2024 06:42
@timotheeg timotheeg force-pushed the safer-pagination-and-refresh-for-site-cache branch from 87ead11 to f0a22d5 Compare April 23, 2024 07:54
afterEach(() => jest.clearAllMocks())
afterEach(() => {
jest.clearAllMocks()
mockAxios.get.mockReset()
Copy link
Contributor Author

@timotheeg timotheeg Apr 24, 2024

Choose a reason for hiding this comment

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

Without this, the tests previously were not actually doing what we think they were doing!! 😱

@timotheeg
Copy link
Contributor Author

Note: Typically another way to deal with pages and fetching data is by using the github sdk octokit.js, which has pagination handling built in. I did not introduce it in this PR because the goal was just to remove the env var ISOMERPAGES_REPO_PAGE_COUNT, and I kept the parallel fetch for pages 2-N, which octokit wouldn't do.

That should probably be revisited at some point.

src/services/identity/SitesCacheService.ts Outdated Show resolved Hide resolved
src/services/identity/SitesCacheService.ts Outdated Show resolved Hide resolved
src/services/identity/SitesCacheService.ts Outdated Show resolved Hide resolved
src/services/identity/SitesCacheService.ts Outdated Show resolved Hide resolved
src/services/identity/SitesCacheService.ts Outdated Show resolved Hide resolved
@timotheeg
Copy link
Contributor Author

All comments addressed @seaerchin , Let me know if good to go. Thanks! 🙏

@seaerchin seaerchin self-requested a review April 24, 2024 05:06
// example value: link: <https://api.github.com/organizations/40887764/repos?page=2>; rel="next", <https://api.github.com/organizations/40887764/repos?page=34>; rel="last"
const links: LinkSet = {}

const linkRe = /<(?<url>[^>]+)>; rel="(?<relid>[a-z]+)"(, )?/g
Copy link
Contributor Author

@timotheeg timotheeg Apr 24, 2024

Choose a reason for hiding this comment

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

Note: this regex is not actually validating anything on the urls, or that relid should be one of "first" | "last" | "prev" | "next", even thought we do assert it in the types below with as IterableIterator<LinkMatch>

Because we are the one calling github, we trust their APIs, but we could add paranoia checks, and warning logging here 🤔

Or the regexp could be made more "validating" in that sense, like so for example, which would at least guarantee that relid is correct according to the type:

Suggested change
const linkRe = /<(?<url>[^>]+)>; rel="(?<relid>[a-z]+)"(, )?/g
const linkRe = /<(?<url>[^>]+)>; rel="(?<relid>first|last|prev|next)"(, )?/g

Copy link
Contributor Author

@timotheeg timotheeg Apr 24, 2024

Choose a reason for hiding this comment

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

I'd rather leave the regexp loose tbh, in case github introduces more link relations in the future, they'd already be captured properly as part of the parsing routine.

@timotheeg timotheeg merged commit c26c016 into develop Apr 24, 2024
12 checks passed
@timotheeg timotheeg deleted the safer-pagination-and-refresh-for-site-cache branch April 24, 2024 07:11
@alexanderleegs alexanderleegs mentioned this pull request Apr 26, 2024
3 tasks
@timotheeg
Copy link
Contributor Author

Result in prod:

  • As expected: /v2/sites is slower now since the first page is always sequential (sample trace here)
    image
  • The refresh is a cheaper process now, since all the ages are sequential (sample internal trace)
    image

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

Successfully merging this pull request may close these issues.

2 participants