feat(analytics-browser): support gzip request body compression#1542
feat(analytics-browser): support gzip request body compression#1542
Conversation
3b91b84 to
e0f6d6e
Compare
test: e2e Co-Authored-By: Cursor <cursoragent@cursor.com>
e0f6d6e to
84254a5
Compare
|
bugbot run |
|
Because this adds extra computation prior to calling |
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable autofix in the Cursor dashboard.
| const shouldCompressBody = | ||
| shouldCompressUploadBody && | ||
| getStringSizeInBytes(bodyString) >= MIN_GZIP_UPLOAD_BODY_SIZE_BYTES && | ||
| isCompressionStreamAvailable(); |
There was a problem hiding this comment.
Looking at the getStringSizeInBytes helper functions, it looks like it runs new TextEncoder(value), which creates a copy of the payload and then returns the size, meaning there's an additional copy of the payload in memory (in byte format) that is generated in O(n) time (because it has to iterate through each character in the string and copy it over).
I'm thinking because this threshold is just a heuristic anyways, maybe could just define the minimum upload size as a minimum string length rather than a minimum byte size, then we'd just need to check bodyString.length.
I understand that this goes against our agreement to make it 2kb, but string length should be "good enough". Most characters in a string are 1 byte each, and at most they're 3 bytes each, which gives an upper bound on how much we can under-exaggerate the "size" of a string.
There was a problem hiding this comment.
This is a good point! English letter, number, and common punctuation are all 1 byte each. So I'm keeping 2 * 1024 * 1024 and compare it with string length.
| if (compressed) { | ||
| headers['Content-Encoding'] = 'gzip'; | ||
| body = compressed; | ||
| } |
There was a problem hiding this comment.
Awesome! This nice.
There was a problem hiding this comment.
Actually I re-thought about this and felt it makes more sense to not allow custom headers override default headers (Content-Type, Accept, Content-Encoding). If it's useless and even harmful to override default headers if transport providers keeps their behavior to set json or gzip payload. Custom headers are support to support proxy authorization in real world for example.
| super(); | ||
| } | ||
|
|
||
| async send(serverUrl: string, payload: Payload, _enableRequestBodyCompression = false): Promise<Response | null> { |
There was a problem hiding this comment.
Since _enableRequestBodyCompression is an optional parameter in the BaseTransport method, you should just be able to leave it out of the send type signature, since it's not being used.
There was a problem hiding this comment.
Oh I just wanna make it look aligned in browser platform.
_enableRequestBodyCompression is an optional parameter in the BaseTransport method,
Yes! So we can keep the http transport in node sdk remains unchanged for now
| // Temporary browser-specific fetch transport with gzip support. | ||
| // TODO: Merge this implementation back into @amplitude/analytics-core FetchTransport | ||
| // once React Native SDK supports request body gzip. | ||
| export class FetchTransport extends BaseTransport implements Transport { |
There was a problem hiding this comment.
What was the reason for making a new fetch.ts in analytics-browser? Is it because the core fetch.ts fails to compile in other environments (like React Native), when you add the compression logic?
There was a problem hiding this comment.
React Native import core fetch. I don't wanna let it use the gzip feature now because by default we compress
There was a problem hiding this comment.
Does React Native fail at runtime or compilation time though? Couldn't we just do a check on !!globalScope.CompressionStream and if it returns false don't run gzip for that platform? I'm just concerned about having 2 separate implementations of fetch that would need to be maintained.
There was a problem hiding this comment.
The browser fetch is for short term. After dogfooding, we can have another pr for react native and merge it to the gzip feature branch and then merge gzip feature branch into main together. I tend to avoid making this giant pr bigger.
|
|
||
| export class BaseTransport implements Transport { | ||
| send(_serverUrl: string, _payload: Payload): Promise<Response | null> { | ||
| send(_serverUrl: string, _payload: Payload, _enableRequestBodyCompression?: boolean): Promise<Response | null> { |
There was a problem hiding this comment.
Thinking about the signature of this some more. I feel like there's a chance, in the future, that there could be more options that we need to add to send and the method signature would be pretty long.
Would it be much trouble if we did it like this:
type SendOptions = { _enableRequestBodyCompression?: boolean };
send(_serverUrl: string, _payload: Payload, _sendOptions?: SendOptions)
I'll leave this one as "optional".
There was a problem hiding this comment.
I thought about this too but realized we probably need a refactor on the whole class. It doesn't make sense to me to set custom headers as a class property but pass server url as a parameter to send. They should be treated the same and as well as the should compress flag.
There was a problem hiding this comment.
I’ll keep it as it is because it’s an internal interface, unlike storage provider.
Summary
enableRequestBodyCompressionto disable it if server url is set to a proxy urlChecklist
Note
Medium Risk
Touches the core event upload pipeline and transport interface; mistakes could break ingestion or proxy setups, though changes are gated by endpoint checks, size thresholds, and extensive tests.
Overview
Adds optional gzip compression for event upload bodies by extending the core
Transport.send()API and plumbing a newenableRequestBodyCompressionconfig through browser/core config and the destination upload path.Compression is forced for Amplitude’s default ingestion endpoints, while custom
serverUrls only compress when explicitly enabled; browser transports (XHRTransportand a browser-localFetchTransport) gzip payloads �≥2MB whenCompressionStreamis available, andSendBeaconTransportexplicitly opts out since it can’t set headers. E2E and unit tests are updated/added to handle gzipped bodies (including request parsing helpers) and to validate compression behavior across transports and endpoint types.Written by Cursor Bugbot for commit df2cac1. Configure here.