-
Notifications
You must be signed in to change notification settings - Fork 780
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
Adding gzip compression support in browser case #4493
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4493 +/- ##
==========================================
+ Coverage 90.77% 91.26% +0.48%
==========================================
Files 90 233 +143
Lines 1930 6352 +4422
Branches 417 1401 +984
==========================================
+ Hits 1752 5797 +4045
- Misses 178 555 +377
|
experimental/packages/otlp-exporter-base/src/platform/browser/OTLPExporterBrowserBase.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/node/types.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
experimental/packages/otlp-exporter-base/src/platform/browser/util.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Marc Pichler <[email protected]>
I do have a question about browser support - CompressionStream is supported on majority of browsers, but not all. On browsers where this is not supported, the export will fail. We could do a feature detection, and if CompressionStream is not available, then default to export without compression. IMO, this would be a better experience, specifically because getting data about browser usage is a key metric in browser monitoring. (This might be a topic for a broader discussion) Alternatively, we should at last document that compression is not available on all browsers. |
@martinkuba I now check if CompressionStream is supported by the browser and proceed with sending without compression if it is not supported. |
@pichlermarc all aspects of this PR are resolved, can you take a look? |
comments are addressed, need to do another pass on the PR
@pichlermarc can you review this PR when you get a chance? There is a commit 8a2e225 from you that needs your attention. |
/easycla |
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.
IMO looks good, I'll let @martinkuba and @MSNev have the last word on this PR as they're more knowledgeable about this than I am.
Sorry about that CLA issue - no idea what that was about. Must've been a EasyCLA hiccup.
@@ -0,0 +1,78 @@ | |||
/* |
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.
I think this needs a rebase as this package does not exist anymore (http and json now use a shared base).
content: string | Blob, | ||
compressionAlgorithm: string | ||
): Promise<Uint8Array> { | ||
const compressionStream = new CompressionStream( |
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.
As CompressionStream was added in 2023, it (may) not exist in all environments and this would likely not be handled nicely.
We should have a check (somewhere) which not only check that the CompressionStream exists, but anything else that may not exist in the current runtime so that it can gracefully fallback to not use compression when it's not available rather than just assume the application has configured it correctly and all of their uses are using their supported runtime.
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.
@MSNev CompressionStream api became baseline stable in 2023.
From https://developer.mozilla.org/en-US/docs/Web/API/CompressionStream -
"Since May 2023, this feature works across the latest devices and browser versions. This feature might not work in older devices or browsers."
and at https://caniuse.com/?search=CompressionStream if you go to the tab "Usage relative", you can see that the browsers that do not support this API is in minority.
Can you tell me more about your suggestion to check for more?
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.
Yes, but as the current browser support is undefined (we have not said only from this version), we need to deal with cases of code running on older browsers. like older end-user devices that may not be using newer versions (ipads, android devices, iphones, etc) otherwise if we don't we could cause the application which is using OTel to fail.
Additionally, there are browser "like" environments (they pretend to be a browser and look and act like one) that don't always include all of the API's
This is part of what I'm trying to get consensus on with this issue so that for OTel v2.0 we can just say -- hey the supported browsers are blah, and if your using some older / other environment that doesn't support at least this level then we don't support that. And as you can see from the discussions while I was trying to push for ES2020 some want to go back to ES2018 (which means that the Compression API would not exist in either of these).
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.
@MSNev the Compression Streams API isn't tied to a specific ECMAScript version but rather individual browser versions. Let me know if I am missing something.
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.
The PR avoids using CompressionStream API if it's not supported, even if it is configured on the exporter. It falls back gracefully to send without any compression. The only time it would fail is when a browser like environment claims support but implements it incorrectly. I can try and see that case is handled well. However, beyond that if there are any possibility of failure let me know.
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.
The PR avoids using CompressionStream API if it's not supported
Can you identify where this is as I missed seeing the check, which is all I was talking about with my initial comment on this thread.
the Compression Streams API isn't tied to a specific ECMAScript version but rather individual browser versions. Let me know if I am missing something.
Understand that, but it is indirectly linked (in time) based on what "runtimes" (and therefore versions for browsers) in that if some runtime is only targeting ESXXX support then it may not necessarily support the current environments of browsers.... eg. something like Electron is not a browser, but it provides a browser like runtime so you can write the code (mostly) once and host it on the web or locally, but the Electron container won't support everything the browser does... (Note: I'm not an electron developer so I don't specifically know if it supports the Compression API or what it does or does not support -- so some of this is hand-wavy and generic comments about detecting support at runtime.)
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.
Can you identify where this is as I missed seeing the check
Oh! Yes, I had to put the check outside of compressContent function so that I can pass the headers appropriately. Please see: https://github.com/open-telemetry/opentelemetry-js/pull/4493/files#diff-2aacfc9dbdc5a63485aad503e0de3cd057da36801b4d109232cf233fcceddcadR165
Which problem is this PR solving?
This introduces gzip compression support to the exporters in the browser case. The change use CompressionStream API available in the modern browsers and does not introduce any additional 3rd party dependency.
Note that this support exists for the node case already.
Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: