opentelemetry-cpp icon indicating copy to clipboard operation
opentelemetry-cpp copied to clipboard

[EXPORTER] Allow to share gRPC clients between OTLP exporters.

Open owent opened this issue 1 year ago • 8 comments

Fixes #3010

Changes

  • Add APIs to allow to share gRPC clients between OTLP exporters.
  • gRPC depends abseil-cpp, so we always enable abseil-cpp when have WITH_OTLP_GRPC
  • Keep grpc::Channel in OtlpGrpcClient so we can share it between multiple exporters.

We can use OtlpGrpcClientFactory::Create() to create an OtlpGrpcClient. This client can then be passed to OtlpGrpcExporterFactory::Create(), OtlpGrpcLogRecordExporterFactory::Create(), and OtlpGrpcMetricExporterFactory::Create(). By doing this, all gRPC exporters created by these methods will share the same OtlpGrpcClient and utilize the same grpc::CompletionQueue and grpc::Channel.

When the exporters call Shutdown, the OtlpGrpcClient will only call ForceFlush and will not Shutdown unless all the exporters using this client object are also shutdown.We can use OtlpGrpcClientFactory::Create() to create OtlpGrpcClient now, and then pass it to OtlpGrpcExporterFactory::Create() , OtlpGrpcLogRecordExporterFactory::Create() and OtlpGrpcMetricExporterFactory::Create() , so all gRPC exporters created by these methods will share the same OtlpGrpcClient and use the same grpc::CompletionQueue and grpc::Channel.

When the exporters call Shutdown, the OtlpGrpcClient just call ForceFlush but do not Shutdown unless all the exporters use this client object are shutdown.

Usage example:

OtlpGrpcClientOptions grpc_opts;
grpc_opts.endpoint = "localhost:12345";
// ... other initializations

OtlpGrpcExporterOptions trace_opts = grpc_opts;
OtlpGrpcLogRecordExporterOptions logs_opts = grpc_opts;

nostd::shared_ptr<OtlpGrpcClient> shared_client = OtlpGrpcClientFactory::Create(OtlpGrpcLogRecordExporterOptions());
auto trace_exporter = OtlpGrpcExporterFactory::Create(trace_opts, shared_client);
auto logs_exporter = OtlpGrpcLogRecordExporterFactory::Create(logs_opts, shared_client);

// ...

For significant contributions please make sure you have completed the following items:

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

owent avatar Aug 29 '24 07:08 owent

Codecov Report

Attention: Patch coverage is 64.93506% with 27 lines in your changes missing coverage. Please review.

Project coverage is 87.83%. Comparing base (a388e87) to head (62756bd). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...metry/nostd/internal/absl/types/internal/variant.h 59.71% 27 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3041   +/-   ##
=======================================
  Coverage   87.83%   87.83%           
=======================================
  Files         195      195           
  Lines        6146     6146           
=======================================
  Hits         5398     5398           
  Misses        748      748           
Files with missing lines Coverage Δ
...lemetry/nostd/internal/absl/base/internal/invoke.h 100.00% <ø> (ø)
...try/nostd/internal/absl/types/bad_variant_access.h 100.00% <ø> (ø)
.../opentelemetry/nostd/internal/absl/types/variant.h 100.00% <100.00%> (ø)
...pentelemetry/nostd/internal/absl/utility/utility.h 100.00% <100.00%> (ø)
api/include/opentelemetry/nostd/variant.h 66.67% <ø> (ø)
...metry/nostd/internal/absl/types/internal/variant.h 74.57% <59.71%> (ø)
---- 🚨 Try these New Features:

codecov[bot] avatar Aug 29 '24 07:08 codecov[bot]

Thanks @owent. It would be helpful to have API details in the PR description. And possibly an example here to understand the usage - as a separate PR if not possible here :)

lalitb avatar Aug 29 '24 09:08 lalitb

Thanks @owent. It would be helpful to have API details in the PR description. And possibly an example here to understand the usage - as a separate PR if not possible here :)

Thanks, the details are added. I can add examples in another PR later.

owent avatar Aug 30 '24 02:08 owent

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

Name Link
Latest commit 62756bde371a91ee8354e7c1018deeac241da419
Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/673bb6aa5bcffa00089d108f

netlify[bot] avatar Sep 12 '24 15:09 netlify[bot]

Is there any change to include this PR into next release?

owent avatar Sep 24 '24 07:09 owent

Is there any change to include this PR into next release?

Hi @owent.

Going though the code review, and I would like to include it in the next release as well.

In the PR description, there are two descriptions for "In async mode, ..." but none for the sync mode.

Could you revise the text for clarity, I assume you wanted two sections, one for sync and one for async.

marcalff avatar Sep 24 '24 21:09 marcalff

Is there any change to include this PR into next release?

Hi @owent.

Going though the code review, and I would like to include it in the next release as well.

In the PR description, there are two descriptions for "In async mode, ..." but none for the sync mode.

Could you revise the text for clarity, I assume you wanted two sections, one for sync and one for async.

Sorry, the first version of this PR do not change the behavior of sync mode. I forgot to remove the descriptions before.

owent avatar Sep 25 '24 13:09 owent

These changes are significant, so it was decided in the community meeting to have at least two approvals since the PR is larger and somewhat intrusive. I'll review it later this week.

lalitb avatar Oct 08 '24 15:10 lalitb

Sorry for requesting change. This is for - #3041 (comment)

If we are agreeing to not being ABI compliant when using OTLP/gRPC exporter, it would be better to update our ABI document to reflect that as part of this PR. No concern as such with the implementation.

I think this is now resolved by the latest changes in internal abseil, by @owent.

Using the OTLP GRPC exporter requires external abseil, which no longer forces the API to use abseil, and therefore does not change the ABI for nostd::variant.

Let's discuss technical details in the team meeting.

marcalff avatar Nov 04 '24 20:11 marcalff

Sorry for requesting change. This is for - #3041 (comment) If we are agreeing to not being ABI compliant when using OTLP/gRPC exporter, it would be better to update our ABI document to reflect that as part of this PR. No concern as such with the implementation.

I think this is now resolved by the latest changes in internal abseil, by @owent.

Using the OTLP GRPC exporter requires external abseil, which no longer forces the API to use abseil, and therefore does not change the ABI for nostd::variant.

Let's discuss technical details in the team meeting.

Yes, the issue is resolved by making the internal Abseil-CPP namespace non-inline. I also checked all include paths and there should be no issues now.

owent avatar Nov 14 '24 03:11 owent

Ping @lalitb . Issue with ABSEIL and ABI is resolved, please take another look.

marcalff avatar Nov 15 '24 07:11 marcalff

There is a typo -> shitdown should be shutdown :) - made some of my coworkes chuckle today :) hahahaha

malkia avatar Nov 19 '24 17:11 malkia