Skip to content

feat: add a simple in-memory cache for suborg/repo config fetches #817

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

Open
wants to merge 1 commit into
base: main-enterprise
Choose a base branch
from

Conversation

DouJohn
Copy link

@DouJohn DouJohn commented Apr 21, 2025

Fixes #816

This PR adds a simple in-memory cache in the loadYaml function, that stores the response + etag of calls to the getContent call. Then, future calls are made with the If-None-Match header set to that etag, so that we can optimistically just return from the cache instead and save ourselves the api call. In our instance of safe settings, this ends up saving about 96% of our api calls for our app. (We've made 3.4M api calls in the last week, of which 3.248M were cached - about 95.5%)

Because safe settings attempts to load all of the repo configs & suborg configs every time a sync is done, that api call usage tends to grow explosively on large orgs. We run this in an org with around 8500 repos in it, and because we have 140-ish suborgs, that ends up being 140 suborg calls against our api limit every time a setting is changed!

With this change, we store the response of each of those calls in memory, and only make an api call if github knows the file contents have changed.

This is a VERY simple cache, so it has some limitations:

  • There's not a method for expiring from this cache, which means it could grow very large. However, it should be bounded by the total size of all the configs you expect to load while safe-settings is running. For our 8500-repo org, that ends up being around 10kb of memory. Given the amount of memory we have free in our container, we don't really see this growing to be a problem.
  • This is just an in-memory cache, so it's not durable to container restarts or anything. The downside for us is that means we burn 140-ish api calls to re-initialize the cache the first time a sync is run, which is an acceptable cost compared to the server cost of running a separate caching instance.

@bmike78
Copy link

bmike78 commented Apr 21, 2025

fixes #816

@bmike78
Copy link

bmike78 commented Apr 21, 2025

probably fixes #559

@zpratt
Copy link

zpratt commented May 6, 2025

@decyjphr, we have tested this with our fork of safe-settings and have been running it in production now for a few weeks without any issues. what else can we do to help get this merged?

@decyjphr
Copy link
Collaborator

decyjphr commented May 6, 2025

Zach, will merge soon. Travel got in the way.

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.

Add Support for in memory caching
4 participants