opentelemetry-cpp
opentelemetry-cpp copied to clipboard
[EXPORTER] Allow to share gRPC clients between OTLP exporters.
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::ChannelinOtlpGrpcClientso 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.mdupdated for non-trivial changes - [x] Unit tests have been added
- [ ] Changes in public API reviewed
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
@@ 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%> (ø) |
- Flaky Tests Detection - Detect and resolve failed and flaky tests
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 @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.
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 |
Is there any change to include this PR into next release?
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.
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.
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.
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.
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.
Ping @lalitb . Issue with ABSEIL and ABI is resolved, please take another look.
There is a typo -> shitdown should be shutdown :) - made some of my coworkes chuckle today :) hahahaha