-
Notifications
You must be signed in to change notification settings - Fork 448
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
Merged
marcalff
merged 40 commits into
open-telemetry:main
from
owent:allow_construct_grpc_exporters_with_existed_grpc_client
Nov 18, 2024
Merged
Changes from 18 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
213e3cf
Allow to share gRPC clients between OTLP exporters.
owent 5c5f9b7
Add changelog, fix compiling problems without async
owent 37e5081
gRPC depends abseil-cp, so we always enable abseil-cpp when have `WIT…
owent c9cb2df
Fix linking error
owent 0d409d4
Share `grpc::Channel`
owent cd80d19
Fix compiling error
owent ad722f4
Make `OtlpGrpcClientReferenceGuard` always available
owent f4521f3
Try to cancel running requests when shutdown timeout
owent 38860d3
Modify `running_calls` do not depends the thread for gRPC callback.
owent b5daf3f
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 8e9d300
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 51e0964
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 40ca747
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent b2aa191
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
marcalff 3547800
Allow to share gRPC Client in sync mode
owent ef62bc0
Fix shutdown in sync mode
owent caa1743
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
marcalff 949c679
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
marcalff e1e6627
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 74ceb64
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
lalitb d5bb715
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent c9419bf
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent d4b0286
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 72e4cb3
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent edd0c16
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 3ff6e4b
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 829294e
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 94260b3
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
ThomsonTan 4367e62
Change unique_ptr and shared_ptr from `nostd::` to `std::`. Change th…
owent e2b23aa
Fix includes
owent 5142036
Do not inline otabsl to avoid namespace conflicts
owent d7266aa
Fix markdown lint
owent 603e2fa
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent adcf88e
Merge remote-tracking branch 'github/main' into allow_construct_grpc_…
owent b71c655
Add internal logs when shutdown gRPC Client
owent 1145003
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
owent 4d4d3a5
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
marcalff fe30625
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
marcalff 7b2f29e
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
marcalff 62756bd
Merge branch 'main' into allow_construct_grpc_exporters_with_existed_…
ThomsonTan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
36 changes: 36 additions & 0 deletions
36
exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_client_factory.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
#pragma once | ||
|
||
#include <memory> | ||
|
||
#include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h" | ||
#include "opentelemetry/nostd/shared_ptr.h" | ||
|
||
OPENTELEMETRY_BEGIN_NAMESPACE | ||
namespace exporter | ||
{ | ||
namespace otlp | ||
{ | ||
|
||
class OtlpGrpcClientReferenceGuard; | ||
class OtlpGrpcClient; | ||
|
||
/** | ||
* Factory class for OtlpGrpcClient. | ||
*/ | ||
class OPENTELEMETRY_EXPORT OtlpGrpcClientFactory | ||
{ | ||
public: | ||
/** | ||
* Create an OtlpGrpcClient using all default options. | ||
*/ | ||
static nostd::shared_ptr<OtlpGrpcClient> Create(const OtlpGrpcClientOptions &options); | ||
|
||
static nostd::shared_ptr<OtlpGrpcClientReferenceGuard> CreateReferenceGuard(); | ||
}; | ||
|
||
} // namespace otlp | ||
} // namespace exporter | ||
OPENTELEMETRY_END_NAMESPACE |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@owent It might be better not to enforce HAVE_ABSEIL at this point. Perhaps we can address it in a separate PR, allowing this one to focus solely on sharing the gRPC client. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a crash before, in which case some codes are compiled without
HAVE_ABSEIL
, but the codes in gRPC exporters must be compiled with it. I think it's the simplest way to keep compatibility to enforce abseil-cpp when using gRPC exporters now.I think we can remove it if we change the include paths of all bundled version of abseil-cpp, or there will be conflicts.( Some codes in gRPC exporters will be compiled with the bundled version of abseil-cpp because of the search order of headers, and the gRPC library is built with the external version)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the crash related to code within this PR. As there is no crash observed before that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR will use the new headers which also includes abseil-cpp's headers in some versions of gRPC.Without this macro, gRPC's headers may include the bundled version of abseil-cpp in otel-cpp.
The codes before did not use these new headers but there are still problems when otel-cpp's api use headers from the bundled version, but gRPC exporters use the external version. I think it's UB, and may lead to export nothing when using gRPC exporter.
In my understanding, the best solution is to change the file paths of bundled version of abseil, so the two versions will not share the same include path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this only apply to protobuf 3.21 and below? With protobuf 3.22, the SDK build will fail if
WITH_OTLP_GRPC
is enabled withoutWITH_ABSEIL
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not related to protobuf, gRpc always depends abseil-cpp. gRPC depends external abseil-cpp.
Maybe we can let all gRPC exporters always search headers from third party's include paths first so they will not use headers from bundled version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/open-telemetry/opentelemetry-cpp/actions/runs/11596474550/job/32287629537?pr=3041
The conflicts are show here, can I remove inline namespace for bundled absl to avoid conflicts? The ABI will not change and the abseil requirement for protobuf can also be removed when namespace conflict is resolved.
@lalitb @ThomsonTan