-
Notifications
You must be signed in to change notification settings - Fork 377
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
chore(tracing): Adds trace_remote field to TraceDigest #3516
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.0 #3516 +/- ##
=======================================
Coverage 98.07% 98.07%
=======================================
Files 1237 1237
Lines 72254 72276 +22
Branches 3408 3408
=======================================
+ Hits 70863 70886 +23
+ Misses 1391 1390 -1 ☔ View full report in Codecov by Sentry. |
b308587
to
c59e423
Compare
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.
No docs review needed
@@ -299,6 +302,7 @@ def to_digest | |||
trace_service: service, | |||
trace_state: @trace_state, | |||
trace_state_unknown_fields: @trace_state_unknown_fields, | |||
trace_remote: (@remote_parent && @active_span.nil?), |
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.
Do we need @active_span.nil?
here?
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.
Yup. If we are creating a digest for an active span then trace_remote
should be False
. The digest represent a span that is local to the process.
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.
Minor changes requested.
What does this PR do?
Adds a mechanism to track whether a TraceDigest represents a remote span or if it is local to a host. This change is analogous to a change made in NodeJS: DataDog/dd-trace-js#4153.
is_remote
ortrace_remote
is an opentelemetry concept and is defined here: https://opentelemetry.io/docs/specs/otel/trace/api/#isremote.Motivation:
This new field will be used to propagate the last datadog parent id in w3c tracecontext headers. This change blocks: #3488
Additional Notes:
How to test the change?
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!