[BUILD] fix build of otlp_grpc_exporter_test with modern grpc #3307

dbarker · 2025-03-21T04:44:16Z

Changes

makes grpc link publicly to the grpc client
remove otlp_grpc_exporter_test's link to gRPC::grpc++
removes extra protobuf links to tests
adds curl link to the zipkin test

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

netlify · 2025-03-21T04:44:29Z

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

Name	Link
🔨 Latest commit	`1c36168`
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/67e0f0412fba190008746dfa

codecov · 2025-03-21T04:57:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.52%. Comparing base (b4328c0) to head (1c36168).
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3307   +/-   ##
=======================================
  Coverage   89.52%   89.52%           
=======================================
  Files         210      210           
  Lines        6522     6522           
=======================================
  Hits         5838     5838           
  Misses        684      684

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

owent

LGTM, thanks.
This issue was introduced by PR #3219. Could you please add some comments describing how to reproduce it?
Otherwise, other contributors might reintroduce the same issue in the future, and we would have difficulty identifying it.

dbarker · 2025-03-21T16:22:09Z

LGTM, thanks. This issue was introduced by PR #3219. Could you please add some comments describing how to reproduce it? Otherwise, other contributors might reintroduce the same issue in the future, and we would have difficulty identifying it.

Thanks! Here are the steps to reproduce the build error in a linux container.

build and run a dev container setting the latest grpc version with compatible protobuf and abseil versions.

cd <path-to-opentelemetry-cpp>
docker build --build-arg GRPC_VERSION="v1.71.0" --build-arg PROTOBUF_VERSION="29.0" --build-arg ABSEIL_CPP_VERSION="20240722.1" --build-arg USER_UID="$(id -u)" --build-arg USER_GID="$(id -g)"  -t opentelemetry-cpp-dev:latest -f ./.devcontainer/Dockerfile.dev .
docker run -it -v "$PWD:/workspaces/opentelemetry-cpp" opentelemetry-cpp-dev:latest bash

build the otlp exporter and run tests

cmake -S . -B ~/build -DWITH_OTLP_GRPC=ON
cmake --build ~/build 
ctest --test-dir ~/build

I'll log an issue to upgrade to the latest release of vcpkg and use newer versions of grpc in ci. The link error solved by this PR should exist when linking to grpc v1.67.0+.

dbarker · 2025-03-21T18:15:57Z

The build failure is in an unrelated test of the otlp file exporter that passed in the previous run on the PR.

…o grpc client. remove extra protobuf links to tests. add missing curl link to zipkin test

dbarker · 2025-03-24T05:54:44Z

@owent after more testing I found that the grpc link to the client should be public since the client is a public interface and exposes grpc headers. Making the link public also allows the tests to get the transitive dependency (solves a link error when building shared libraries on later versions of grpc). While I was in that file I've also removed extra protobuf links to tests since opentelemetry_proto exposes it publicly. I've tested this on old versions of grpc (1.49.2) and the latest (1.71.0). The zipkin exporter test needed curl and I've added it the branch.

owent · 2025-03-24T06:56:25Z

exporters/otlp/CMakeLists.txt

-      opentelemetry_exporter_otlp_grpc_client BEFORE
-      PUBLIC "$<BUILD_INTERFACE:${GRPC_INCLUDE_DIRECTORY}>")
-  endif()
+           opentelemetry_proto_grpc gRPC::grpc++


gRPC client definitions are not exposed to users, so it's fine to keep the gRPC dependency private. Alternatively, we could use PUBLIC $<BUILD_INTERFACE:gRPC::grpc++> PRIVATE $<INSTALL_INTERFACE:gRPC::grpc++> to expose the include directories internally. Symbol conflicts don't only cause issues during compilation or linking but also will result in runtime crashes.

More details can be found at #1603 , #1891 , #1998

Thanks for the details and appreciate the feedback. It is definitely a challenge to get cmake linking everything under the various build configurations we have.

The feature to create and share a grpc client among exporters is a really nice one and will require users to include the grpc headers brought in by OtlpGrpcClient, which is included by the OtlpGrpcClientFactory. At the moment they would need to explicitly link in gRPC::grpc++ to their project to get those headers. I'll propose a change in a separate PR to add those headers to the install interface of the grpc client target.

Do you think we can move OtlpGrpcClientReferenceGuard out of otlp_grpc_client.h, so we don't need to include otlp_grpc_client.h in headers of OtlpGrpcClientFactory?

I raised #3321 , could you please have a look when you have time?

I think we'd need to remove all grpc from otlp_grpc_client.h using pmpl and some generic status codes. Alternatively we could give the client a base class and use it in the constructors of the exporters and return from the factory - then deprecate the constructors and create method with the OtlpGrpcClient shard ptr.

actually #3321 is a good solution as long as the client methods are accessed outside the scope of the exporters. good call.

dbarker · 2025-03-24T22:20:00Z

After more testing I found that building against grpc 1.71.0 is working on the current main branch. I'm not able to reproduce the link error originally reported example here.

I suspect the root issue was an ill-configured grpc install in my docker container and github runner. I had a typo on the arguments to the setup-grpc.sh script and abseil should be abseil-cpp

opentelemetry-cpp/.devcontainer/Dockerfile.dev

Line 33 in 2c9b68f

    
           && bash ci/setup_grpc.sh -r $GRPC_VERSION -s $CXX_STANDARD -p protobuf -p abseil

I'm going to close this PR and open separate PRs for:

Making the grpc headers public in the installed otlp grpc client target (so grpc will not need to be linked explicit when creating a shared client)
Fix the missing curl link to the zipkin exporter test
Fix the grpc install in the devcontainer docker file.

dbarker · 2025-03-24T22:20:18Z

Closing.

remove otlp_grpc_exporter_test link to gRPC::grpc++

4ed22ee

dbarker requested a review from a team as a code owner March 21, 2025 04:44

owent approved these changes Mar 21, 2025

View reviewed changes

Merge branch 'main' into fix_link_error_with_latest_grpc

7f325ce

dbarker changed the title ~~remove otlp_grpc_exporter_test link to gRPC::grpc++~~ [BUILD] fix build of otlp_grpc_exporter_test with modern grpc Mar 21, 2025

make grpc's link public to the grpc client. make ext's link private t…

1c36168

…o grpc client. remove extra protobuf links to tests. add missing curl link to zipkin test

owent requested changes Mar 24, 2025

View reviewed changes

dbarker closed this Mar 24, 2025

dbarker mentioned this pull request Mar 25, 2025

unable to build with grpc 1.71.0 #3306

Closed

dbarker deleted the fix_link_error_with_latest_grpc branch April 25, 2025 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BUILD] fix build of otlp_grpc_exporter_test with modern grpc #3307

[BUILD] fix build of otlp_grpc_exporter_test with modern grpc #3307

Uh oh!

dbarker commented Mar 21, 2025 •

edited

Loading

Uh oh!

netlify bot commented Mar 21, 2025 •

edited

Loading

Uh oh!

codecov bot commented Mar 21, 2025 •

edited

Loading

Uh oh!

owent left a comment

Uh oh!

dbarker commented Mar 21, 2025 •

edited

Loading

Uh oh!

dbarker commented Mar 21, 2025

Uh oh!

dbarker commented Mar 24, 2025

Uh oh!

owent Mar 24, 2025 •

edited

Loading

Uh oh!

dbarker Mar 24, 2025

Uh oh!

owent Mar 25, 2025 •

edited

Loading

Uh oh!

dbarker Mar 25, 2025

Uh oh!

dbarker Mar 25, 2025

Uh oh!

dbarker commented Mar 24, 2025

Uh oh!

dbarker commented Mar 24, 2025

Uh oh!

Uh oh!

[BUILD] fix build of otlp_grpc_exporter_test with modern grpc #3307

[BUILD] fix build of otlp_grpc_exporter_test with modern grpc #3307

Uh oh!

Conversation

dbarker commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

netlify bot commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

codecov bot commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

owent left a comment

Choose a reason for hiding this comment

Uh oh!

dbarker commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbarker commented Mar 21, 2025

Uh oh!

dbarker commented Mar 24, 2025

Uh oh!

owent Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbarker Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

owent Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbarker Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

dbarker Mar 25, 2025

Choose a reason for hiding this comment

Uh oh!

dbarker commented Mar 24, 2025

Uh oh!

dbarker commented Mar 24, 2025

Uh oh!

Uh oh!

dbarker commented Mar 21, 2025 •

edited

Loading

netlify bot commented Mar 21, 2025 •

edited

Loading

codecov bot commented Mar 21, 2025 •

edited

Loading

dbarker commented Mar 21, 2025 •

edited

Loading

owent Mar 24, 2025 •

edited

Loading

owent Mar 25, 2025 •

edited

Loading