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.
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
[EXPORTER] Allow to share gRPC clients between OTLP exporters #3041
Changes from 28 commits
213e3cf
5c5f9b7
37e5081
c9cb2df
0d409d4
cd80d19
ad722f4
f4521f3
38860d3
b5daf3f
8e9d300
51e0964
40ca747
b2aa191
3547800
ef62bc0
caa1743
949c679
e1e6627
74ceb64
d5bb715
c9419bf
d4b0286
72e4cb3
edd0c16
3ff6e4b
829294e
94260b3
4367e62
e2b23aa
5142036
d7266aa
603e2fa
adcf88e
b71c655
1145003
4d4d3a5
fe30625
7b2f29e
62756bd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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