open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
890 stars 426 forks source link

[BUILD] allow building with -DWITH_OTLP_HTTP_COMPRESSION=OFF without zlib #3120

Closed cfstras closed 3 weeks ago

cfstras commented 3 weeks ago

Issue

Currently, when building -DWITH_OTLP_HTTP_COMPRESSION=OFF, and not providing the libz dependency, we get this error:

...\opentelemetry-cpp\ext\src\http\client\curl\http_
client_curl.cc(6,10): error C1083: Cannot open include file: 'zconf.h': No such file or directory [...opentelemetry-cpp\build\ext\src\http\client\curl\opentelemetry_http_cli
ent_curl.vcxproj]

Changes

This fixes the issue by moving the #include <zconf.h> inside the corresponding #ifdef block, letting the build pass.

Do I need to do any of these? LMK, happy to :)

  • [ ] CHANGELOG.md updated for non-trivial changes
  • [ ] Unit tests have been added
  • [ ] Changes in public API reviewed
netlify[bot] commented 3 weeks ago

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
Latest commit d05928081e6b91a6e928f3a846f9f48e772bed9e
Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/6728f62d3eacbe0008cc31f0
marcalff commented 3 weeks ago

This makes sense.

Note however that ENABLE_OTLP_COMPRESSION_PREVIEW is what we call a "feature flag":

By the time the compression code is deemed stable, the feature flag will be removed, so the dependency at build time on zlib will become mandatory.

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.88%. Comparing base (497eaf4) to head (d059280). Report is 152 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/3120/graphs/tree.svg?width=650&height=150&src=pr&token=FJESTYQ2AD&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/3120?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ Coverage Diff @@ ## main #3120 +/- ## ========================================== + Coverage 87.12% 87.88% +0.76% ========================================== Files 200 195 -5 Lines 6109 6137 +28 ========================================== + Hits 5322 5393 +71 + Misses 787 744 -43 ``` [see 99 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/3120/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
cfstras commented 2 weeks ago

Hi @marcalff Thanks for the note. I guess once it's stable, we'll have to bite the bullet and add that to our build scripts.

FWIW, for our situation compression is pretty unnecessary, we have a native binary running alongside a DataDog daemon on the same box, so data never leaves the box. More dependencies just complicate the already quite complex build process. Anyway, thanks for maintaining this, it's very helpful for us! :)