opentelemetry-cpp
opentelemetry-cpp copied to clipboard
open-telemetry
[EXPORTER] Gzip compression support for OTLP/HTTP and OTLP/gRPC exporter
Fixes #2351
Changes
Added gzip compression as an option for the OtlpHttp and OtlpGrpc exporters.
For significant contributions please make sure you have completed the following items:
- [x]
CHANGELOG.mdupdated for non-trivial changes - [ ] Unit tests have been added
- [ ] Changes in public API reviewed
I'm not completely sure about why some builds pass and some fail. Do we need to add a separate find_package command for ZLIB. I'm a little new to CMake and Bazel and would appreciate any help.
I'm not completely sure about why some builds pass and some fail. Do we need to add a separate find_package command for ZLIB. I'm a little new to CMake and Bazel and would appreciate any help.
Yes, you need to use find_package for CMake. For Bazel, see if this helps - https://stackoverflow.com/questions/70877000/bazel-c-how-to-include-a-library-that-needs-to-be-cloned-from-github-then-buil
@perhapsmaple
Also, please indicate how testing was done.
For OTLP HTTP, a possibility is to change locally functional/otlp/func_http_main.cc to use gzip compression, and run tests against an opentelemetry-collector.
@marcalff
Testing was done with an internal message queue which collects logs from multiple services with gzip compression enabled. However, I did find a major issue in my implementation. Because I was using the simple wrappers provided by zlib for compression and decompression, it turns out the zlib does not add the gzip headers by default unless you increase the windows size manually by 16. This was not a problem during my testing as my server uses the decompress wrapper function from zlib and does not require the gzip headers to be present. The opentelemetry-collector seems to require these headers. I have pushed a commit (excuse the typos) which does solve the issue and includes the proper gzip headers. I tested it with the otlp examples and it now works properly with the opentelemetry-collector. I am also investigating whether it will be beneficial to reuse one z_stream rather than create a new z_stream object for each export. It should not matter much but I will benchmark to verify the same.
I will update the rest of the documentation, and change functional/otlp/func_http_main.cc to add tests by tomorrow.
@perhapsmaple
This PR was discussed in the maintainers meeting today.
To avoid disruption in makefiles and build scripts, we do need to define a feature flag.
Please implement the following:
- Add a WITH_OTLP_HTTP_COMPRESSION option in CMakeLists.txt, OFF by default
- Add a ENABLE_OTLP_HTTP_COMPRESSION_PREVIEW define in api/CMakeLists.txt
- In http_operation_curl.cc
- Protects the zlib path with ifdef ENABLE_OTLP_COMPRESSION_PREVIEW
- Raise an error otherwise, if asking for compression without the preview option
- Link with zlib only when WITH_OTLP_COMPRESSION is set
This applies only to the OTLP HTTP exporter, because the link changed to depend on a new library (zlib).
The OTLP GRPC exporter is not affected.
The plan is to:
- release version 1.15.0 with WITH_OTLP_HTTP_COMPRESSION = OFF by default
- later, release version 1.16.0 or better with WITH_OTLP_HTTP_COMPRESSION = ON by default
- much later, release version 1.17.0 or better with the flag removed.
@marcalff I have update the CI accordingly. I think the bazel build also has to be updated to link zlib only when built with the otlp compression flag. I'm not very familiar with bazel and would appreciate any help.
@perhapsmaple Thanks for the CMake fixes in CI.
About bazel, you are correct this should also include a feature flag in the bazel build.
We lack experience with bazel as well, and looking closely, in fact, none of the feature flags provided in CMake have a bazel equivalent in opentelemetry.
I think this PR can go as is:
- optional feature in CMake, opt in currently
- no option available for bazel
When later the code is changed to have the feature by default, the bazel build will be expanded to include zlib.
For bazel, this should be less intrusive compared to CMake, because the bazel build in general already depends on the OTLP GRPC exporter, which depends on grpc, which depends on zlib.
Approving review, and adding a temporary do-not-merge flag (don't worry), because we are making a 1.14.2 release right now.
@lalitb Sorry for the late reply, I'm a little busy till next week. I think the in place compression technique would very much help with allocations. I will take a look and implement it as soon as possible.
@perhapsmaple
No rush, thanks for all the work done already.
I am merging this PR now then, the optimization can be done separately (and at your own pace).