-
Notifications
You must be signed in to change notification settings - Fork 220
Automatic rate limit restoration w/ admin API calls #6688
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
base: main
Are you sure you want to change the base?
Automatic rate limit restoration w/ admin API calls #6688
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3452 tests passing in 1397 suites. Report generated by 🧪jest coverage report action from 68d8802 |
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
80c8dcb to
68d8802
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/http.d.ts@@ -18,7 +18,7 @@ type AutomaticCancellationBehaviour = {
} | {
useAbortSignal: AbortSignal | (() => AbortSignal);
};
-type RequestBehaviour = NetworkRetryBehaviour & AutomaticCancellationBehaviour;
+export type RequestBehaviour = NetworkRetryBehaviour & AutomaticCancellationBehaviour;
export type RequestModeInput = PresetFetchBehaviour | RequestBehaviour;
/**
* Specify the behaviour of a network request.
packages/cli-kit/dist/public/node/api/graphql.d.ts@@ -47,6 +47,7 @@ export type GraphQLRequestDocOptions<TResult, TVariables> = GraphQLRequestBaseOp
}>>;
variables?: TVariables;
unauthorizedHandler?: UnauthorizedHandler;
+ autoRateLimitRestore?: boolean;
};
export interface GraphQLResponseOptions<T> {
handleErrors?: boolean;
|

WHY are these changes introduced?
Add optional support for automatic recovery of rate limit consumed in Admin API calls. If a call consumes 10 units (from
extensions.cost.actualQueryCost), and the recovery rate per second is 1000 units (fromextensions.cost.restoreRate), then we should wait 0.01s after making our API call to ensure the same amount of tokens we've consumed have had the chance to recover.This means we can have code that hits the Admin API in succession -- such as Metafield import, where there are many queries to make -- without being a bad citizen or hitting a rate limit.
From a callers perspective its as if the API call took very very slightly longer.
WHAT is this pull request doing?
After making a GraphQL call, if the relevant option is enabled, inspect the
extensions.costpart of the raw GraphQL response, check for unit usage/recovery rate, and sleep the requisite amount of time. The time is dependent on query complexity, but it's often no more than double digit milliseconds.Sleep information is included in the debug output. Time spent sleeping is counted under network usage time for analytics purposes.
How to test your changes?
There's good test coverage, and an upstack PR makes direct use of this in a CLI command
Measuring impact
How do we know this change was effective? Please choose one:
Checklist