[EXPORTER] Gzip compression support for OTLP/HTTP and OTLP/gRPC exporter

[perhapsmaple]

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:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[perhapsmaple]

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.

[lalitb]

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

[lalitb]

@perhapsmaple - The CI seems to be successful. Is this ready for review?

[marcalff]

Thanks for the feature.

It is nicely done, I must say you have a pretty good grasp of the code organization, and the patch fits nicely in the code base.

Please update file docs/requirements.md, to detail the new dependency on ZLIB.

Please provide a CHANGELOG.md entry, with an important notice that the build now depends on a new ZLIB dependency.

Typically, new features need to be protected by a ENABLE_COMPRESSION_PREVIEW flag.

In this case:

• ZLIB itself is very mature, and introduces low risks
• compression is set to none by default

so I think we can do without a ENABLE_COMPRESSION_PREVIEW build option, which adds complexity when adding it, and would add complexity when removing it in one or two releases.

We will discuss this in maintainers meeting, with approval to follow.

[marcalff]

@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.

[perhapsmaple]

@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.

[marcalff]

@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]

Now that compression support is optional, the CI no longer tests with it.

See file ci/do_ci.sh

See entries like

elif [[ "$1" == "cmake.maintainer.sync.test" ]]; then

and friends (cmake.maintainer.*)

Add:

-DWITH_OTLP_HTTP_COMPRESSION=ON

This will run maintainer tests with compression.

[marcalff]

@perhapsmaple See previous comment for ci/do_ci.sh.

[perhapsmaple]

@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.

[marcalff]

@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.

[marcalff]

Approving review, and adding a temporary do-not-merge flag (don't worry), because we are making a 1.14.2 release right now.

[marcalff]

LGTM, thanks for the compression support.

[lalitb]

@perhapsmaple - Do you think we can use the in-place compression of the data without using a separate output buffer compressed_body, as suggested here - https://stackoverflow.com/questions/12398377/is-it-possible-to-have-zlib-read-from-and-write-to-the-same-memory-buffer/12412863#12412863. This is suggested by "Mark Adler", the author of zlib, and would be better memory optimzation in the hot-path of upload.

If not feasible in this PR., good to add a TODO and create an issue for tracking, so some one can pick it up.

[lalitb]

LGTM. Excellent work. Would be great to have more contributions from you :)

[perhapsmaple]

@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.

[marcalff]

@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).