-
Notifications
You must be signed in to change notification settings - Fork 369
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
Add b3 metadata in grpc #2110
Add b3 metadata in grpc #2110
Conversation
why? this will allow us to extend the behavior of injection and extraction already present in HTTP.
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.
👋 @henrich-m, this PR is super flushed out, thank you! I left a minor comment.
If I can: could you add a new configuration to the gRPC integration: option :distributed_tracing, default: true
(at lib/datadog/tracing/contrib/grpc/configuration/settings.rb
)? This allows anyone to disable header propagation, in case gPRC is used strictly to contact 3rd parties: in that case, you might not want to expose internal headers to them. You can see most of other HTTP (e.g. Faraday, Ethon) integrations implement such option.
# B3 gRPC metadata used for distributed tracing | ||
B3_METADATA_TRACE_ID = 'x-b3-traceid'.freeze | ||
B3_METADATA_SPAN_ID = 'x-b3-spanid'.freeze | ||
B3_METADATA_SAMPLED = 'x-b3-sampled'.freeze | ||
B3_METADATA_SINGLE = 'b3'.freeze |
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 think it's ok to use
B3_HEADER_TRACE_ID = 'x-b3-traceid'.freeze
B3_HEADER_SPAN_ID = 'x-b3-spanid'.freeze
B3_HEADER_SAMPLED = 'x-b3-sampled'.freeze
B3_HEADER_SINGLE = 'b3'.freeze
directly, given these are mostly likely the standard header names for most transport protocols for B3.
I see the possibility of a different protocol where these tags have to be different, but I think HTTP and gRPC can share the existing constants we already had above.
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 was just following the same approach adopted for Datadod headers, I will refactor them :D
why? to disable the injection of metadata injection in the gRPC client calls.
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.
Thank you, @henrich-m! 🙇
👋 @henrich-m this change has been released in v1.2.0. |
What does this PR do?
It adds the support of B3 Metadata injection in gRPC calls.
Motivation
The B3 Standard is adopted in almost all observability frameworks, including dd-trace, it already does the injection and extraction in HTTP calls, but not in GRPC calls, we have 2 backends services that communicate with each other through gRPC and I can't see the traces correlated because one of them is written in Golang and utilizes Open Telemetry, that's why we need the B3 also in Metadata.
How to test the change?
Unit tests should cover the changes. :)