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

feat(axios): implement etag functionality #765

Merged
merged 4 commits into from
May 17, 2023

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented May 15, 2023

Problem

Currently, Isomer uses a huge amount of tokens due to not caching. This leads to scaling issue, where our github token limits are quickly reached and users are unable to access the CMS.

Closes IS-172

Solution

  1. use an external package axios-cache-interceptor to utilise ETags for cache. See here for info on etags.
    • note that this is using in memory storage. the implications are that: we could potentially have some requests that could be cached but are not (current ETag value is 1 -> user A edits -> ETag value now 2 -> other instance doesn't knows -> queries from other instance -> ETag value updated)
    • potential for stale reads - github will tell us if the old etag value is outdated or not (ie, return 200) but the package uses the cache-control header to control freshness (which is good). because github's header sets max-age=60, the package will internally use the old value for 60s.

Tests

  1. first, add a console.log for remainingRequests into respHandler for AxiosInstance.ts, so that we know how much requests are remaining
  2. next, issue a GET request to the e2e-test-repo's homepage (curl --request GET \ --url http://localhost:8081/v2/sites/e2e-test-repo/homepage)
  3. observe the # of requests remaining.
  4. repeat steps 2-3; the # of requests remaining should be unchanged
  5. make an edit on the homepage
  6. repeat steps 2-3, the # of requests should be unchanged if it's less than a minute
  7. update the page again
  8. do steps 2-3, the request should succeed

New dependencies:

  • axios-cache-interceptor : interceptor for axios to use etags. the current backing storage is in memory.

Deploy notes

  1. axios got upgraded from 0.21.x to 0.25.0
    • list of breaking changes here
    • went through most of them, mostly typing changes.
  2. had to use 0.9.2 as a minimum for axios-cache-interceptor
    • see compat chart here
    • this is because of 2 reasons - ETag functionality only added in 0.6 + their import for object-code is an outdated one, which doesn't get fixed till 0.9.2.

seaerchin added 3 commits May 15, 2023 14:31
pin axios to only allow minor patch upgrades, and pin the version of interceptor exactly. this is
because there is a support table
[here](https://axios-cache-interceptor.js.org/guide/getting-started) that lists the compat and we
require at least 0.6.x for ETag support. in order to prevent versioning errors (0.8.4 of interceptor
works wtih 0.25 of axios!!), we will pin the exact version of interceptor + patch version of axios
@seaerchin seaerchin requested review from a team and removed request for a team May 15, 2023 09:43
@seaerchin seaerchin marked this pull request as draft May 15, 2023 09:51
@seaerchin seaerchin marked this pull request as ready for review May 16, 2023 07:00
@seaerchin seaerchin requested a review from a team May 16, 2023 07:00
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

[Non-nit]

I think the stale time should be something that we should explicitly define in our application. If we monitor that our token usage is getting too high, it would be a lot easier for us to tweak this number to reduce the number of calls. Thoughts?

src/services/api/AxiosInstance.ts Show resolved Hide resolved
src/services/api/AxiosInstance.ts Show resolved Hide resolved
@seaerchin
Copy link
Contributor Author

seaerchin commented May 16, 2023

[Non-nit]

I think the stale time should be something that we should explicitly define in our application. If we monitor that our token usage is getting too high, it would be a lot easier for us to tweak this number to reduce the number of calls. Thoughts?

to clarify, you mean the max age of 60s? i think this number is explicitly given by github as a header for to let us know how long to cache a request before revalidation. i think the use case of allowing it in code to be able to control it is fair but we should add this as a follow up if required rather than having it as a first pass, because the default that github sends might already be enough! do take note that this refetch is only triggered on etag change - ie, past 60s, file is unchanged, github will return a 304 without consuming token usage

wdyt?

@seaerchin seaerchin requested review from kishore03109 and a team May 16, 2023 09:36
Copy link
Contributor

@kishore03109 kishore03109 left a comment

Choose a reason for hiding this comment

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

to clarify, you mean the max age of 60s? i think this number is explicitly given by github as a header for to let us know how long to cache a request before revalidation. i think the use case of allowing it in code to be able to control it is fair but we should add this as a follow up if required rather than having it as a first pass, because the default that github sends might already be enough! do take note that this refetch is only triggered on etag change - ie, past 60s, file is unchanged, github will return a 304 without consuming token usage

makes sense to me, we can monitor if we actually want the ability to toggle the time taken for invalidation in the future then

@seaerchin seaerchin merged commit 0518d49 into develop May 17, 2023
@seaerchin seaerchin deleted the feat/is-172-implement-etags branch May 17, 2023 04:15
@kishore03109 kishore03109 mentioned this pull request May 18, 2023
alexanderleegs pushed a commit that referenced this pull request May 19, 2023
* chore(axios): upgrade axios to 0.24 and add axios cache interceptor

pin axios to only allow minor patch upgrades, and pin the version of interceptor exactly. this is
because there is a support table
[here](https://axios-cache-interceptor.js.org/guide/getting-started) that lists the compat and we
require at least 0.6.x for ETag support. in order to prevent versioning errors (0.8.4 of interceptor
works wtih 0.25 of axios!!), we will pin the exact version of interceptor + patch version of axios

* feat(axiosinstance): add initialisation code

* chore(package): lock versions

* fix(axiosinstance): update config for cache interceptor
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