opentelemetry-js
opentelemetry-js copied to clipboard
Adding gzip compression support in browser case
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
- Updated sendWithXhr function to take additional parameter for compression. This parameter is not added at the end, so this could potentially qualify as a breaking change. however, I have also modified its only usage in otlp-proto-exporter-base so I'm not sure if this still qualifies to be a breaking change.
- I've used Copilot to generate the code for compressContent, let me know if you find anything odd.
Type of change
Please delete options that are not relevant.
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [x] This change requires a documentation update
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
- I tested compressContent function separately, and was able to uncompress the content using gunzip command line
- I also tested examples/opentelemetry-web/xml-http-request enabling gzip compression in the OTLPTraceExporter
Checklist:
- [ ] Followed the style guidelines of this project
- [x] Unit tests have been added
- [ ] Documentation has been updated
Codecov Report
Attention: Patch coverage is 87.75510% with 6 lines in your changes are missing coverage. Please review.
Project coverage is 91.26%. Comparing base (
2610122) to head (ca734e5). Report is 35 commits behind head on main.
:exclamation: Current head ca734e5 differs from pull request most recent head ca10067
Please upload reports for the commit ca10067 to get more accurate results.
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
| Files | Coverage Δ | |
|---|---|---|
| ...ter-base/src/platform/node/OTLPExporterNodeBase.ts | 50.00% <ø> (ø) |
|
| ...kages/otlp-exporter-base/src/platform/node/util.ts | 63.80% <100.00%> (ø) |
|
| ...erimental/packages/otlp-exporter-base/src/types.ts | 44.44% <100.00%> (ø) |
|
| ...es/otlp-exporter-base/src/platform/browser/util.ts | 54.36% <86.66%> (ø) |
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.
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? @martinkuba @MSNev can you approve this PR when you get a chance?
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: scheler / name: Santosh Cheler (c06426af4e2cea16291fa17044c341b9134506c2, 196672cb4033f2f84ddde4b7d5c76af767206e5f, 554154b24f9c4d84f2780796ef139c2f4e8e7926, 6e4bebb02a752c206f1e9b2124bf2c3a9fe2e88c, 2ffcf6cc832f75d636bcb27ea0a04c340aeccaa4, c087270efbd214f79c932fff35d1f42cb504e5a3, 066707ca6b4da2489e3eb3495958dc158c5a4f49, ad1f9733e71a2c595a5d1b91ddeebfce5e9a2a25, 1feb3eb32c698968d77e665dc34cd6f93886b64a, bd2b25bad93902d029b9f1498e883b50ac2fb992, 80db59fc10893e2c40d71f92143adbe981b62fd3, 67453e03e3d77f91ba7f6a2a20697883546cf0f6, ca10067b21edf1212c4f71c153d331d5b94cf1a7, 135db53b7dc8a5b8965a61d06c68fcb4de83f910, 8a2e225997f0c8cb850ee500bcde415ed0e2ea31, 40f8094cc616e4bb9e961dff13c24fa1e3a769bd, e97b1dea111674e15315b8ddead2458f62db58e2, 383c7ac7f696d00d558e2d16ded9a3ba74a70211, 667183f69d23eb5251052b6a9b392ad3dd75b2d2, 8b07fd5f109e41d17838d25c499229b5d3a24285, c0273d34b5a1d4c00e9b207716cbad622772a0df, ca734e575274ad974355ee9a63e9ab1a12013fec, 56bb3ca6fc10e70c72d1b8803de71d4fc5b78f4f)
@pichlermarc can you review this PR when you get a chance? There is a commit https://github.com/open-telemetry/opentelemetry-js/commit/8a2e225997f0c8cb850ee500bcde415ed0e2ea31 from you that needs your attention.
/easycla