-
Notifications
You must be signed in to change notification settings - Fork 851
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
Add compressor SPI to support additional compression algos #5990
Add compressor SPI to support additional compression algos #5990
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5990 +/- ##
=========================================
Coverage 91.12% 91.13%
- Complexity 5736 5742 +6
=========================================
Files 628 630 +2
Lines 16810 16844 +34
Branches 1662 1664 +2
=========================================
+ Hits 15318 15350 +32
- Misses 1028 1029 +1
- Partials 464 465 +1 ☔ View full report in Codecov by Sentry. |
Draft PR to add a zstd implementation to contrib: open-telemetry/opentelemetry-java-contrib#1108 |
I think this seems like a great approach. One very small comment: it might be better not to pull in another test dependency, but just create a fake compressor (maybe one that just base64s or something if you want it to change the payload) for testing. Only better in that it's one less dependency that has to be maintained/upgraded/etc. |
Nice suggestion. I pushed a commit to use base64 to test the mechanism and removed the lz4 dependency. |
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.
👍🏽
Related to #5196.
I would say this closes the issue, but we might also consider adding a contrib artifact containing one or more popular alternatives to gzip.
If folks agree on the direction, I'll followup on this to extend support to the gRPC version of the OTLP exporters.