From 2944795b95b75e6f6d8b761da7f01a86a7cee34c Mon Sep 17 00:00:00 2001 From: Marco Costa Date: Tue, 7 Nov 2023 15:04:15 -0800 Subject: [PATCH] wip --- lib/datadog/tracing/configuration/ext.rb | 3 + lib/datadog/tracing/configuration/settings.rb | 11 +++ .../tracing/distributed/propagation.rb | 46 ++++++++- .../tracing/configuration/settings_spec.rb | 6 ++ .../tracing/distributed/propagation_spec.rb | 98 +++++++++++++------ 5 files changed, 128 insertions(+), 36 deletions(-) diff --git a/lib/datadog/tracing/configuration/ext.rb b/lib/datadog/tracing/configuration/ext.rb index 6435899df6f..3ff0b40c60f 100644 --- a/lib/datadog/tracing/configuration/ext.rb +++ b/lib/datadog/tracing/configuration/ext.rb @@ -62,6 +62,9 @@ module Distributed # @see https://opentelemetry.io/docs/concepts/sdk-configuration/general-sdk-configuration/#get_otel__propagators PROPAGATION_STYLE_NONE = 'none' + # Strictly stop at the first successfully serialized style. + EXTRACT_FIRST = 'DD_TRACE_PROPAGATION_EXTRACT_FIRST' + ENV_X_DATADOG_TAGS_MAX_LENGTH = 'DD_TRACE_X_DATADOG_TAGS_MAX_LENGTH' end diff --git a/lib/datadog/tracing/configuration/settings.rb b/lib/datadog/tracing/configuration/settings.rb index ddc3f93e861..602396be55e 100644 --- a/lib/datadog/tracing/configuration/settings.rb +++ b/lib/datadog/tracing/configuration/settings.rb @@ -144,6 +144,17 @@ def self.extended(base) set_option(:propagation_inject_style, styles) end end + + # Strictly stop at the first successfully serialized style. + # This prevents the tracer from enriching the extracted context with information from + # other valid propagations styles present in the request. + # @default `DD_TRACE_PROPAGATION_EXTRACT_FIRST` environment variable, otherwise `false`. + # @return [Boolean] + option :propagation_extract_first do |o| + o.env Tracing::Configuration::Ext::Distributed::EXTRACT_FIRST + o.default false + o.type :bool + end end # Enable trace collection and span generation. diff --git a/lib/datadog/tracing/distributed/propagation.rb b/lib/datadog/tracing/distributed/propagation.rb index fab3d9b804b..43617ddc668 100644 --- a/lib/datadog/tracing/distributed/propagation.rb +++ b/lib/datadog/tracing/distributed/propagation.rb @@ -83,19 +83,57 @@ def extract(data) extracted_trace_digest = nil - ::Datadog.configuration.tracing.distributed_tracing.propagation_extract_style.each do |style| + config = ::Datadog.configuration.tracing.distributed_tracing + + config.propagation_extract_style.each do |style| propagator = @propagation_styles[style] next if propagator.nil? begin - extracted_trace_digest = propagator.extract(data) + if extracted_trace_digest + # Return if we are only inspecting the first valid style. + next if config.propagation_extract_first + + # Continue parsing styles to find the W3C `tracestate` header, if present. + # `tracestate` must always be propagated, as it might contain pass-through data that we don't control. + # @see https://www.w3.org/TR/2021/REC-trace-context-1-20211123/#mutating-the-tracestate-field + next if style != Configuration::Ext::Distributed::PROPAGATION_STYLE_TRACE_CONTEXT + + if (tracecontext_digest = propagator.extract(data)) + # Only parse if it represent the same trace as the successfully extracted one + next unless tracecontext_digest.trace_id == extracted_trace_digest.trace_id + + # TODO: make this less worse + extracted_trace_digest = TraceDigest.new( + span_id: extracted_trace_digest.span_id, + span_name: extracted_trace_digest.span_name, + span_resource: extracted_trace_digest.span_resource, + span_service: extracted_trace_digest.span_service, + span_type: extracted_trace_digest.span_type, + trace_distributed_tags: extracted_trace_digest.trace_distributed_tags, + trace_hostname: extracted_trace_digest.trace_hostname, + trace_id: extracted_trace_digest.trace_id, + trace_name: extracted_trace_digest.trace_name, + trace_origin: extracted_trace_digest.trace_origin, + trace_process_id: extracted_trace_digest.trace_process_id, + trace_resource: extracted_trace_digest.trace_resource, + trace_runtime_id: extracted_trace_digest.trace_runtime_id, + trace_sampling_priority: extracted_trace_digest.trace_sampling_priority, + trace_service: extracted_trace_digest.trace_service, + trace_distributed_id: extracted_trace_digest.trace_distributed_id, + trace_flags: extracted_trace_digest.trace_flags, + trace_state: tracecontext_digest.trace_state, + trace_state_unknown_fields: tracecontext_digest.trace_state_unknown_fields, + ) + end + else + extracted_trace_digest = propagator.extract(data) + end rescue => e ::Datadog.logger.error( "Error extracting distributed trace data. Cause: #{e} Location: #{Array(e.backtrace).first}" ) end - - break if extracted_trace_digest end extracted_trace_digest diff --git a/spec/datadog/tracing/configuration/settings_spec.rb b/spec/datadog/tracing/configuration/settings_spec.rb index d016007bec7..d4e12cedca6 100644 --- a/spec/datadog/tracing/configuration/settings_spec.rb +++ b/spec/datadog/tracing/configuration/settings_spec.rb @@ -239,6 +239,12 @@ def propagation_inject_style end end end + + describe '#propagation_extract_first' do + it do + raise 'test this' + end + end end describe '#enabled' do diff --git a/spec/datadog/tracing/distributed/propagation_spec.rb b/spec/datadog/tracing/distributed/propagation_spec.rb index 731f455fef5..02010ace836 100644 --- a/spec/datadog/tracing/distributed/propagation_spec.rb +++ b/spec/datadog/tracing/distributed/propagation_spec.rb @@ -228,28 +228,12 @@ end context 'datadog, and tracecontext header' do - let(:data) do - { - prepare_key['x-datadog-trace-id'] => '61185', - prepare_key['x-datadog-parent-id'] => '73456', - prepare_key['traceparent'] => '00-11111111111111110000000000000001-000000003ade68b1-01', - } - end - - it do - expect(trace_digest).to be_a_kind_of(Datadog::Tracing::TraceDigest) - expect(trace_digest.span_id).to eq(73456) - expect(trace_digest.trace_id).to eq(61185) - expect(trace_digest.trace_sampling_priority).to be nil - end - - context 'and sampling priority' do + context 'with trace_id not matching' do let(:data) do { prepare_key['x-datadog-trace-id'] => '61185', prepare_key['x-datadog-parent-id'] => '73456', - prepare_key['x-datadog-sampling-priority'] => '1', - prepare_key['traceparent'] => '00-00000000000000000000000000c0ffee-0000000000000bee-00', + prepare_key['traceparent'] => '00-11111111111111110000000000000001-000000003ade68b1-01', } end @@ -257,27 +241,77 @@ expect(trace_digest).to be_a_kind_of(Datadog::Tracing::TraceDigest) expect(trace_digest.span_id).to eq(73456) expect(trace_digest.trace_id).to eq(61185) - expect(trace_digest.trace_sampling_priority).to eq(1) + expect(trace_digest.trace_sampling_priority).to be nil end - context 'with a failing propagator (Datadog)' do - let(:error) { StandardError.new('test_err').tap { |e| e.set_backtrace('caller:1') } } + context 'and sampling priority' do + let(:data) do + { + prepare_key['x-datadog-trace-id'] => '61185', + prepare_key['x-datadog-parent-id'] => '73456', + prepare_key['x-datadog-sampling-priority'] => '1', + prepare_key['traceparent'] => '00-00000000000000000000000000c0ffee-0000000000000bee-00', + } + end - before do - allow_any_instance_of(::Datadog::Tracing::Distributed::Datadog).to receive(:extract).and_raise(error) - allow(Datadog.logger).to receive(:error) + it do + expect(trace_digest).to be_a_kind_of(Datadog::Tracing::TraceDigest) + expect(trace_digest.span_id).to eq(73456) + expect(trace_digest.trace_id).to eq(61185) + expect(trace_digest.trace_sampling_priority).to eq(1) end - it 'does not propagate error to caller' do - trace_digest - expect(Datadog.logger).to have_received(:error).with(/Cause: test_err Location: caller:1/) + context 'with a failing propagator (Datadog)' do + let(:error) { StandardError.new('test_err').tap { |e| e.set_backtrace('caller:1') } } + + before do + allow_any_instance_of(::Datadog::Tracing::Distributed::Datadog).to receive(:extract).and_raise(error) + allow(Datadog.logger).to receive(:error) + end + + it 'does not propagate error to caller' do + trace_digest + expect(Datadog.logger).to have_received(:error).with(/Cause: test_err Location: caller:1/) + end + + it 'extracts values from non-failing propagator (tracecontext)' do + expect(trace_digest).to be_a_kind_of(Datadog::Tracing::TraceDigest) + expect(trace_digest.span_id).to eq(0xbee) + expect(trace_digest.trace_id).to eq(0xc0ffee) + expect(trace_digest.trace_sampling_priority).to eq(0) + end end + end - it 'extracts values from non-failing propagator (tracecontext)' do - expect(trace_digest).to be_a_kind_of(Datadog::Tracing::TraceDigest) - expect(trace_digest.span_id).to eq(0xbee) - expect(trace_digest.trace_id).to eq(0xc0ffee) - expect(trace_digest.trace_sampling_priority).to eq(0) + context 'and tracecontext' do + let(:data) { super().merge(prepare_key['tracestate'] => 'dd=unknown_field;,other=vendor') } + + it 'does not preserve tracestate' do + expect(trace_digest.trace_state).to be nil + expect(trace_digest.trace_state_unknown_fields).to be nil + end + end + end + + context 'with a matching trace_id' do + let(:data) do + { + prepare_key['x-datadog-trace-id'] => '61185', + prepare_key['x-datadog-parent-id'] => '73456', + prepare_key['traceparent'] => '00-0000000000000000000000000000ef01-0000000000011ef0-01', + } + end + + it 'does not parse tracecontext sampling priority' do + expect(trace_digest.trace_sampling_priority).to be nil + end + + context 'and tracecontext' do + let(:data) { super().merge(prepare_key['tracestate'] => 'dd=unknown_field;,other=vendor') } + + it 'preserves tracestate' do + expect(trace_digest.trace_state).to eq('other=vendor') + expect(trace_digest.trace_state_unknown_fields).to eq('unknown_field;') end end end