Skip to content

[core] Propagate synthetics origin headers#699

Merged
brettlangdon merged 9 commits into
0.20-devfrom
brettlangdon/origin
Feb 28, 2019
Merged

[core] Propagate synthetics origin headers#699
brettlangdon merged 9 commits into
0.20-devfrom
brettlangdon/origin

Conversation

@brettlangdon

Copy link
Copy Markdown
Member

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 as context.origin.
  • X-Datadog-Origin header will be appended to any distributed tracing request from a context with context.origin set.
  • _dd.origin tag will be set on the root trace of the context set to the value from context.origin.

@brettlangdon brettlangdon added the core Involves Datadog core libraries label Feb 25, 2019
@brettlangdon
brettlangdon requested a review from delner February 25, 2019 18:07
Comment thread test/context_test.rb Outdated
Comment thread test/propagation/distributed_headers_test.rb Outdated
def origin
hdr = @carrier[HTTP_HEADER_ORIGIN]
# Only return the value if it is not an empty string
hdr if hdr != ''

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member Author

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.

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

@delner delner Feb 27, 2019

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Member Author

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"

'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'

Copy link
Copy Markdown
Contributor

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.

Comment thread spec/ddtrace/integration_spec.rb Outdated
delner
delner previously approved these changes Feb 27, 2019

@delner delner left a comment

Copy link
Copy Markdown
Contributor

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! 🎉

@brettlangdon
brettlangdon changed the base branch from master to 0.20-dev February 28, 2019 14:54
@brettlangdon
brettlangdon merged commit 6481330 into 0.20-dev Feb 28, 2019
@brettlangdon
brettlangdon deleted the brettlangdon/origin branch February 28, 2019 15:16
@brettlangdon brettlangdon added this to the 0.20.0 milestone Mar 7, 2019
This was referenced Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Involves Datadog core libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants