Skip to content
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

Merged
merged 9 commits into from
Feb 28, 2019
21 changes: 21 additions & 0 deletions lib/ddtrace/context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,18 @@ def sampling_priority=(priority)
end
end

def origin
@mutex.synchronize do
@origin
end
end

def origin=(origin)
@mutex.synchronize do
@origin = origin
end
end

# Return the last active span that corresponds to the last inserted
# item in the trace list. This cannot be considered as the current active
# span in asynchronous environments, because some spans can be closed
Expand Down Expand Up @@ -138,6 +150,7 @@ def get
sampled = @sampled

attach_sampling_priority if sampled && @sampling_priority
attach_origin if @origin

# still return sampled attribute, even if context is not finished
return nil, sampled unless check_finished_spans()
Expand All @@ -163,6 +176,7 @@ def reset(options = {})
@parent_span_id = options.fetch(:span_id, nil)
@sampled = options.fetch(:sampled, false)
@sampling_priority = options.fetch(:sampling_priority, nil)
@origin = options.fetch(:origin, nil)
@finished_spans = 0
@current_span = nil
@current_root_span = nil
Expand Down Expand Up @@ -192,6 +206,13 @@ def attach_sampling_priority
)
end

def attach_origin
@trace.first.set_tag(
Ext::DistributedTracing::ORIGIN_KEY,
@origin
)
end

# Return the start time of the root span, or nil if there are no spans or this is undefined.
def start_time
@mutex.synchronize do
Expand Down
3 changes: 3 additions & 0 deletions lib/ddtrace/ext/distributed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@ module DistributedTracing
HTTP_HEADER_PARENT_ID = 'x-datadog-parent-id'.freeze
HTTP_HEADER_SAMPLING_PRIORITY = 'x-datadog-sampling-priority'.freeze
SAMPLING_PRIORITY_KEY = '_sampling_priority_v1'.freeze
HTTP_HEADER_ORIGIN = 'x-datadog-origin'.freeze
ORIGIN_KEY = '_dd.origin'.freeze

# gRPC metadata keys for distributed tracing. https://github.com/grpc/grpc-go/blob/v1.10.x/Documentation/grpc-metadata.md
GRPC_METADATA_TRACE_ID = 'x-datadog-trace-id'.freeze
GRPC_METADATA_PARENT_ID = 'x-datadog-parent-id'.freeze
GRPC_METADATA_SAMPLING_PRIORITY = 'x-datadog-sampling-priority'.freeze
GRPC_METADATA_ORIGIN = 'x-datadog-origin'.freeze
end
end
end
6 changes: 6 additions & 0 deletions lib/ddtrace/opentracer/distributed_headers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ def sampling_priority
value
end

def origin
hdr = @carrier[HTTP_HEADER_ORIGIN]
# Only return the value if it is not an empty string
hdr if hdr != ''
Copy link
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
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.

end

private

def id(header)
Expand Down
4 changes: 3 additions & 1 deletion lib/ddtrace/opentracer/text_map_propagator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def inject(span_context, carrier)
carrier[HTTP_HEADER_TRACE_ID] = datadog_context.trace_id
carrier[HTTP_HEADER_PARENT_ID] = datadog_context.span_id
carrier[HTTP_HEADER_SAMPLING_PRIORITY] = datadog_context.sampling_priority
carrier[HTTP_HEADER_ORIGIN] = datadog_context.origin
end

# Inject baggage
Expand All @@ -43,7 +44,8 @@ def extract(carrier)
Datadog::Context.new(
trace_id: headers.trace_id,
span_id: headers.parent_id,
sampling_priority: headers.sampling_priority
sampling_priority: headers.sampling_priority,
origin: headers.origin
)
else
Datadog::Context.new
Expand Down
8 changes: 7 additions & 1 deletion lib/ddtrace/propagation/distributed_headers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def initialize(env)
end

def valid?
# Sampling priority is optional.
# Sampling priority and origin are optional.
trace_id && parent_id
end

Expand All @@ -33,6 +33,12 @@ def sampling_priority
value
end

def origin
hdr = header(HTTP_HEADER_ORIGIN)
# Only return the value if it is not an empty string
hdr if hdr != ''
end

private

def header(name)
Expand Down
9 changes: 8 additions & 1 deletion lib/ddtrace/propagation/grpc_propagator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ def self.inject!(context, metadata)
metadata[GRPC_METADATA_TRACE_ID] = context.trace_id.to_s
metadata[GRPC_METADATA_PARENT_ID] = context.span_id.to_s
metadata[GRPC_METADATA_SAMPLING_PRIORITY] = context.sampling_priority.to_s if context.sampling_priority
metadata[GRPC_METADATA_ORIGIN] = context.origin.to_s if context.origin
end

def self.extract(metadata)
metadata = Carrier.new(metadata)
return Datadog::Context.new unless metadata.valid?
Datadog::Context.new(trace_id: metadata.trace_id,
span_id: metadata.parent_id,
sampling_priority: metadata.sampling_priority)
sampling_priority: metadata.sampling_priority,
origin: metadata.origin)
end

# opentracing.io compliant carrier object
Expand Down Expand Up @@ -49,6 +51,11 @@ def sampling_priority
value = @metadata[GRPC_METADATA_SAMPLING_PRIORITY]
value && value.to_i
end

def origin
value = @metadata[GRPC_METADATA_ORIGIN]
value if value != ''
end
end
end
end
4 changes: 3 additions & 1 deletion lib/ddtrace/propagation/http_propagator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@delner delner Feb 27, 2019

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

env.delete(HTTP_HEADER_SAMPLING_PRIORITY) unless context.sampling_priority
end

Expand All @@ -28,7 +29,8 @@ def self.extract(env)
return Datadog::Context.new unless headers.valid?
Datadog::Context.new(trace_id: headers.trace_id,
span_id: headers.parent_id,
sampling_priority: headers.sampling_priority)
sampling_priority: headers.sampling_priority,
origin: headers.origin)
end
end
end
18 changes: 18 additions & 0 deletions spec/ddtrace/context_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,22 @@
end
end
end

describe '#origin' do
context 'with nil' do
before(:each) { context.origin = nil }
it { expect(context.origin).to be_nil }
end

context 'with empty string' do
# We do not do any filtering based on value
before(:each) { context.origin = '' }
it { expect(context.origin).to eq('') }
end

context 'with synthetics' do
before(:each) { context.origin = 'synthetics' }
it { expect(context.origin).to eq('synthetics') }
end
end
end
5 changes: 5 additions & 0 deletions spec/ddtrace/contrib/rack/distributed_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,21 @@
let(:trace_id) { 8694058539399423136 }
let(:parent_id) { 3605612475141592985 }
let(:sampling_priority) { Datadog::Ext::Priority::AUTO_KEEP }
let(:origin) { 'synthetics' }

before(:each) do
header Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID, trace_id
header Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID, parent_id
header Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY, sampling_priority
header Datadog::Ext::DistributedTracing::HTTP_HEADER_ORIGIN, origin
end
end

shared_context 'no distributed tracing headers' do
let(:trace_id) { nil }
let(:parent_id) { nil }
let(:sampling_priority) { nil }
let(:origin) { nil }
end

shared_examples_for 'a Rack request with distributed tracing' do
Expand All @@ -62,6 +65,7 @@
expect(span.trace_id).to eq(trace_id)
expect(span.parent_id).to eq(parent_id)
expect(span.get_metric(Datadog::Ext::DistributedTracing::SAMPLING_PRIORITY_KEY)).to eq(sampling_priority)
expect(span.get_tag(Datadog::Ext::DistributedTracing::ORIGIN_KEY)).to eq(origin)
end
end

Expand All @@ -73,6 +77,7 @@
expect(span.trace_id).to_not eq(trace_id)
expect(span.parent_id).to eq(0)
expect(span.get_metric(Datadog::Ext::DistributedTracing::SAMPLING_PRIORITY_KEY)).to be nil
expect(span.get_tag(Datadog::Ext::DistributedTracing::ORIGIN_KEY)).to be nil
end
end

Expand Down
4 changes: 3 additions & 1 deletion spec/ddtrace/contrib/sinatra/tracer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
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.

}
end

Expand All @@ -319,6 +320,7 @@
expect(span.trace_id).to eq(1)
expect(span.parent_id).to eq(2)
expect(span.get_metric(Datadog::Ext::DistributedTracing::SAMPLING_PRIORITY_KEY)).to eq(2.0)
expect(span.get_tag(Datadog::Ext::DistributedTracing::ORIGIN_KEY)).to eq('synthetics')
end
end
end
Expand Down
20 changes: 20 additions & 0 deletions spec/ddtrace/integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,26 @@ def agent_receives_span_step3(previous_success)
end
end

describe 'origin tag' do
# Sampling priority is enabled by default
let(:tracer) { get_test_tracer }

context 'when #sampling_priority is set on a parent span' do
subject(:tag_value) { parent_span.get_tag(Datadog::Ext::DistributedTracing::ORIGIN_KEY) }
let(:parent_span) { tracer.start_span('parent span') }

before(:each) do
parent_span.tap do
parent_span.context.origin = 'synthetics'
end.finish

try_wait_until { tracer.writer.spans(:keep).any? }
end

it { is_expected.to eq('synthetics') }
end
end

describe 'sampling priority integration' do
include_context 'agent-based test'

Expand Down
26 changes: 22 additions & 4 deletions spec/ddtrace/opentracer/propagation_integration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ def sampling_priority_metric(span)
span.get_metric(Datadog::OpenTracer::TextMapPropagator::SAMPLING_PRIORITY_KEY)
end

def origin_tag(span)
span.get_tag(Datadog::OpenTracer::TextMapPropagator::ORIGIN_KEY)
end

describe 'via OpenTracing::FORMAT_TEXT_MAP' do
def baggage_to_carrier_format(baggage)
baggage.map { |k, v| [Datadog::OpenTracer::TextMapPropagator::BAGGAGE_PREFIX + k, v] }.to_h
Expand All @@ -28,6 +32,7 @@ def baggage_to_carrier_format(baggage)
before(:each) do
tracer.start_active_span(span_name) do |scope|
scope.span.context.datadog_context.sampling_priority = 1
scope.span.context.datadog_context.origin = 'synthetics'
baggage.each { |k, v| scope.span.set_baggage_item(k, v) }
tracer.inject(
scope.span.context,
Expand All @@ -41,7 +46,8 @@ def baggage_to_carrier_format(baggage)
expect(carrier).to include(
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_TRACE_ID => a_kind_of(Integer),
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_PARENT_ID => a_kind_of(Integer),
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_SAMPLING_PRIORITY => a_kind_of(Integer)
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_SAMPLING_PRIORITY => a_kind_of(Integer),
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_ORIGIN => a_kind_of(String)
)

expect(carrier[Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_PARENT_ID]).to be > 0
Expand Down Expand Up @@ -71,13 +77,15 @@ def baggage_to_carrier_format(baggage)
super().merge(
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_TRACE_ID => trace_id.to_s,
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_PARENT_ID => parent_id.to_s,
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_SAMPLING_PRIORITY => sampling_priority.to_s
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_SAMPLING_PRIORITY => sampling_priority.to_s,
Datadog::OpenTracer::TextMapPropagator::HTTP_HEADER_ORIGIN => origin.to_s
)
end

let(:trace_id) { Datadog::Span::MAX_ID - 1 }
let(:parent_id) { Datadog::Span::MAX_ID - 2 }
let(:sampling_priority) { 2 }
let(:origin) { 'synthetics' }

let(:datadog_span) { datadog_spans.first }

Expand All @@ -87,6 +95,7 @@ def baggage_to_carrier_format(baggage)
it { expect(datadog_span.trace_id).to eq(trace_id) }
it { expect(datadog_span.parent_id).to eq(parent_id) }
it { expect(sampling_priority_metric(datadog_span)).to eq(sampling_priority) }
it { expect(origin_tag(datadog_span)).to eq(origin) }
it { expect(@scope.span.context.baggage).to include(baggage) }
end

Expand All @@ -111,6 +120,7 @@ def baggage_to_carrier_format(baggage)
before(:each) do
tracer.start_active_span(sender_span_name) do |sender_scope|
sender_scope.span.context.datadog_context.sampling_priority = 1
sender_scope.span.context.datadog_context.origin = 'synthetics'
baggage.each { |k, v| sender_scope.span.set_baggage_item(k, v) }
tracer.inject(
sender_scope.span.context,
Expand All @@ -134,11 +144,13 @@ def baggage_to_carrier_format(baggage)
it { expect(sender_datadog_span.finished?).to be(true) }
it { expect(sender_datadog_span.parent_id).to eq(0) }
it { expect(sampling_priority_metric(sender_datadog_span)).to eq(1) }
it { expect(origin_tag(sender_datadog_span)).to eq('synthetics') }
it { expect(receiver_datadog_span.name).to eq(receiver_span_name) }
it { expect(receiver_datadog_span.finished?).to be(true) }
it { expect(receiver_datadog_span.trace_id).to eq(sender_datadog_span.trace_id) }
it { expect(receiver_datadog_span.parent_id).to eq(sender_datadog_span.span_id) }
it { expect(sampling_priority_metric(receiver_datadog_span)).to eq(1) }
it { expect(origin_tag(receiver_datadog_span)).to eq('synthetics') }
it { expect(@receiver_scope.span.context.baggage).to include(baggage) }
end
end
Expand All @@ -160,6 +172,7 @@ def baggage_to_carrier_format(baggage)
before(:each) do
tracer.start_active_span(span_name) do |scope|
scope.span.context.datadog_context.sampling_priority = 1
scope.span.context.datadog_context.origin = 'synthetics'
baggage.each { |k, v| scope.span.set_baggage_item(k, v) }
tracer.inject(
scope.span.context,
Expand All @@ -173,7 +186,8 @@ def baggage_to_carrier_format(baggage)
expect(carrier).to include(
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_TRACE_ID => a_kind_of(String),
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_PARENT_ID => a_kind_of(String),
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_SAMPLING_PRIORITY => a_kind_of(String)
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_SAMPLING_PRIORITY => a_kind_of(String),
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_ORIGIN => a_kind_of(String)
)

expect(carrier[Datadog::OpenTracer::RackPropagator::HTTP_HEADER_PARENT_ID].to_i).to be > 0
Expand Down Expand Up @@ -204,14 +218,16 @@ def baggage_to_carrier_format(baggage)
carrier_to_rack_format(
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_TRACE_ID => trace_id.to_s,
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_PARENT_ID => parent_id.to_s,
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_SAMPLING_PRIORITY => sampling_priority.to_s
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_SAMPLING_PRIORITY => sampling_priority.to_s,
Datadog::OpenTracer::RackPropagator::HTTP_HEADER_ORIGIN => origin.to_s
)
)
end

let(:trace_id) { Datadog::Span::MAX_ID - 1 }
let(:parent_id) { Datadog::Span::MAX_ID - 2 }
let(:sampling_priority) { 2 }
let(:origin) { 'synthetics' }

let(:datadog_span) { datadog_spans.first }

Expand All @@ -221,6 +237,7 @@ def baggage_to_carrier_format(baggage)
it { expect(datadog_span.trace_id).to eq(trace_id) }
it { expect(datadog_span.parent_id).to eq(parent_id) }
it { expect(sampling_priority_metric(datadog_span)).to eq(sampling_priority) }
it { expect(origin_tag(datadog_span)).to eq('synthetics') }
it { expect(@scope.span.context.baggage).to include(baggage) }
end

Expand Down Expand Up @@ -249,6 +266,7 @@ def baggage_to_carrier_format(baggage)
before(:each) do
tracer.start_active_span(sender_span_name) do |sender_scope|
sender_scope.span.context.datadog_context.sampling_priority = 1
sender_scope.span.context.datadog_context.origin = 'synthetics'
baggage.each { |k, v| sender_scope.span.set_baggage_item(k, v) }

carrier = {}
Expand Down
Loading