-
Notifications
You must be signed in to change notification settings - Fork 373
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
[core] Propagate synthetics origin headers #699
Conversation
def origin | ||
hdr = @carrier[HTTP_HEADER_ORIGIN] | ||
# Only return the value if it is not an empty string | ||
hdr if hdr != '' |
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 nil
an acceptable output 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.
Yeah, I want nil
instead of ''
so I can tell whether we should set/propagate this header further.
@@ -18,6 +18,7 @@ def self.inject!(context, env) | |||
env[HTTP_HEADER_TRACE_ID] = context.trace_id.to_s | |||
env[HTTP_HEADER_PARENT_ID] = context.span_id.to_s | |||
env[HTTP_HEADER_SAMPLING_PRIORITY] = context.sampling_priority.to_s | |||
env[HTTP_HEADER_ORIGIN] = context.origin.to_s if context.origin |
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.
Not a big deal, but it's fine if you call to_s
on nil
, it will just give you a ''
string, as long as that's acceptable. Although when I look at this, the goal is to not set this header at all if the value is blank, so this is probably fine as is.
I might suggest we do the same for sampling priority line above, just as a minor bit of housekeeping and consistency.
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, using nil
as a means to say "do not propagate this header"
@@ -309,7 +309,8 @@ | |||
{ | |||
'HTTP_X_DATADOG_TRACE_ID' => '1', | |||
'HTTP_X_DATADOG_PARENT_ID' => '2', | |||
'HTTP_X_DATADOG_SAMPLING_PRIORITY' => Datadog::Ext::Priority::USER_KEEP.to_s | |||
'HTTP_X_DATADOG_SAMPLING_PRIORITY' => Datadog::Ext::Priority::USER_KEEP.to_s, | |||
'HTTP_X_DATADOG_ORIGIN' => 'synthetics' |
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.
Side note (not an action item for this PR), but we should move these distributed tracing examples, where possible, into some kind of shared examples, so we don't have to re-implement the tests for each integration.
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.
Overall looks good. Left a few minor suggestions, but they aren't blockers. Act at your discretion. Thanks @brettlangdon! 🎉
This PR adds support for extracting, injecting, and propagating
X-Datadog-Origin
headers as a part of distributed tracing.X-Datadog-Origin
header will be parsed and attached to the context ascontext.origin
.X-Datadog-Origin
header will be appended to any distributed tracing request from a context withcontext.origin
set._dd.origin
tag will be set on the root trace of the context set to the value fromcontext.origin
.