Skip to content

Commit

Permalink
HTTP propagator: Better handling of invalid data
Browse files Browse the repository at this point in the history
  • Loading branch information
marcotc committed Jun 21, 2024
1 parent 4b19011 commit 3b07702
Show file tree
Hide file tree
Showing 10 changed files with 83 additions and 7 deletions.
2 changes: 1 addition & 1 deletion lib/datadog/tracing/distributed/b3_multi.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def inject!(digest, data = {})

# DEV: We need these to be hex encoded
data[@trace_id_key] = format('%032x', digest.trace_id)
data[@span_id_key] = format('%016x', digest.span_id)
data[@span_id_key] = format('%016x', digest.span_id || 0) # # Fall back to zero (invalid) if not present

if digest.trace_sampling_priority
sampling_priority = Helpers.clamp_sampling_priority(
Expand Down
4 changes: 3 additions & 1 deletion lib/datadog/tracing/distributed/b3_single.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ def initialize(fetcher:, key: B3_SINGLE_HEADER_KEY)
def inject!(digest, env)
return if digest.nil?

span_id = digest.span_id || 0 # Fall back to zero (invalid) if not present

# DEV: We need these to be hex encoded
value = "#{format('%032x', digest.trace_id)}-#{format('%016x', digest.span_id)}"
value = "#{format('%032x', digest.trace_id)}-#{format('%016x', span_id)}"

if digest.trace_sampling_priority
sampling_priority = Helpers.clamp_sampling_priority(
Expand Down
2 changes: 1 addition & 1 deletion lib/datadog/tracing/distributed/datadog.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def inject!(digest, data)

data[@trace_id_key] = Tracing::Utils::TraceId.to_low_order(digest.trace_id).to_s

data[@parent_id_key] = digest.span_id.to_s
data[@parent_id_key] = digest.span_id.to_s if digest.span_id
data[@sampling_priority_key] = digest.trace_sampling_priority.to_s if digest.trace_sampling_priority
data[@origin_key] = digest.trace_origin.to_s if digest.trace_origin

Expand Down
11 changes: 9 additions & 2 deletions lib/datadog/tracing/distributed/propagation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ def initialize(

# inject! populates the env with span ID, trace ID and sampling priority
#
# This method will never raise errors, but instead log them to `Datadog.logger`.
# This method will never raise errors.
# It can propagate partial data, if deemed useful, instead of failing.
# In case of unrecoverable errors, it will log them to `Datadog.logger`.
#
# DEV-2.0: inject! should work without arguments, injecting the active_trace's digest
# DEV-2.0: and returning a new Hash with the injected data.
Expand All @@ -45,7 +47,7 @@ def initialize(
# @param digest [TraceDigest]
# @param data [Hash]
# @return [Boolean] `true` if injected successfully, `false` if no propagation style is configured
# @return [nil] in case of error, see `Datadog.logger` output for details.
# @return [nil] in case of unrecoverable errors, see `Datadog.logger` output for details.
def inject!(digest, data)
if digest.nil?
::Datadog.logger.debug('Cannot inject distributed trace data: digest is nil.')
Expand All @@ -54,6 +56,11 @@ def inject!(digest, data)

digest = digest.to_digest if digest.respond_to?(:to_digest)

if digest.trace_id.nil?
::Datadog.logger.debug('Cannot inject distributed trace data: digest.trace_id is nil.')
return nil
end

result = false

# Inject all configured propagation styles
Expand Down
5 changes: 3 additions & 2 deletions lib/datadog/tracing/distributed/trace_context.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def delete_prefix(prefix)
def build_traceparent(digest)
build_traceparent_string(
digest.trace_id,
digest.span_id,
digest.span_id || 0, # Fall back to zero (invalid) if not present
build_trace_flags(digest)
)
end
Expand Down Expand Up @@ -198,7 +198,8 @@ def build_tracestate(digest)

def last_dd_parent_id(digest)
if !digest.span_remote
"p:#{format('%016x', digest.span_id)};"
span_id = digest.span_id || 0 # Fall back to zero (invalid) if not present
"p:#{format('%016x', span_id)};"
elsif digest.trace_distributed_tags&.key?(Tracing::Metadata::Ext::Distributed::TAG_DD_PARENT_ID)
"p:#{digest.trace_distributed_tags[Tracing::Metadata::Ext::Distributed::TAG_DD_PARENT_ID]};"
else
Expand Down
8 changes: 8 additions & 0 deletions spec/datadog/tracing/distributed/b3_multi_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,14 @@
)
end
end

context 'with span_id nil' do
let(:digest) { Datadog::Tracing::TraceDigest.new(trace_id: 0xdef) }

it 'sets x-b3-spanid to all zeros' do
is_expected.to include('x-b3-spanid' => '0000000000000000')
end
end
end

describe '#extract' do
Expand Down
12 changes: 12 additions & 0 deletions spec/datadog/tracing/distributed/b3_single_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,18 @@
expect(data).to eq(b3_single_header => 'aaaaaaaaaaaaaaaaffffffffffffffff-bbbbbbbbbbbbbbbb')
end
end

context 'with span_id nil' do
let(:digest) do
Datadog::Tracing::TraceDigest.new(
trace_id: 0xabcdef
)
end

it 'sets span component to all zeros' do
expect(data).to eq(b3_single_header => '00000000000000000000000000abcdef-0000000000000000')
end
end
end

describe '#extract' do
Expand Down
13 changes: 13 additions & 0 deletions spec/datadog/tracing/distributed/datadog_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,19 @@
)
end
end

context 'with span_id nil' do
let(:digest) do
Datadog::Tracing::TraceDigest.new(
trace_id: 30000
)
end

it 'does not include the x-datadog-parent-id field' do
inject!
expect(data).to eq('x-datadog-trace-id' => '30000')
end
end
end
end

Expand Down
20 changes: 20 additions & 0 deletions spec/datadog/tracing/distributed/propagation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,26 @@
expect(data).to include('x-datadog-parent-id' => '9876543210')
end

context 'when trace_id is nil' do
let(:trace_id) { nil }

before { skip('TraceOperation always has a trace_id') if trace.is_a?(Datadog::Tracing::TraceOperation) }

it 'does not inject the trace id' do
inject!
expect(data).to be_empty
end
end

context 'when span_id is nil' do
let(:span_id) { nil }

it 'includes an empty x-datadog-parent-id tag' do
inject!
expect(data).to include('x-datadog-parent-id' => '')
end
end

context 'when sampling priority is set' do
let(:sampling_priority) { 0 }

Expand Down
13 changes: 13 additions & 0 deletions spec/datadog/tracing/distributed/trace_context_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,19 @@
end
end
end

context 'with span_id nil' do
let(:digest) do
Datadog::Tracing::TraceDigest.new(
trace_id: 0xC0FFEE,
span_id: nil
)
end

it 'sets span component to all zeros' do
expect(traceparent).to eq('00-00000000000000000000000000c0ffee-0000000000000000-00')
end
end
end
end

Expand Down

0 comments on commit 3b07702

Please sign in to comment.