From 3b07702222e8c17abd461f96ffaf0dc56be88175 Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Fri, 21 Jun 2024 14:12:57 -0700 Subject: [PATCH] HTTP propagator: Better handling of invalid data --- lib/datadog/tracing/distributed/b3_multi.rb | 2 +- lib/datadog/tracing/distributed/b3_single.rb | 4 +++- lib/datadog/tracing/distributed/datadog.rb | 2 +- .../tracing/distributed/propagation.rb | 11 ++++++++-- .../tracing/distributed/trace_context.rb | 5 +++-- .../tracing/distributed/b3_multi_spec.rb | 8 ++++++++ .../tracing/distributed/b3_single_spec.rb | 12 +++++++++++ .../tracing/distributed/datadog_spec.rb | 13 ++++++++++++ .../tracing/distributed/propagation_spec.rb | 20 +++++++++++++++++++ .../tracing/distributed/trace_context_spec.rb | 13 ++++++++++++ 10 files changed, 83 insertions(+), 7 deletions(-) diff --git a/lib/datadog/tracing/distributed/b3_multi.rb b/lib/datadog/tracing/distributed/b3_multi.rb index 6122b16db54..2340dea0bd1 100644 --- a/lib/datadog/tracing/distributed/b3_multi.rb +++ b/lib/datadog/tracing/distributed/b3_multi.rb @@ -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( diff --git a/lib/datadog/tracing/distributed/b3_single.rb b/lib/datadog/tracing/distributed/b3_single.rb index 4bb7de203fd..45f7a386cfa 100644 --- a/lib/datadog/tracing/distributed/b3_single.rb +++ b/lib/datadog/tracing/distributed/b3_single.rb @@ -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( diff --git a/lib/datadog/tracing/distributed/datadog.rb b/lib/datadog/tracing/distributed/datadog.rb index d6c5fc4d671..b9e45cc70c4 100644 --- a/lib/datadog/tracing/distributed/datadog.rb +++ b/lib/datadog/tracing/distributed/datadog.rb @@ -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 diff --git a/lib/datadog/tracing/distributed/propagation.rb b/lib/datadog/tracing/distributed/propagation.rb index b18de48809a..2c56b441220 100644 --- a/lib/datadog/tracing/distributed/propagation.rb +++ b/lib/datadog/tracing/distributed/propagation.rb @@ -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. @@ -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.') @@ -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 diff --git a/lib/datadog/tracing/distributed/trace_context.rb b/lib/datadog/tracing/distributed/trace_context.rb index c5871f03175..b81107c629b 100644 --- a/lib/datadog/tracing/distributed/trace_context.rb +++ b/lib/datadog/tracing/distributed/trace_context.rb @@ -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 @@ -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 diff --git a/spec/datadog/tracing/distributed/b3_multi_spec.rb b/spec/datadog/tracing/distributed/b3_multi_spec.rb index bb9e6654c5b..0484064a4ad 100644 --- a/spec/datadog/tracing/distributed/b3_multi_spec.rb +++ b/spec/datadog/tracing/distributed/b3_multi_spec.rb @@ -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 diff --git a/spec/datadog/tracing/distributed/b3_single_spec.rb b/spec/datadog/tracing/distributed/b3_single_spec.rb index 49ca0c0a781..9f534bd58ef 100644 --- a/spec/datadog/tracing/distributed/b3_single_spec.rb +++ b/spec/datadog/tracing/distributed/b3_single_spec.rb @@ -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 diff --git a/spec/datadog/tracing/distributed/datadog_spec.rb b/spec/datadog/tracing/distributed/datadog_spec.rb index 1a5c7429402..cdb51684f99 100644 --- a/spec/datadog/tracing/distributed/datadog_spec.rb +++ b/spec/datadog/tracing/distributed/datadog_spec.rb @@ -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 diff --git a/spec/datadog/tracing/distributed/propagation_spec.rb b/spec/datadog/tracing/distributed/propagation_spec.rb index 68297248d9d..a07ed877325 100644 --- a/spec/datadog/tracing/distributed/propagation_spec.rb +++ b/spec/datadog/tracing/distributed/propagation_spec.rb @@ -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 } diff --git a/spec/datadog/tracing/distributed/trace_context_spec.rb b/spec/datadog/tracing/distributed/trace_context_spec.rb index 994492162c2..856b6a43628 100644 --- a/spec/datadog/tracing/distributed/trace_context_spec.rb +++ b/spec/datadog/tracing/distributed/trace_context_spec.rb @@ -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