Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

owent
Copy link
Member

@owent owent commented Aug 29, 2024

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:

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

@owent owent requested a review from a team August 29, 2024 07:00
Copy link

codecov bot commented Aug 29, 2024

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 (497eaf4) to head (7b2f29e).
Report is 164 commits behind head on main.

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

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3041      +/-   ##
==========================================
+ Coverage   87.12%   87.83%   +0.72%     
==========================================
  Files         200      195       -5     
  Lines        6109     6146      +37     
==========================================
+ Hits         5322     5398      +76     
+ Misses        787      748      -39     
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%> (ø)

... and 116 files with indirect coverage changes

@lalitb
Copy link
Member

lalitb commented Aug 29, 2024

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 :)

@owent
Copy link
Member Author

owent commented Aug 30, 2024

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 owent changed the title Allow to share gRPC clients between OTLP exporters. [WIP] Allow to share gRPC clients between OTLP exporters. Aug 30, 2024
@owent owent changed the title [WIP] Allow to share gRPC clients between OTLP exporters. Allow to share gRPC clients between OTLP exporters. Aug 30, 2024
Copy link

netlify bot commented Sep 12, 2024

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

Name Link
🔨 Latest commit 7b2f29e
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/67363c6e0d444d0008215bd6

@marcalff marcalff requested a review from a team as a code owner September 23, 2024 06:52
@owent
Copy link
Member Author

owent commented Sep 24, 2024

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

@marcalff
Copy link
Member

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 marcalff changed the title Allow to share gRPC clients between OTLP exporters. [EXPORTER] Allow to share gRPC clients between OTLP exporters. Sep 24, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@ThomsonTan ThomsonTan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@marcalff
Copy link
Member

marcalff commented Nov 4, 2024

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.

@owent
Copy link
Member Author

owent commented Nov 14, 2024

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.

@marcalff
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:please-review This PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EXPORTER] Allow construct OTLP gRPC exporters from existing grpc::Channel
5 participants