[EXPORTER] Delegate all API calls of gRPC into `opentelemetry_exporter_otlp_grpc_client`, and make it contains all symbols needed. #2005

owent · 2023-02-24T17:47:36Z

Fixes #1998

Changes

Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed.
Add CI job to build shared libraries with static gRPC.
Add unit test to check "example_otlp_grpc_log" sample not running. libopentelemetry_exporter_otlp_grpc_log.so and libopentelemetry_exporter_otlp_grpc.so library symbols are mixing/overlapping when used together. #1603 .

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

codecov · 2023-02-24T18:02:56Z

Codecov Report

Merging #2005 (33a625e) into main (7887d32) will decrease coverage by 0.02%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2005      +/-   ##
==========================================
- Coverage   87.19%   87.17%   -0.02%     
==========================================
  Files         166      166              
  Lines        4784     4784              
==========================================
- Hits         4171     4170       -1     
- Misses        613      614       +1

see 1 file with indirect coverage changes

.vscode/launch.json

exporters/otlp/BUILD

VivekSubr · 2023-02-25T06:13:05Z

Changes are in only in bazel BUILD, no changes are required in any CMake file?

owent · 2023-02-25T06:39:35Z

Changes are in only in bazel BUILD, no changes are required in any CMake file?

The main change is moving gRPC callings into otlp_grpc_client.cc , and it changes the dependencies of otlp_grpc_client. With cmake, otlp_grpc_client already linked opentelemetry_proto before.

@VivekSubr I have reproduced #1998 in my building environment and verified it. Could you try this PR or try main branch later after this PR is merged?

VivekSubr · 2023-02-25T14:19:26Z

@owent - cloned your branch and verified it builds, thank you!

lalitb · 2023-02-27T22:46:31Z

@owent Just curious, if you know how was it working in CI env, and also our local setup. As I could see opentelemetry_exporter_otlp_grpc_client having gRPC++ symbols locally in WSL ubuntu 22.04. OR is it anything to do with compiler toolchain in customer environment ?

owent · 2023-02-28T08:08:36Z

@owent Just curious, if you know how was it working in CI env, and also our local setup. As I could see opentelemetry_exporter_otlp_grpc_client having gRPC++ symbols locally in WSL ubuntu 22.04. OR is it anything to do with compiler toolchain in customer environment ?

This problem happens only when we built gRPC as static libraries and build otel-cpp as dynamic libraries.In CI jobs, we only build both gRPC and otel-cpp as static or dynamic libraries. Do you think we should add a jobs to check this situation?

lalitb · 2023-03-02T03:45:04Z

exporters/otlp/src/otlp_grpc_client.cc

@@ -103,6 +103,60 @@ std::unique_ptr<grpc::ClientContext> OtlpGrpcClient::MakeClientContext(
 return context;
 }

+std::unique_ptr<grpc::CompletionQueue> OtlpGrpcClient::MakeCompletionQueue()


why is this method added, as it is not been used anywhere - to prevent stripping of grpc symbols ?

I plan use it to implement concurrency exporting in the future. Also , the grpc::CompletionQueue is also used by Export .I think we better export it.

lalitb · 2023-03-02T05:08:51Z

This problem happens only when we built gRPC as static libraries and build otel-cpp as dynamic libraries.

Thanks @owent . This is strange, as I normally use this setup locally on my machine ( i.e., linking dynamic otel-cpp libraries with static gRPC) but never had this error :) But if @VivekSubr is validating the problem is solved with the PR, it should be fine :)

owent · 2023-03-03T17:29:13Z

Sorry but some commits are missing in this PR, I will push them again on next Monday.

owent · 2023-03-07T02:26:16Z

All commits are pushed now.

…_client`, and make it contains all symbols needed. Signed-off-by: owent <[email protected]>

Signed-off-by: owent <[email protected]>

This reverts commit 1743bc8d3a1fa5c18a0d8f87d304245cb1cb26de. Signed-off-by: WenTao Ou <[email protected]>

Signed-off-by: owent <[email protected]>

lalitb · 2023-03-24T06:34:11Z

Sorry for the delay in reviewing this. The changes look good in general, I just want to test these changes as I always have the working setup of dynamic otel-cpp libraries with static gRPC. Will be completing the review in this week.

owent · 2023-03-24T07:28:23Z

Sorry for the delay in reviewing this. The changes look good in general, I just want to test these changes as I always have the working setup of dynamic otel-cpp libraries with static gRPC. Will be completing the review in this week.

This PR add a ci job cmake_otprotocol_shared_libs_with_static_grpc_test to test it.

marcalff

Looks good to me.

My only question is about OtlpGrpcClient::MakeCompletionQueue(),
as already mentioned by @lalitb

Please merge with a recent main, to keep the PR up to date, and check CI.

owent · 2023-04-06T16:18:09Z

Looks good to me.

My only question is about OtlpGrpcClient::MakeCompletionQueue(), as already mentioned by @lalitb

Please merge with a recent main, to keep the PR up to date, and check CI.

Sorry , I missed the comment before. I plan use it to implement concurrency exporting in the future. Also , the grpc::CompletionQueue is also used by Export of gRPC services. I think we better export it.

marcalff · 2023-04-07T06:43:54Z

Sorry , I missed the comment before. I plan use it to implement concurrency exporting in the future. Also , the grpc::CompletionQueue is also used by Export of gRPC services. I think we better export it.

Ok, approved.

marcalff · 2023-04-08T00:33:41Z

@lalitb @ThomsonTan - Any chance this PR can go in 1.9.0 ?

lalitb · 2023-04-12T14:44:21Z

@lalitb @ThomsonTan - Any chance this PR can go in 1.9.0 ?

Looking into it now. Sorry for delay, was on vacation, back today morning.

ThomsonTan · 2023-04-12T14:56:19Z

Do we plan to include this in v1.9.0 or after that? Probably merge it after the release because it is not a trivial change.

lalitb · 2023-04-12T15:20:20Z

Do we plan to include this in v1.9.0 or after that? Probably merge it after the release because it is not a trivial change.

I agree. While changes look good going through the PR, and has been open for long time, still let's not merge it at last moment before the release. Taking lesson from previous releases where we introduced breaking changes with merges just before the release.

marcalff · 2023-04-12T15:26:52Z

Do we plan to include this in v1.9.0 or after that? Probably merge it after the release because it is not a trivial change.

I agree. While changes look good going through the PR, and has been open for long time, still let's not merge it at last moment before the release. Taking lesson from previous releases where we introduced breaking changes with merges just before the release.

Ok, will merge after 1.9.0 then.

owent requested a review from a team February 24, 2023 17:47

owent mentioned this pull request Feb 24, 2023

OTLP example fails to build with error "undefined reference to `grpc::Status::OK' " #1998

Closed

ThomsonTan reviewed Feb 24, 2023

View reviewed changes

.vscode/launch.json Outdated Show resolved Hide resolved

ThomsonTan reviewed Feb 24, 2023

View reviewed changes

exporters/otlp/BUILD Show resolved Hide resolved

esigo added the size/L Denotes a PR that changes 100-499 lines. label Feb 26, 2023

lalitb mentioned this pull request Feb 27, 2023

Prepare release v.1.9.0 (tentative: 3rd March) #2007

Closed

lalitb added this to the OpenTelemetry C++ Release v1.9.2 (Feb Release) milestone Feb 27, 2023

lalitb reviewed Mar 2, 2023

View reviewed changes

owent force-pushed the fix_1998 branch from 01bdd68 to f8465f2 Compare March 3, 2023 15:46

owent changed the title ~~Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed.~~ [WIP] Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed. Mar 3, 2023

owent force-pushed the fix_1998 branch 3 times, most recently from 0f6b412 to b0a0e2b Compare March 6, 2023 03:21

owent changed the title ~~[WIP] Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed.~~ Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed. Mar 6, 2023

ThomsonTan mentioned this pull request Mar 6, 2023

Prepare v1.8.3 release #2018

Merged

3 tasks

lalitb modified the milestones: OpenTelemetry C++ Feb 2023 release , OpenTelemetry C++ March 2023 release Mar 9, 2023

owent force-pushed the fix_1998 branch 2 times, most recently from 4b97264 to b0ebea7 Compare March 16, 2023 06:35

owent added 2 commits March 20, 2023 11:12

Delegate all API calls of gRPC into `opentelemetry_exporter_otlp_grpc…

b9acee5

…_client`, and make it contains all symbols needed. Signed-off-by: owent <[email protected]>

Fix links in bazel

cdfe4d0

Signed-off-by: owent <[email protected]>

owent added 2 commits March 20, 2023 11:12

Revert "Auxiliary commit to revert individual files from 399ed00"

dff5da5

This reverts commit 1743bc8d3a1fa5c18a0d8f87d304245cb1cb26de. Signed-off-by: WenTao Ou <[email protected]>

Fix gRPC static build script

e497b24

Signed-off-by: owent <[email protected]>

owent force-pushed the fix_1998 branch from b0ebea7 to e497b24 Compare March 20, 2023 03:13

ays7 mentioned this pull request Mar 22, 2023

mac shlib dependencies fixes #1891

Closed

Merge branch 'main' into fix_1998

e01d900

owent force-pushed the fix_1998 branch from 3af75f3 to e01d900 Compare March 28, 2023 06:36

marcalff approved these changes Apr 6, 2023

View reviewed changes

Merge branch 'main' into fix_1998

88eadf6

Merge branch 'main' into fix_1998

9db7266

owent mentioned this pull request Apr 12, 2023

[RELEASE] Release version 1.9.0 #2091

Merged

3 tasks

Merge branch 'main' into fix_1998

1d20c56

lalitb approved these changes Apr 14, 2023

View reviewed changes

lalitb added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Apr 14, 2023

Merge branch 'main' into fix_1998

33a625e

marcalff merged commit 6a3ee16 into open-telemetry:main Apr 14, 2023

owent deleted the fix_1998 branch April 17, 2023 02:11

marcalff changed the title ~~Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed.~~ [EXPORTER] Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed. May 23, 2023

chenrui333 mentioned this pull request May 28, 2023

opentelemetry-cpp 1.9.1 Homebrew/homebrew-core#132204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EXPORTER] Delegate all API calls of gRPC into `opentelemetry_exporter_otlp_grpc_client`, and make it contains all symbols needed. #2005

[EXPORTER] Delegate all API calls of gRPC into `opentelemetry_exporter_otlp_grpc_client`, and make it contains all symbols needed. #2005

owent commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

VivekSubr commented Feb 25, 2023

owent commented Feb 25, 2023 •

edited

Loading

VivekSubr commented Feb 25, 2023

lalitb commented Feb 27, 2023

owent commented Feb 28, 2023

lalitb Mar 2, 2023 •

edited

Loading

owent Apr 6, 2023

lalitb commented Mar 2, 2023

owent commented Mar 3, 2023

owent commented Mar 7, 2023

lalitb commented Mar 24, 2023 •

edited

Loading

owent commented Mar 24, 2023

marcalff left a comment

owent commented Apr 6, 2023

marcalff commented Apr 7, 2023

marcalff commented Apr 8, 2023

lalitb commented Apr 12, 2023

ThomsonTan commented Apr 12, 2023

lalitb commented Apr 12, 2023

marcalff commented Apr 12, 2023

[EXPORTER] Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed. #2005

[EXPORTER] Delegate all API calls of gRPC into opentelemetry_exporter_otlp_grpc_client, and make it contains all symbols needed. #2005

Conversation

owent commented Feb 24, 2023 • edited Loading

Changes

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

VivekSubr commented Feb 25, 2023

owent commented Feb 25, 2023 • edited Loading

VivekSubr commented Feb 25, 2023

lalitb commented Feb 27, 2023

owent commented Feb 28, 2023

lalitb Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

owent Apr 6, 2023

Choose a reason for hiding this comment

lalitb commented Mar 2, 2023

owent commented Mar 3, 2023

owent commented Mar 7, 2023

lalitb commented Mar 24, 2023 • edited Loading

owent commented Mar 24, 2023

marcalff left a comment

Choose a reason for hiding this comment

owent commented Apr 6, 2023

marcalff commented Apr 7, 2023

marcalff commented Apr 8, 2023

lalitb commented Apr 12, 2023

ThomsonTan commented Apr 12, 2023

lalitb commented Apr 12, 2023

marcalff commented Apr 12, 2023

[EXPORTER] Delegate all API calls of gRPC into `opentelemetry_exporter_otlp_grpc_client`, and make it contains all symbols needed. #2005

[EXPORTER] Delegate all API calls of gRPC into `opentelemetry_exporter_otlp_grpc_client`, and make it contains all symbols needed. #2005

owent commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

owent commented Feb 25, 2023 •

edited

Loading

lalitb Mar 2, 2023 •

edited

Loading

lalitb commented Mar 24, 2023 •

edited

Loading