-
Notifications
You must be signed in to change notification settings - Fork 5
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 [sc 106247]: Add a caching layer to prevent SDK from sending the same payloads too frequently upstream #193
Conversation
e10bca8
to
33576b3
Compare
755dcac
to
5aeba44
Compare
5aeba44
to
6073f9e
Compare
Is there a reason we don't apply this to other methods in the API? If we do, this should be a middleware in the same implementation that gin middlewares work. It's weird to me that we are returning "Served From Cache" when we rate limit in this method. I do like that we are including a custom header to indicate that the request was rate limited, but the served from cache is intended to mean that the API was disconnected. Will reusing this same header create a more difficult to debug scenario where the user can't tell if the API was down or they are sending too frequently? Continuing to make this return the same status code is great, don't 429 and push the issue to the developer. |
8a7cd44
to
1d0a0cd
Compare
1d0a0cd
to
246780e
Compare
Good thought process @marccampbell . My initial reading of the issue was that we had a few delinquent endpoints (e.g custom-metrics) that needed to be "rate-limited" via a caching mechanism. The current closure implementation of the middleware allows each endpoint to have its own independent cache. Typing it out makes me thing that this might be unnecessary. I can definitely change the cache header to be more unique. Do you some meaningful name suggestions already in mind? Fundamentally, we are serving cached data so I re-used the header. There is |
de3bf1a
to
ce88810
Compare
ce88810
to
ee11442
Compare
137c8e5
to
690b6b5
Compare
690b6b5
to
28bc4e7
Compare
|
||
cache.Set(key, CacheEntry{ | ||
StatusCode: recorder.StatusCode, | ||
RequestBody: body, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should consider making this a digest too because even without historical record this map can get big
@divolgin (CC: @marccampbell ) I made a cached sub-router and added a |
42d9e4c
to
639acd8
Compare
639acd8
to
a632f58
Compare
} | ||
} | ||
|
||
func IsSamePayload(a, b []byte) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just use bytes.Equal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgalsaleh JSON key ordering differences when you marshal from map[string]interface{}
results in different byte slices for the same payload. Also raw JSON payloads might have different white space with same underlying content.
What this PR does / why we need it:
Caching middleware helps rate limits intentional or unintentional spamming of endpoints.
replicated-app
while preserving the client side UXWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Steps to reproduce
Does this PR introduce a user-facing change?
Does this PR require documentation?