perf: [do not deserialize json objects or update local cache if remote response has not changed from the previous one] (FF-2576) #56
Conversation
…e response has not changed from the previous one] (FF-2576)
| configStore: configStore, | ||
| httpClient: httpClient, | ||
| configStore: configStore, | ||
| deserializeCount: 0, |
There was a problem hiding this comment.
This is only for testing validation
|
|
||
| return HttpResponse{ | ||
| Body: string(b), | ||
| ETag: resp.Header.Get("ETag"), // Capture the ETag header |
There was a problem hiding this comment.
I am learning that there are two types but ETag (capital T) is the canonical form: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/ETag
The Eppo Fastly CDN seems to return Etag prefix with a W which is a "weak" tag. I think this is because by default express, the web server underlying NestJS, adds this by default.
Before we release this, Eppo's web server might need to be updated to return "strong" etags.
There was a problem hiding this comment.
Weak ETag is fine for our purposes as we don't care about byte-by-byte identity—we only need the json value to match.
| if cr.skipDeserializeAndUpdateFlagConfigIfUnchanged { | ||
| if result.ETag == cr.storedUFCResponseETag { |
There was a problem hiding this comment.
We should use If-None-Match header on the request, so the server can respond with 304 Not Modified and skip sending the body, instead of doing this check on the client side
| | `SdkKey` | The SDK key for authenticating requests. | | ||
| | `AssignmentLogger` | An implementation of the `IAssignmentLogger` interface for logging assignments. | | ||
| | `PollerInterval` | The interval at which the SDK polls for updates. Defaults to 10 seconds. | | ||
| | `SkipDeserializeAndUpdateFlagConfigIfUnchanged` | A boolean flag to skip deserialization and update if the configuration has not changed. Defaults to false. | |
There was a problem hiding this comment.
Why do we want it configurable? Is there any reason for a user to ever disable it?
| SdkKey string | ||
| AssignmentLogger IAssignmentLogger | ||
| PollerInterval time.Duration | ||
| SkipDeserializeAndUpdateFlagConfigIfUnchanged bool // default false |
There was a problem hiding this comment.
I think we want it enabled by default?
|
|
||
| return HttpResponse{ | ||
| Body: string(b), | ||
| ETag: resp.Header.Get("ETag"), // Capture the ETag header |
There was a problem hiding this comment.
Weak ETag is fine for our purposes as we don't care about byte-by-byte identity—we only need the json value to match.
| @@ -23,22 +26,26 @@ type SDKParams struct { | |||
| sdkVersion string | |||
| } | |||
|
|
|||
| func newHttpClient(baseUrl string, client *http.Client, sdkParams SDKParams) *httpClient { | |||
| type HttpResponse struct { | |||
| Body string | |||
There was a problem hiding this comment.
This should be []byte to avoid copying response bytes when converting to/from string.
| Body string | |
| Body []byte |
motivation
Following integrating the Eppo SDK into their server, we have a customer report of high CPU and memory utilization.
The default polling interval on golang is every 10 seconds and almost always the flag configuration from the CDN is not changed. However each of these refreshes the SDK deserialized the JSON into go objects and updates the cache.
proposed changes
Only perform json deserialization if the received response is different than the cached version. This is determined by comparing the
ETagof the received response with the value from the previously stored version.The Fastly CDN supports this: https://www.fastly.com/blog/etags-what-they-are-and-how-to-use-them/
👇 An example from Eppo's frontend
👉 By default I have left this behavior disabled so that we do not need a major version bump. A beta customer can try this in this release - if the results are positive we can enable it by default in future major versions.
rollout
It is unknown whether this change will fully mitigate the high memory utilization. We have another changes in scope.
for reviewers