From a2ef4c66868bdcbacb10589526d010b63c66cb72 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Mon, 4 Apr 2022 12:26:33 +0200 Subject: [PATCH 01/21] Add setting a tag at the trace-level Since trace transport does not have a concept of trace, tags are set on the topmost span. When partial flushing is involved, the topmost span will be sent upon last flush, therefore tags will only appear in the UI when that final segment is processed. --- lib/datadog/tracing/trace_operation.rb | 25 +++++++++ spec/datadog/tracing/trace_operation_spec.rb | 58 ++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index b739002b59d..b9921fc330a 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -105,6 +105,10 @@ def finished? @finished == true end + def started? + !@root_span.nil? + end + def sampled? @sampled == true || (!@sampling_priority.nil? && @sampling_priority > 0) end @@ -318,6 +322,13 @@ def subscribe_deactivate_trace(&block) :events, :root_span + def set_tag(key, value = nil) + raise UnstartedError unless started? + raise FinishedError if finished? + + @root_span.set_tag(key, value) + end + def activate_span!(span_op) parent = @active_span @@ -407,6 +418,20 @@ def build_trace(spans, partial = false) root_span_id: !partial ? @root_span && @root_span.id : nil ) end + + # Error when an action is attempted that needs an active trace + class UnstartedError < StandardError + def message + 'Trace not started'.freeze + end + end + + # Error when an action is attempted that needs an active trace + class FinishedError < StandardError + def message + 'Trace finished'.freeze + end + end end # rubocop:enable Metrics/ClassLength end diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index ca94aa5df55..16d1454aa60 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -775,6 +775,64 @@ end end + describe '#set_tag' do + it 'needs a started trace' do + expect { trace_op.send(:set_tag, 'foo', 'bar') }.to raise_error(Datadog::Tracing::TraceOperation::UnstartedError) + end + + it 'needs an unfinished trace' do + trace_op.measure('top') {} + expect { trace_op.send(:set_tag, 'foo', 'bar') }.to raise_error(Datadog::Tracing::TraceOperation::FinishedError) + end + + it 'sets tag on trace from a measurement' do + trace_op.measure('top') do + trace_op.send(:set_tag, 'foo', 'bar') + end + + trace = trace_op.flush! + + expect(trace.spans.find { |span| span.name == 'top' }.meta).to include('foo' => 'bar') + end + + it 'sets tag on trace from a nested measurement' do + trace_op.measure('grandparent') do + trace_op.measure('parent') do + trace_op.send(:set_tag, 'foo', 'bar') + end + end + + trace = trace_op.flush! + + expect(trace.spans).to have(2).items + expect(trace.spans.map(&:name)).to include('parent') + expect(trace.spans.find { |span| span.name == 'grandparent' }.meta).to include('foo' => 'bar') + end + + context 'with partial flushing' do + subject(:flush!) { trace_op.flush! } + let(:trace) { flush! } + + it 'sets tag on trace from a nested measurement' do + trace_op.measure('grandparent') do + trace_op.measure('parent') do + trace_op.send(:set_tag, 'foo', 'bar') + end + flush! + end + + expect(trace.spans).to have(1).items + expect(trace.spans.map(&:name)).to include('parent') + expect(trace.spans.map(&:meta)).to_not include('foo' => 'bar') + + final_flush = trace_op.flush! + expect(final_flush.spans).to have(1).items + expect(final_flush.spans.map(&:name)).to include('grandparent') + expect(final_flush.spans.map(&:meta)).to include('foo' => 'bar') + end + end + end + describe '#build_span' do subject(:build_span) { trace_op.build_span(span_name, **span_options) } let(:span_name) { 'web.request' } From fafd2a1d8fe21fe70f13ba0837871e2506a4bdfb Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Mon, 4 Apr 2022 14:08:23 +0200 Subject: [PATCH 02/21] Use TraceFormatter instead of TraceOperation to set tags TraceOperation is largely oblivious to the root span concept. The lack of a concept of trace is part of the Trace API transport, therefore TraceFormatter is more appropriate to carry and decide where to put the trace-level values. Trace tags explicitly do not carry to partially flushed traces in order to avoid double processing. --- lib/datadog/tracing/trace_operation.rb | 41 +++++++--------- lib/datadog/tracing/trace_segment.rb | 5 +- lib/ddtrace/transport/trace_formatter.rb | 9 ++++ spec/datadog/tracing/trace_operation_spec.rb | 17 ++----- .../ddtrace/transport/trace_formatter_spec.rb | 48 ++++++++++++++++++- 5 files changed, 80 insertions(+), 40 deletions(-) diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index b9921fc330a..abe7e797e58 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -63,7 +63,8 @@ def initialize( sample_rate: nil, sampled: nil, sampling_priority: nil, - service: nil + service: nil, + tags: nil ) # Attributes @events = events || Events.new @@ -84,6 +85,9 @@ def initialize( @sampling_priority = sampling_priority @service = service + # Generic tags + @tags = set_tags(tags) + # State @root_span = nil @active_span = nil @@ -105,10 +109,6 @@ def finished? @finished == true end - def started? - !@root_span.nil? - end - def sampled? @sampled == true || (!@sampling_priority.nil? && @sampling_priority > 0) end @@ -243,7 +243,7 @@ def to_digest trace_resource: @resource, trace_runtime_id: Core::Environment::Identity.id, trace_sampling_priority: @sampling_priority, - trace_service: @service + trace_service: @service, ).freeze end @@ -265,7 +265,8 @@ def fork_clone sample_rate: @sample_rate, sampled: @sampled, sampling_priority: @sampling_priority, - service: (@service && @service.dup) + service: (@service && @service.dup), + tags: @tags.dup ) end @@ -322,11 +323,16 @@ def subscribe_deactivate_trace(&block) :events, :root_span + def tags + @tags ||= {} + end + def set_tag(key, value = nil) - raise UnstartedError unless started? - raise FinishedError if finished? + tags[key] = value + end - @root_span.set_tag(key, value) + def set_tags(tags) + (tags || {}).each { |k, v| set_tag(k, v) } end def activate_span!(span_op) @@ -415,23 +421,10 @@ def build_trace(spans, partial = false) name: @name, resource: @resource, service: @service, + tags: !partial ? @tags : nil, root_span_id: !partial ? @root_span && @root_span.id : nil ) end - - # Error when an action is attempted that needs an active trace - class UnstartedError < StandardError - def message - 'Trace not started'.freeze - end - end - - # Error when an action is attempted that needs an active trace - class FinishedError < StandardError - def message - 'Trace finished'.freeze - end - end end # rubocop:enable Metrics/ClassLength end diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index 46f2508fc11..4efd10762fe 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -49,12 +49,13 @@ def initialize( runtime_id: nil, sample_rate: nil, sampling_priority: nil, - service: nil + service: nil, + tags: nil ) @id = id @root_span_id = root_span_id @spans = spans || [] - @tags = {} + @tags = tags || {} # Set well-known tags self.agent_sample_rate = agent_sample_rate diff --git a/lib/ddtrace/transport/trace_formatter.rb b/lib/ddtrace/transport/trace_formatter.rb index 567638817ff..f8e06a905ac 100644 --- a/lib/ddtrace/transport/trace_formatter.rb +++ b/lib/ddtrace/transport/trace_formatter.rb @@ -32,6 +32,11 @@ def format! # trace metadata, we must put our trace # metadata on the root span. This "root span" # is needed by the agent/API to ingest the trace. + + # Apply generic trace tags. Any more specific value will be overridden + # by the subsequent calls below. + set_trace_tags! + set_resource! tag_agent_sample_rate! @@ -59,6 +64,10 @@ def set_resource! root_span.resource = trace.resource end + def set_trace_tags! + root_span.set_tags(trace.tags) + end + def tag_agent_sample_rate! return unless trace.agent_sample_rate diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index 16d1454aa60..d16fc1ff0c6 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -776,15 +776,6 @@ end describe '#set_tag' do - it 'needs a started trace' do - expect { trace_op.send(:set_tag, 'foo', 'bar') }.to raise_error(Datadog::Tracing::TraceOperation::UnstartedError) - end - - it 'needs an unfinished trace' do - trace_op.measure('top') {} - expect { trace_op.send(:set_tag, 'foo', 'bar') }.to raise_error(Datadog::Tracing::TraceOperation::FinishedError) - end - it 'sets tag on trace from a measurement' do trace_op.measure('top') do trace_op.send(:set_tag, 'foo', 'bar') @@ -792,7 +783,7 @@ trace = trace_op.flush! - expect(trace.spans.find { |span| span.name == 'top' }.meta).to include('foo' => 'bar') + expect(trace.tags).to include('foo' => 'bar') end it 'sets tag on trace from a nested measurement' do @@ -806,7 +797,7 @@ expect(trace.spans).to have(2).items expect(trace.spans.map(&:name)).to include('parent') - expect(trace.spans.find { |span| span.name == 'grandparent' }.meta).to include('foo' => 'bar') + expect(trace.tags).to include('foo' => 'bar') end context 'with partial flushing' do @@ -823,12 +814,12 @@ expect(trace.spans).to have(1).items expect(trace.spans.map(&:name)).to include('parent') - expect(trace.spans.map(&:meta)).to_not include('foo' => 'bar') + expect(trace.tags).to_not include('foo' => 'bar') final_flush = trace_op.flush! expect(final_flush.spans).to have(1).items expect(final_flush.spans.map(&:name)).to include('grandparent') - expect(final_flush.spans.map(&:meta)).to include('foo' => 'bar') + expect(final_flush.tags).to include('foo' => 'bar') end end end diff --git a/spec/ddtrace/transport/trace_formatter_spec.rb b/spec/ddtrace/transport/trace_formatter_spec.rb index 50f4b1cb309..b28448cf4d5 100644 --- a/spec/ddtrace/transport/trace_formatter_spec.rb +++ b/spec/ddtrace/transport/trace_formatter_spec.rb @@ -16,6 +16,10 @@ let(:trace_options) { {} } shared_context 'trace metadata' do + let(:trace_tags) do + nil + end + let(:trace_options) do { resource: resource, @@ -28,7 +32,8 @@ rule_sample_rate: rule_sample_rate, runtime_id: runtime_id, sample_rate: sample_rate, - sampling_priority: sampling_priority + sampling_priority: sampling_priority, + tags: trace_tags } end @@ -45,6 +50,17 @@ let(:sampling_priority) { Datadog::Tracing::Sampling::Ext::Priority::USER_KEEP } end + shared_context 'trace metadata with tags' do + include_context 'trace metadata' + + let(:trace_tags) do + { + 'foo' => 'bar', + 'baz' => 42 + } + end + end + shared_context 'no root span' do let(:trace) { Datadog::Tracing::TraceSegment.new(spans, **trace_options) } let(:spans) { Array.new(3) { Datadog::Tracing::Span.new('my.job') } } @@ -139,6 +155,16 @@ end end + shared_examples 'root span with generic tags' do + context 'metrics' do + it { expect(root_span.metrics).to include({ 'baz' => 42 }) } + end + + context 'meta' do + it { expect(root_span.meta).to include({ 'foo' => 'bar' }) } + end + end + context 'with no root span' do include_context 'no root span' @@ -159,6 +185,16 @@ it_behaves_like 'root span with tags' end + + context 'when trace has metadata set with generic tags' do + include_context 'trace metadata with tags' + + it { is_expected.to be(trace) } + it { expect(root_span.resource).to eq('my.job') } + + it_behaves_like 'root span with tags' + it_behaves_like 'root span with generic tags' + end end context 'with missing root span' do @@ -181,6 +217,16 @@ it_behaves_like 'root span with tags' end + + context 'when trace has metadata set with generic tags' do + include_context 'trace metadata with tags' + + it { is_expected.to be(trace) } + it { expect(root_span.resource).to eq('my.job') } + + it_behaves_like 'root span with tags' + it_behaves_like 'root span with generic tags' + end end context 'with a root span' do From 700b398e65fe707cf3039039f07691d4bf1a8aa8 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Mon, 4 Apr 2022 16:12:30 +0200 Subject: [PATCH 03/21] Remove references to root span from AppSec --- .../appsec/contrib/rack/gateway/watcher.rb | 4 --- lib/datadog/appsec/event.rb | 27 +++++++------------ 2 files changed, 10 insertions(+), 21 deletions(-) diff --git a/lib/datadog/appsec/contrib/rack/gateway/watcher.rb b/lib/datadog/appsec/contrib/rack/gateway/watcher.rb index 87c2e7da2ff..603f99793ef 100644 --- a/lib/datadog/appsec/contrib/rack/gateway/watcher.rb +++ b/lib/datadog/appsec/contrib/rack/gateway/watcher.rb @@ -27,7 +27,6 @@ def self.watch # TODO: factor out if defined?(Datadog::Tracing) && Datadog::Tracing.respond_to?(:active_span) active_trace = Datadog::Tracing.active_trace - root_span = active_trace.send(:root_span) if active_trace active_span = Datadog::Tracing.active_span Datadog.logger.debug { "active span: #{active_span.span_id}" } if active_span @@ -45,7 +44,6 @@ def self.watch event = { waf_result: result, trace: active_trace, - root_span: root_span, span: active_span, request: request, action: action @@ -77,7 +75,6 @@ def self.watch # TODO: factor out if defined?(Datadog::Tracing) && Datadog::Tracing.respond_to?(:active_span) active_trace = Datadog::Tracing.active_trace - root_span = active_trace.send(:root_span) if active_trace active_span = Datadog::Tracing.active_span Datadog.logger.debug { "active span: #{active_span.span_id}" } if active_span @@ -95,7 +92,6 @@ def self.watch event = { waf_result: result, trace: active_trace, - root_span: root_span, span: active_span, response: response, action: action diff --git a/lib/datadog/appsec/event.rb b/lib/datadog/appsec/event.rb index 02b5895757f..025dd28f1a0 100644 --- a/lib/datadog/appsec/event.rb +++ b/lib/datadog/appsec/event.rb @@ -43,26 +43,20 @@ def self.record(*events) end # rubocop:disable Metrics/AbcSize - # rubocop:disable Metrics/MethodLength def self.record_via_span(*events) - events.group_by { |e| e[:root_span] }.each do |root_span, event_group| - unless root_span - Datadog.logger.debug { "{ error: 'no root span: cannot record', event_group: #{event_group.inspect}}" } + events.group_by { |e| e[:trace] }.each do |trace, event_group| + unless trace + Datadog.logger.debug { "{ error: 'no trace: cannot record', event_group: #{event_group.inspect}}" } next end - # TODO: this is a hack but there is no API to do that - root_span_tags = root_span.send(:meta).keys + trace.keep! # prepare and gather tags to apply - root_tags = event_group.each_with_object({}) do |event, tags| + trace_tags = event_group.each_with_object({}) do |event, tags| span = event[:span] - trace = event[:trace] - if span - span.set_tag('appsec.event', 'true') - trace.keep! - end + span.set_tag('appsec.event', 'true') if span request = event[:request] response = event[:response] @@ -98,16 +92,15 @@ def self.record_via_span(*events) # apply tags to root span # complex types are unsupported, we need to serialize to a string - triggers = root_tags.delete('_dd.appsec.triggers') - root_span.set_tag('_dd.appsec.json', JSON.dump({ triggers: triggers })) + triggers = trace_tags.delete('_dd.appsec.triggers') + trace.send(:set_tag, '_dd.appsec.json', JSON.dump({ triggers: triggers })) - root_tags.each do |key, value| - root_span.set_tag(key, value.is_a?(String) ? value.encode('UTf-8') : value) unless root_span_tags.include?(key) + trace_tags.each do |key, value| + trace.send(:set_tag, key, value.is_a?(String) ? value.encode('UTF-8') : value) end end end # rubocop:enable Metrics/AbcSize - # rubocop:enable Metrics/MethodLength end end end From 958f95e00db27778e2be0bf78c6aa12044a2c3b7 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Wed, 6 Apr 2022 13:34:05 +0200 Subject: [PATCH 04/21] Add missing spec cases --- spec/datadog/tracing/trace_operation_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index d16fc1ff0c6..6f97bb7f838 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -776,6 +776,24 @@ end describe '#set_tag' do + it 'sets tag on trace before a measurement' do + trace_op.send(:set_tag, 'foo', 'bar') + trace_op.measure('top') {} + + trace = trace_op.flush! + + expect(trace.tags).to include('foo' => 'bar') + end + + it 'sets tag on trace after a measurement' do + trace_op.measure('top') {} + trace_op.send(:set_tag, 'foo', 'bar') + + trace = trace_op.flush! + + expect(trace.tags).to include('foo' => 'bar') + end + it 'sets tag on trace from a measurement' do trace_op.measure('top') do trace_op.send(:set_tag, 'foo', 'bar') From aeaceeff522a88e2196fbf639f5ffb4aec50ab2f Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Wed, 6 Apr 2022 13:34:26 +0200 Subject: [PATCH 05/21] Add #get_tag --- lib/datadog/tracing/trace_operation.rb | 4 ++++ spec/datadog/tracing/trace_operation_spec.rb | 14 ++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index abe7e797e58..1976fa7c1b2 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -327,6 +327,10 @@ def tags @tags ||= {} end + def get_tag(key) + tags[key] + end + def set_tag(key, value = nil) tags[key] = value end diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index 6f97bb7f838..3ceceedfc50 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -775,6 +775,20 @@ end end + describe '#get_tag' do + before do + trace_op.send(:set_tag, 'foo', 'bar') + end + + it 'gets tag set on trace' do + expect(trace_op.send(:get_tag, 'foo')).to eq('bar') + end + + it 'gets unset tag as nil' do + expect(trace_op.send(:get_tag, 'unset')).to be_nil + end + end + describe '#set_tag' do it 'sets tag on trace before a measurement' do trace_op.send(:set_tag, 'foo', 'bar') From 90614bb9cc54b0a2daebcea7840d852b572c36ce Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Mon, 11 Apr 2022 18:09:45 +0200 Subject: [PATCH 06/21] Leverage Metadata::Analytics on TraceOperation --- lib/datadog/tracing/trace_operation.rb | 23 ++------- spec/datadog/tracing/trace_operation_spec.rb | 49 ++++++++++++++++++++ 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index 1976fa7c1b2..de7ae13a026 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -9,6 +9,7 @@ require 'datadog/tracing/span_operation' require 'datadog/tracing/trace_segment' require 'datadog/tracing/trace_digest' +require 'datadog/tracing/metadata' module Datadog module Tracing @@ -24,6 +25,8 @@ module Tracing # rubocop:disable Metrics/ClassLength # @public_api class TraceOperation + include Metadata + DEFAULT_MAX_LENGTH = 100_000 attr_accessor \ @@ -86,7 +89,7 @@ def initialize( @service = service # Generic tags - @tags = set_tags(tags) + set_tags(tags) if tags # State @root_span = nil @@ -323,22 +326,6 @@ def subscribe_deactivate_trace(&block) :events, :root_span - def tags - @tags ||= {} - end - - def get_tag(key) - tags[key] - end - - def set_tag(key, value = nil) - tags[key] = value - end - - def set_tags(tags) - (tags || {}).each { |k, v| set_tag(k, v) } - end - def activate_span!(span_op) parent = @active_span @@ -425,7 +412,7 @@ def build_trace(spans, partial = false) name: @name, resource: @resource, service: @service, - tags: !partial ? @tags : nil, + tags: !partial ? meta.merge(metrics) : nil, root_span_id: !partial ? @root_span && @root_span.id : nil ) end diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index 3ceceedfc50..62ee522545b 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -789,6 +789,28 @@ end end + describe '#set_metric' do + it 'sets metrics' do + trace_op.send(:set_metric, 'foo', 42) + trace_op.measure('top') {} + + trace = trace_op.flush! + + expect(trace.tags).to include('foo' => 42) + end + end + + describe '#set_error' do + it 'sets errors' do + trace_op.send(:set_error, Exception.new('foo')) + trace_op.measure('top') {} + + trace = trace_op.flush! + + expect(trace.tags).to include('error.msg' => 'foo', 'error.type' => 'Exception') + end + end + describe '#set_tag' do it 'sets tag on trace before a measurement' do trace_op.send(:set_tag, 'foo', 'bar') @@ -832,6 +854,33 @@ expect(trace.tags).to include('foo' => 'bar') end + it 'sets metrics' do + trace_op.send(:set_tag, 'foo', 42) + trace_op.measure('top') {} + + trace = trace_op.flush! + + expect(trace.tags).to include('foo' => 42) + end + + it 'sets analytics enabled' do + trace_op.send(:set_tag, Datadog::Tracing::Metadata::Ext::Analytics::TAG_ENABLED, true) + trace_op.measure('top') {} + + trace = trace_op.flush! + + expect(trace.tags).to include(Datadog::Tracing::Metadata::Ext::Analytics::TAG_SAMPLE_RATE => 1.0) + end + + it 'sets analytics sample rate' do + trace_op.send(:set_tag, Datadog::Tracing::Metadata::Ext::Analytics::TAG_SAMPLE_RATE, 42) + trace_op.measure('top') {} + + trace = trace_op.flush! + + expect(trace.tags).to include(Datadog::Tracing::Metadata::Ext::Analytics::TAG_SAMPLE_RATE => 42.0) + end + context 'with partial flushing' do subject(:flush!) { trace_op.flush! } let(:trace) { flush! } From 8ea77203c13795dc32440f105686d66c6697d56f Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Tue, 12 Apr 2022 18:07:01 +0200 Subject: [PATCH 07/21] Move partial trace tagging condition to TraceFormatter TraceOperation passes its tags unconditionally, therefore TraceSegment now carries trace tags all the way to TraceFormatter, which has the responsibility to decide whether to apply them or not to one of the spans. --- lib/datadog/tracing/trace_operation.rb | 2 +- lib/ddtrace/transport/trace_formatter.rb | 7 ++++++ spec/datadog/tracing/trace_operation_spec.rb | 2 +- .../ddtrace/transport/trace_formatter_spec.rb | 24 +++++++++++++++++-- 4 files changed, 31 insertions(+), 4 deletions(-) diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index de7ae13a026..e7db012bb11 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -412,7 +412,7 @@ def build_trace(spans, partial = false) name: @name, resource: @resource, service: @service, - tags: !partial ? meta.merge(metrics) : nil, + tags: meta.merge(metrics), root_span_id: !partial ? @root_span && @root_span.id : nil ) end diff --git a/lib/ddtrace/transport/trace_formatter.rb b/lib/ddtrace/transport/trace_formatter.rb index f8e06a905ac..55b06a77c85 100644 --- a/lib/ddtrace/transport/trace_formatter.rb +++ b/lib/ddtrace/transport/trace_formatter.rb @@ -65,6 +65,11 @@ def set_resource! end def set_trace_tags! + # If the root span wasn't specified, don't set this. We don't want to + # misset or overwrite the tags of a span that is in the middle of the + # trace. + return if !@found_root_span + root_span.set_tags(trace.tags) end @@ -168,6 +173,8 @@ def find_root_span(trace) root_span_id = trace.send(:root_span_id) root_span = trace.spans.find { |s| s.id == root_span_id } if root_span_id @found_root_span = !root_span.nil? + + # when root span is not found, fall back to last span (partial flush) root_span || trace.spans.last end end diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index 62ee522545b..3c31c95d2ff 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -895,7 +895,7 @@ expect(trace.spans).to have(1).items expect(trace.spans.map(&:name)).to include('parent') - expect(trace.tags).to_not include('foo' => 'bar') + expect(trace.tags).to include('foo' => 'bar') final_flush = trace_op.flush! expect(final_flush.spans).to have(1).items diff --git a/spec/ddtrace/transport/trace_formatter_spec.rb b/spec/ddtrace/transport/trace_formatter_spec.rb index b28448cf4d5..5bf629ec1f0 100644 --- a/spec/ddtrace/transport/trace_formatter_spec.rb +++ b/spec/ddtrace/transport/trace_formatter_spec.rb @@ -165,6 +165,16 @@ end end + shared_examples 'root span without generic tags' do + context 'metrics' do + it { expect(root_span.metrics).to_not include({ 'baz' => 42 }) } + end + + context 'meta' do + it { expect(root_span.meta).to_not include({ 'foo' => 'bar' }) } + end + end + context 'with no root span' do include_context 'no root span' @@ -193,7 +203,7 @@ it { expect(root_span.resource).to eq('my.job') } it_behaves_like 'root span with tags' - it_behaves_like 'root span with generic tags' + it_behaves_like 'root span without generic tags' end end @@ -225,7 +235,7 @@ it { expect(root_span.resource).to eq('my.job') } it_behaves_like 'root span with tags' - it_behaves_like 'root span with generic tags' + it_behaves_like 'root span without generic tags' end end @@ -249,6 +259,16 @@ it_behaves_like 'root span with tags' end + + context 'when trace has metadata set with generic tags' do + include_context 'trace metadata with tags' + + it { is_expected.to be(trace) } + it { expect(root_span.resource).to eq(resource) } + + it_behaves_like 'root span with tags' + it_behaves_like 'root span with generic tags' + end end end end From 24a294dfe3fa97aed29e7502c84d3344e8eb4593 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Tue, 12 Apr 2022 21:58:41 +0200 Subject: [PATCH 08/21] Split set_error out of Metadata::Tagging --- lib/datadog/tracing/metadata.rb | 2 ++ lib/datadog/tracing/metadata/errors.rb | 24 +++++++++++++ lib/datadog/tracing/metadata/tagging.rb | 10 ------ spec/datadog/tracing/metadata/errors_spec.rb | 35 +++++++++++++++++++ spec/datadog/tracing/metadata/tagging_spec.rb | 19 ---------- spec/datadog/tracing/metadata_spec.rb | 5 ++- 6 files changed, 63 insertions(+), 32 deletions(-) create mode 100644 lib/datadog/tracing/metadata/errors.rb create mode 100644 spec/datadog/tracing/metadata/errors_spec.rb diff --git a/lib/datadog/tracing/metadata.rb b/lib/datadog/tracing/metadata.rb index bf57c110ae0..3ed206de476 100644 --- a/lib/datadog/tracing/metadata.rb +++ b/lib/datadog/tracing/metadata.rb @@ -2,6 +2,7 @@ require 'datadog/tracing/metadata/analytics' require 'datadog/tracing/metadata/tagging' +require 'datadog/tracing/metadata/errors' module Datadog module Tracing @@ -9,6 +10,7 @@ module Tracing module Metadata def self.included(base) base.include(Metadata::Tagging) + base.include(Metadata::Errors) # Additional extensions base.prepend(Metadata::Analytics) diff --git a/lib/datadog/tracing/metadata/errors.rb b/lib/datadog/tracing/metadata/errors.rb new file mode 100644 index 00000000000..a24e1ca634d --- /dev/null +++ b/lib/datadog/tracing/metadata/errors.rb @@ -0,0 +1,24 @@ +# typed: false + +require 'datadog/core/error' + +require 'datadog/tracing/metadata/ext' + +module Datadog + module Tracing + module Metadata + # Adds error tagging behavior + # @public_api + module Errors + # Mark the span with the given error. + def set_error(e) + e = Core::Error.build_from(e) + + set_tag(Ext::Errors::TAG_TYPE, e.type) unless e.type.empty? + set_tag(Ext::Errors::TAG_MSG, e.message) unless e.message.empty? + set_tag(Ext::Errors::TAG_STACK, e.backtrace) unless e.backtrace.empty? + end + end + end + end +end diff --git a/lib/datadog/tracing/metadata/tagging.rb b/lib/datadog/tracing/metadata/tagging.rb index 5d21308cfee..12a66555cef 100644 --- a/lib/datadog/tracing/metadata/tagging.rb +++ b/lib/datadog/tracing/metadata/tagging.rb @@ -1,6 +1,5 @@ # typed: false -require 'datadog/core/error' require 'datadog/core/environment/ext' require 'datadog/tracing/metadata/ext' @@ -96,15 +95,6 @@ def clear_metric(key) metrics.delete(key) end - # Mark the span with the given error. - def set_error(e) - e = Core::Error.build_from(e) - - set_tag(Ext::Errors::TAG_TYPE, e.type) unless e.type.empty? - set_tag(Ext::Errors::TAG_MSG, e.message) unless e.message.empty? - set_tag(Ext::Errors::TAG_STACK, e.backtrace) unless e.backtrace.empty? - end - protected def meta diff --git a/spec/datadog/tracing/metadata/errors_spec.rb b/spec/datadog/tracing/metadata/errors_spec.rb new file mode 100644 index 00000000000..e312644a7eb --- /dev/null +++ b/spec/datadog/tracing/metadata/errors_spec.rb @@ -0,0 +1,35 @@ +# typed: ignore + +require 'spec_helper' + +require 'datadog/tracing/metadata/tagging' +require 'datadog/tracing/metadata/errors' + +RSpec.describe Datadog::Tracing::Metadata::Errors do + subject(:test_object) { test_class.new } + let(:test_class) do + Class.new do + include Datadog::Tracing::Metadata::Tagging + include Datadog::Tracing::Metadata::Errors + end + end + + describe '#set_error' do + subject(:set_error) { test_object.set_error(error) } + + let(:error) { RuntimeError.new('oops') } + let(:backtrace) { %w[method1 method2 method3] } + + before { error.set_backtrace(backtrace) } + + it do + set_error + + expect(test_object).to have_error_message('oops') + expect(test_object).to have_error_type('RuntimeError') + backtrace.each do |method| + expect(test_object).to have_error_stack(include(method)) + end + end + end +end diff --git a/spec/datadog/tracing/metadata/tagging_spec.rb b/spec/datadog/tracing/metadata/tagging_spec.rb index bf78b6ed323..30990ab735e 100644 --- a/spec/datadog/tracing/metadata/tagging_spec.rb +++ b/spec/datadog/tracing/metadata/tagging_spec.rb @@ -352,23 +352,4 @@ expect(test_object.send(:metrics)).to_not have_key(key) end end - - describe '#set_error' do - subject(:set_error) { test_object.set_error(error) } - - let(:error) { RuntimeError.new('oops') } - let(:backtrace) { %w[method1 method2 method3] } - - before { error.set_backtrace(backtrace) } - - it do - set_error - - expect(test_object).to have_error_message('oops') - expect(test_object).to have_error_type('RuntimeError') - backtrace.each do |method| - expect(test_object).to have_error_stack(include(method)) - end - end - end end diff --git a/spec/datadog/tracing/metadata_spec.rb b/spec/datadog/tracing/metadata_spec.rb index 33d59a96690..4a30cce1beb 100644 --- a/spec/datadog/tracing/metadata_spec.rb +++ b/spec/datadog/tracing/metadata_spec.rb @@ -3,8 +3,6 @@ require 'spec_helper' require 'datadog/tracing/metadata' -require 'datadog/tracing/metadata/analytics' -require 'datadog/tracing/metadata/tagging' RSpec.describe Datadog::Tracing::Metadata do context 'when included' do @@ -16,7 +14,8 @@ it 'has all of the tagging behavior in correct order' do expect(ancestors.first(5)).to include( described_class::Analytics, - described_class::Tagging + described_class::Tagging, + described_class::Errors ) end end From 643f0f0b38d9fa4b1fce599d73caf2df38a954e3 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Tue, 12 Apr 2022 22:01:47 +0200 Subject: [PATCH 09/21] Add only tagging behaviour to TraceOperation --- lib/datadog/tracing/trace_operation.rb | 4 +-- spec/datadog/tracing/trace_operation_spec.rb | 29 -------------------- 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index e7db012bb11..53fdc603bc0 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -9,7 +9,7 @@ require 'datadog/tracing/span_operation' require 'datadog/tracing/trace_segment' require 'datadog/tracing/trace_digest' -require 'datadog/tracing/metadata' +require 'datadog/tracing/metadata/tagging' module Datadog module Tracing @@ -25,7 +25,7 @@ module Tracing # rubocop:disable Metrics/ClassLength # @public_api class TraceOperation - include Metadata + include Metadata::Tagging DEFAULT_MAX_LENGTH = 100_000 diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index 3c31c95d2ff..76ef8b507da 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -800,17 +800,6 @@ end end - describe '#set_error' do - it 'sets errors' do - trace_op.send(:set_error, Exception.new('foo')) - trace_op.measure('top') {} - - trace = trace_op.flush! - - expect(trace.tags).to include('error.msg' => 'foo', 'error.type' => 'Exception') - end - end - describe '#set_tag' do it 'sets tag on trace before a measurement' do trace_op.send(:set_tag, 'foo', 'bar') @@ -863,24 +852,6 @@ expect(trace.tags).to include('foo' => 42) end - it 'sets analytics enabled' do - trace_op.send(:set_tag, Datadog::Tracing::Metadata::Ext::Analytics::TAG_ENABLED, true) - trace_op.measure('top') {} - - trace = trace_op.flush! - - expect(trace.tags).to include(Datadog::Tracing::Metadata::Ext::Analytics::TAG_SAMPLE_RATE => 1.0) - end - - it 'sets analytics sample rate' do - trace_op.send(:set_tag, Datadog::Tracing::Metadata::Ext::Analytics::TAG_SAMPLE_RATE, 42) - trace_op.measure('top') {} - - trace = trace_op.flush! - - expect(trace.tags).to include(Datadog::Tracing::Metadata::Ext::Analytics::TAG_SAMPLE_RATE => 42.0) - end - context 'with partial flushing' do subject(:flush!) { trace_op.flush! } let(:trace) { flush! } From 7a521711e3e17d6c99a378c8a444294302edfe5f Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Tue, 12 Apr 2022 23:27:56 +0200 Subject: [PATCH 10/21] Stop using send for public Metadata::Tagging methods --- lib/datadog/appsec/event.rb | 4 ++-- spec/datadog/tracing/trace_operation_spec.rb | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/datadog/appsec/event.rb b/lib/datadog/appsec/event.rb index 025dd28f1a0..536316dc730 100644 --- a/lib/datadog/appsec/event.rb +++ b/lib/datadog/appsec/event.rb @@ -93,10 +93,10 @@ def self.record_via_span(*events) # complex types are unsupported, we need to serialize to a string triggers = trace_tags.delete('_dd.appsec.triggers') - trace.send(:set_tag, '_dd.appsec.json', JSON.dump({ triggers: triggers })) + trace.set_tag('_dd.appsec.json', JSON.dump({ triggers: triggers })) trace_tags.each do |key, value| - trace.send(:set_tag, key, value.is_a?(String) ? value.encode('UTF-8') : value) + trace.set_tag(key, value.is_a?(String) ? value.encode('UTF-8') : value) end end end diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index 76ef8b507da..9edaa2c4e02 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -777,7 +777,7 @@ describe '#get_tag' do before do - trace_op.send(:set_tag, 'foo', 'bar') + trace_op.set_tag('foo', 'bar') end it 'gets tag set on trace' do @@ -791,7 +791,7 @@ describe '#set_metric' do it 'sets metrics' do - trace_op.send(:set_metric, 'foo', 42) + trace_op.set_metric('foo', 42) trace_op.measure('top') {} trace = trace_op.flush! @@ -802,7 +802,7 @@ describe '#set_tag' do it 'sets tag on trace before a measurement' do - trace_op.send(:set_tag, 'foo', 'bar') + trace_op.set_tag('foo', 'bar') trace_op.measure('top') {} trace = trace_op.flush! @@ -812,7 +812,7 @@ it 'sets tag on trace after a measurement' do trace_op.measure('top') {} - trace_op.send(:set_tag, 'foo', 'bar') + trace_op.set_tag('foo', 'bar') trace = trace_op.flush! @@ -821,7 +821,7 @@ it 'sets tag on trace from a measurement' do trace_op.measure('top') do - trace_op.send(:set_tag, 'foo', 'bar') + trace_op.set_tag('foo', 'bar') end trace = trace_op.flush! @@ -832,7 +832,7 @@ it 'sets tag on trace from a nested measurement' do trace_op.measure('grandparent') do trace_op.measure('parent') do - trace_op.send(:set_tag, 'foo', 'bar') + trace_op.set_tag('foo', 'bar') end end @@ -844,7 +844,7 @@ end it 'sets metrics' do - trace_op.send(:set_tag, 'foo', 42) + trace_op.set_tag('foo', 42) trace_op.measure('top') {} trace = trace_op.flush! @@ -859,7 +859,7 @@ it 'sets tag on trace from a nested measurement' do trace_op.measure('grandparent') do trace_op.measure('parent') do - trace_op.send(:set_tag, 'foo', 'bar') + trace_op.set_tag('foo', 'bar') end flush! end From b53631a8b035357fc1903665b2d8825796bc20df Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Tue, 12 Apr 2022 23:30:28 +0200 Subject: [PATCH 11/21] Use Metadata::Tagging for TraceSegment --- lib/datadog/tracing/trace_operation.rb | 3 +- lib/datadog/tracing/trace_segment.rb | 90 ++++++++++---------- lib/ddtrace/transport/trace_formatter.rb | 2 +- spec/datadog/tracing/trace_operation_spec.rb | 20 ++--- spec/datadog/tracing/trace_segment_spec.rb | 11 ++- 5 files changed, 69 insertions(+), 57 deletions(-) diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index 53fdc603bc0..54f16a0e3ff 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -412,7 +412,8 @@ def build_trace(spans, partial = false) name: @name, resource: @resource, service: @service, - tags: meta.merge(metrics), + tags: meta, + metrics: metrics, root_span_id: !partial ? @root_span && @root_span.id : nil ) end diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index 4efd10762fe..4233e104ac8 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -4,6 +4,7 @@ require 'datadog/tracing/sampling/ext' require 'datadog/tracing/metadata/ext' +require 'datadog/tracing/metadata/tagging' module Datadog module Tracing @@ -11,14 +12,15 @@ module Tracing # @public_api # rubocop:disable Metrics/ClassLength class TraceSegment + include Metadata::Tagging + TAG_NAME = 'name'.freeze TAG_RESOURCE = 'resource'.freeze TAG_SERVICE = 'service'.freeze attr_reader \ :id, - :spans, - :tags + :spans if RUBY_VERSION < '2.2' # nil.dup only fails in Ruby 2.1 # Ensures #initialize can call nil.dup safely @@ -50,12 +52,14 @@ def initialize( sample_rate: nil, sampling_priority: nil, service: nil, - tags: nil + tags: nil, + metrics: nil ) @id = id @root_span_id = root_span_id @spans = spans || [] - @tags = tags || {} + + set_tags((tags || {}).merge(metrics || {})) # Set well-known tags self.agent_sample_rate = agent_sample_rate @@ -94,172 +98,172 @@ def size end def agent_sample_rate - tags[Metadata::Ext::Sampling::TAG_AGENT_RATE] + get_metric(Metadata::Ext::Sampling::TAG_AGENT_RATE) end def agent_sample_rate=(value) if value.nil? - tags.delete(Metadata::Ext::Sampling::TAG_AGENT_RATE) + clear_metric(Metadata::Ext::Sampling::TAG_AGENT_RATE) return end - tags[Metadata::Ext::Sampling::TAG_AGENT_RATE] = value + set_metric(Metadata::Ext::Sampling::TAG_AGENT_RATE, value) end def hostname - tags[Metadata::Ext::NET::TAG_HOSTNAME] + get_tag(Metadata::Ext::NET::TAG_HOSTNAME) end def hostname=(value) if value.nil? - tags.delete(Metadata::Ext::NET::TAG_HOSTNAME) + clear_tag(Metadata::Ext::NET::TAG_HOSTNAME) return end - tags[Metadata::Ext::NET::TAG_HOSTNAME] = value + set_tag(Metadata::Ext::NET::TAG_HOSTNAME, value) end def lang - tags[Core::Runtime::Ext::TAG_LANG] + get_tag(Core::Runtime::Ext::TAG_LANG) end def lang=(value) if value.nil? - tags.delete(Core::Runtime::Ext::TAG_LANG) + clear_tag(Core::Runtime::Ext::TAG_LANG) return end - tags[Core::Runtime::Ext::TAG_LANG] = value + set_tag(Core::Runtime::Ext::TAG_LANG, value) end def name - tags[TAG_NAME] + get_tag(TAG_NAME) end def name=(value) if value.nil? - tags.delete(TAG_NAME) + clear_tag(TAG_NAME) return end - tags[TAG_NAME] = value + set_tag(TAG_NAME, value) end def origin - tags[Metadata::Ext::Distributed::TAG_ORIGIN] + get_tag(Metadata::Ext::Distributed::TAG_ORIGIN) end def origin=(value) if value.nil? - tags.delete(Metadata::Ext::Distributed::TAG_ORIGIN) + clear_tag(Metadata::Ext::Distributed::TAG_ORIGIN) return end - tags[Metadata::Ext::Distributed::TAG_ORIGIN] = value + set_tag(Metadata::Ext::Distributed::TAG_ORIGIN, value) end def process_id - tags[Core::Runtime::Ext::TAG_PID] + get_tag(Core::Runtime::Ext::TAG_PID) end def process_id=(value) if value.nil? - tags.delete(Core::Runtime::Ext::TAG_PID) + clear_tag(Core::Runtime::Ext::TAG_PID) return end - tags[Core::Runtime::Ext::TAG_PID] = value + set_tag(Core::Runtime::Ext::TAG_PID, value) end def rate_limiter_rate - tags[Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE] + get_metric(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) end def rate_limiter_rate=(value) if value.nil? - tags.delete(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) + clear_metric(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) return end - tags[Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE] = value + set_metric(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE, value) end def resource - tags[TAG_RESOURCE] + get_tag(TAG_RESOURCE) end def resource=(value) if value.nil? - tags.delete(TAG_RESOURCE) + clear_tag(TAG_RESOURCE) return end - tags[TAG_RESOURCE] = value + set_tag(TAG_RESOURCE, value) end def rule_sample_rate - tags[Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE] + get_metric(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) end def rule_sample_rate=(value) if value.nil? - tags.delete(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) + clear_metric(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) return end - tags[Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE] = value + set_metric(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE, value) end def runtime_id - tags[Core::Runtime::Ext::TAG_ID] + get_tag(Core::Runtime::Ext::TAG_ID) end def runtime_id=(value) if value.nil? - tags.delete(Core::Runtime::Ext::TAG_ID) + clear_tag(Core::Runtime::Ext::TAG_ID) return end - tags[Core::Runtime::Ext::TAG_ID] = value + set_tag(Core::Runtime::Ext::TAG_ID, value) end def sample_rate - tags[Metadata::Ext::Sampling::TAG_SAMPLE_RATE] + get_metric(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) end def sample_rate=(value) if value.nil? - tags.delete(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) + clear_metric(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) return end - tags[Metadata::Ext::Sampling::TAG_SAMPLE_RATE] = value + set_metric(Metadata::Ext::Sampling::TAG_SAMPLE_RATE, value) end def sampling_priority - tags[Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY] + get_tag(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) end def sampling_priority=(value) if value.nil? - tags.delete(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) + clear_tag(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) return end - tags[Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY] = value + set_tag(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY, value) end def service - tags[TAG_SERVICE] + get_tag(TAG_SERVICE) end def service=(value) if value.nil? - tags.delete(TAG_SERVICE) + clear_tag(TAG_SERVICE) return end - tags[TAG_SERVICE] = value + set_tag(TAG_SERVICE, value) end # If an active trace is present, forces it to be retained by the Datadog backend. diff --git a/lib/ddtrace/transport/trace_formatter.rb b/lib/ddtrace/transport/trace_formatter.rb index 55b06a77c85..5f0bdc43c8e 100644 --- a/lib/ddtrace/transport/trace_formatter.rb +++ b/lib/ddtrace/transport/trace_formatter.rb @@ -70,7 +70,7 @@ def set_trace_tags! # trace. return if !@found_root_span - root_span.set_tags(trace.tags) + root_span.set_tags(trace.send(:meta).merge(trace.send(:metrics))) end def tag_agent_sample_rate! diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index 9edaa2c4e02..d679661aaa1 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -781,11 +781,11 @@ end it 'gets tag set on trace' do - expect(trace_op.send(:get_tag, 'foo')).to eq('bar') + expect(trace_op.get_tag('foo')).to eq('bar') end it 'gets unset tag as nil' do - expect(trace_op.send(:get_tag, 'unset')).to be_nil + expect(trace_op.get_tag('unset')).to be_nil end end @@ -796,7 +796,7 @@ trace = trace_op.flush! - expect(trace.tags).to include('foo' => 42) + expect(trace.get_metric('foo')).to eq(42) end end @@ -807,7 +807,7 @@ trace = trace_op.flush! - expect(trace.tags).to include('foo' => 'bar') + expect(trace.get_tag('foo')).to eq('bar') end it 'sets tag on trace after a measurement' do @@ -816,7 +816,7 @@ trace = trace_op.flush! - expect(trace.tags).to include('foo' => 'bar') + expect(trace.get_tag('foo')).to eq('bar') end it 'sets tag on trace from a measurement' do @@ -826,7 +826,7 @@ trace = trace_op.flush! - expect(trace.tags).to include('foo' => 'bar') + expect(trace.get_tag('foo')).to eq('bar') end it 'sets tag on trace from a nested measurement' do @@ -840,7 +840,7 @@ expect(trace.spans).to have(2).items expect(trace.spans.map(&:name)).to include('parent') - expect(trace.tags).to include('foo' => 'bar') + expect(trace.get_tag('foo')).to eq('bar') end it 'sets metrics' do @@ -849,7 +849,7 @@ trace = trace_op.flush! - expect(trace.tags).to include('foo' => 42) + expect(trace.get_metric('foo')).to eq(42) end context 'with partial flushing' do @@ -866,12 +866,12 @@ expect(trace.spans).to have(1).items expect(trace.spans.map(&:name)).to include('parent') - expect(trace.tags).to include('foo' => 'bar') + expect(trace.get_tag('foo')).to eq('bar') final_flush = trace_op.flush! expect(final_flush.spans).to have(1).items expect(final_flush.spans.map(&:name)).to include('grandparent') - expect(final_flush.tags).to include('foo' => 'bar') + expect(final_flush.get_tag('foo')).to eq('bar') end end end diff --git a/spec/datadog/tracing/trace_segment_spec.rb b/spec/datadog/tracing/trace_segment_spec.rb index 049b974d765..b3971bf47e7 100644 --- a/spec/datadog/tracing/trace_segment_spec.rb +++ b/spec/datadog/tracing/trace_segment_spec.rb @@ -45,9 +45,16 @@ sampling_priority: nil, service: nil, spans: spans, - tags: {} ) end + + it do + expect(subject.send(:meta)).to eq({}) + end + + it do + expect(subject.send(:metrics)).to eq({}) + end end context 'given' do @@ -118,7 +125,7 @@ let(:options) { { runtime_id: runtime_id } } let(:runtime_id) { Datadog::Core::Environment::Identity.id } - it { is_expected.to have_attributes(runtime_id: be(runtime_id)) } + it { is_expected.to have_attributes(runtime_id: runtime_id) } end context ':sample_rate' do From ebd12ee62241c0aa867a6cd91ffdf2139386418b Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Tue, 12 Apr 2022 23:35:31 +0200 Subject: [PATCH 12/21] Materialize 'middle-of-trace' semantics for TraceSegment This makes explicit the condition laid out in the comments --- lib/ddtrace/transport/trace_formatter.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/ddtrace/transport/trace_formatter.rb b/lib/ddtrace/transport/trace_formatter.rb index 5f0bdc43c8e..0db01c5a89a 100644 --- a/lib/ddtrace/transport/trace_formatter.rb +++ b/lib/ddtrace/transport/trace_formatter.rb @@ -59,7 +59,7 @@ def set_resource! # If the trace resource is undefined, or the root span wasn't # specified, don't set this. We don't want to overwrite the # resource of a span that is in the middle of the trace. - return if trace.resource.nil? || !@found_root_span + return if trace.resource.nil? || partial? root_span.resource = trace.resource end @@ -68,7 +68,7 @@ def set_trace_tags! # If the root span wasn't specified, don't set this. We don't want to # misset or overwrite the tags of a span that is in the middle of the # trace. - return if !@found_root_span + return if partial? root_span.set_tags(trace.send(:meta).merge(trace.send(:metrics))) end @@ -165,6 +165,10 @@ def tag_sampling_priority! private + def partial? + !@found_root_span + end + def find_root_span(trace) # TODO: Should we memoize this? # Would be safe, but `spans` is mutable, so if From 14d10b6e47de1ab5ec7ded64ac834f0c1b11aba2 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Tue, 12 Apr 2022 23:40:59 +0200 Subject: [PATCH 13/21] Remove redundant conversion to UTF-8 --- lib/datadog/appsec/event.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/datadog/appsec/event.rb b/lib/datadog/appsec/event.rb index 536316dc730..d12bd63de3c 100644 --- a/lib/datadog/appsec/event.rb +++ b/lib/datadog/appsec/event.rb @@ -96,7 +96,7 @@ def self.record_via_span(*events) trace.set_tag('_dd.appsec.json', JSON.dump({ triggers: triggers })) trace_tags.each do |key, value| - trace.set_tag(key, value.is_a?(String) ? value.encode('UTF-8') : value) + trace.set_tag(key, value) end end end From 942570ece0e2eed2dca0c6a1443f2f3e55c40021 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Thu, 14 Apr 2022 12:08:46 +0200 Subject: [PATCH 14/21] Make TraceSegment tag handling internal This keeps the meta/metrics split but removes the Metadata::Tagging behaviour which is redundantly executed (TraceOperation is supposed to having handled that). `meta` and `metrics` become protected accessors and internal details. Additionally, setting meta and metrics is split, avoiding a hash merge. --- lib/datadog/tracing/trace_segment.rb | 89 ++++++++++---------- lib/ddtrace/transport/trace_formatter.rb | 3 +- spec/datadog/tracing/trace_operation_spec.rb | 16 ++-- 3 files changed, 56 insertions(+), 52 deletions(-) diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index 4233e104ac8..249ea7cfc5c 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -12,8 +12,6 @@ module Tracing # @public_api # rubocop:disable Metrics/ClassLength class TraceSegment - include Metadata::Tagging - TAG_NAME = 'name'.freeze TAG_RESOURCE = 'resource'.freeze TAG_SERVICE = 'service'.freeze @@ -59,7 +57,10 @@ def initialize( @root_span_id = root_span_id @spans = spans || [] - set_tags((tags || {}).merge(metrics || {})) + # Does not make an effort to move metrics out of tags + # The caller is expected to have done that + @meta = tags || {} + @metrics = metrics || {} # Set well-known tags self.agent_sample_rate = agent_sample_rate @@ -98,172 +99,172 @@ def size end def agent_sample_rate - get_metric(Metadata::Ext::Sampling::TAG_AGENT_RATE) + metrics[Metadata::Ext::Sampling::TAG_AGENT_RATE] end def agent_sample_rate=(value) if value.nil? - clear_metric(Metadata::Ext::Sampling::TAG_AGENT_RATE) + metrics.delete(Metadata::Ext::Sampling::TAG_AGENT_RATE) return end - set_metric(Metadata::Ext::Sampling::TAG_AGENT_RATE, value) + metrics[Metadata::Ext::Sampling::TAG_AGENT_RATE] = value end def hostname - get_tag(Metadata::Ext::NET::TAG_HOSTNAME) + meta[Metadata::Ext::NET::TAG_HOSTNAME] end def hostname=(value) if value.nil? - clear_tag(Metadata::Ext::NET::TAG_HOSTNAME) + meta.delete(Metadata::Ext::NET::TAG_HOSTNAME) return end - set_tag(Metadata::Ext::NET::TAG_HOSTNAME, value) + meta[Metadata::Ext::NET::TAG_HOSTNAME] = value end def lang - get_tag(Core::Runtime::Ext::TAG_LANG) + meta[Core::Runtime::Ext::TAG_LANG] end def lang=(value) if value.nil? - clear_tag(Core::Runtime::Ext::TAG_LANG) + meta.delete(Core::Runtime::Ext::TAG_LANG) return end - set_tag(Core::Runtime::Ext::TAG_LANG, value) + meta[Core::Runtime::Ext::TAG_LANG] = value end def name - get_tag(TAG_NAME) + meta[TAG_NAME] end def name=(value) if value.nil? - clear_tag(TAG_NAME) + meta.delete(TAG_NAME) return end - set_tag(TAG_NAME, value) + meta[TAG_NAME] = value end def origin - get_tag(Metadata::Ext::Distributed::TAG_ORIGIN) + meta[Metadata::Ext::Distributed::TAG_ORIGIN] end def origin=(value) if value.nil? - clear_tag(Metadata::Ext::Distributed::TAG_ORIGIN) + meta.delete(Metadata::Ext::Distributed::TAG_ORIGIN) return end - set_tag(Metadata::Ext::Distributed::TAG_ORIGIN, value) + meta[Metadata::Ext::Distributed::TAG_ORIGIN] = value end def process_id - get_tag(Core::Runtime::Ext::TAG_PID) + meta[Core::Runtime::Ext::TAG_PID] end def process_id=(value) if value.nil? - clear_tag(Core::Runtime::Ext::TAG_PID) + meta.delete(Core::Runtime::Ext::TAG_PID) return end - set_tag(Core::Runtime::Ext::TAG_PID, value) + meta[Core::Runtime::Ext::TAG_PID] = value end def rate_limiter_rate - get_metric(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) + metrics[Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE] end def rate_limiter_rate=(value) if value.nil? - clear_metric(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) + metrics.delete(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) return end - set_metric(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE, value) + metrics[Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE] = value end def resource - get_tag(TAG_RESOURCE) + meta[TAG_RESOURCE] end def resource=(value) if value.nil? - clear_tag(TAG_RESOURCE) + meta.delete(TAG_RESOURCE) return end - set_tag(TAG_RESOURCE, value) + meta[TAG_RESOURCE] = value end def rule_sample_rate - get_metric(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) + metrics[Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE] end def rule_sample_rate=(value) if value.nil? - clear_metric(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) + metrics.delete(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) return end - set_metric(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE, value) + metrics[Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE] = value end def runtime_id - get_tag(Core::Runtime::Ext::TAG_ID) + meta[Core::Runtime::Ext::TAG_ID] end def runtime_id=(value) if value.nil? - clear_tag(Core::Runtime::Ext::TAG_ID) + meta.delete(Core::Runtime::Ext::TAG_ID) return end - set_tag(Core::Runtime::Ext::TAG_ID, value) + meta[Core::Runtime::Ext::TAG_ID] = value end def sample_rate - get_metric(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) + metrics[Metadata::Ext::Sampling::TAG_SAMPLE_RATE] end def sample_rate=(value) if value.nil? - clear_metric(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) + metrics.delete(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) return end - set_metric(Metadata::Ext::Sampling::TAG_SAMPLE_RATE, value) + metrics[Metadata::Ext::Sampling::TAG_SAMPLE_RATE] = value end def sampling_priority - get_tag(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) + meta[Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY] end def sampling_priority=(value) if value.nil? - clear_tag(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) + meta.delete(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) return end - set_tag(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY, value) + meta[Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY] = value end def service - get_tag(TAG_SERVICE) + meta[TAG_SERVICE] end def service=(value) if value.nil? - clear_tag(TAG_SERVICE) + meta.delete(TAG_SERVICE) return end - set_tag(TAG_SERVICE, value) + meta[TAG_SERVICE] = value end # If an active trace is present, forces it to be retained by the Datadog backend. @@ -292,7 +293,9 @@ def sampled? protected attr_reader \ - :root_span_id + :root_span_id, + :meta, + :metrics end # rubocop:enable Metrics/ClassLength end diff --git a/lib/ddtrace/transport/trace_formatter.rb b/lib/ddtrace/transport/trace_formatter.rb index 0db01c5a89a..c413075a3d0 100644 --- a/lib/ddtrace/transport/trace_formatter.rb +++ b/lib/ddtrace/transport/trace_formatter.rb @@ -70,7 +70,8 @@ def set_trace_tags! # trace. return if partial? - root_span.set_tags(trace.send(:meta).merge(trace.send(:metrics))) + root_span.set_tags(trace.send(:meta)) + root_span.set_tags(trace.send(:metrics)) end def tag_agent_sample_rate! diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index d679661aaa1..96c0abe525e 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -796,7 +796,7 @@ trace = trace_op.flush! - expect(trace.get_metric('foo')).to eq(42) + expect(trace.send(:metrics)['foo']).to eq(42) end end @@ -807,7 +807,7 @@ trace = trace_op.flush! - expect(trace.get_tag('foo')).to eq('bar') + expect(trace.send(:meta)['foo']).to eq('bar') end it 'sets tag on trace after a measurement' do @@ -816,7 +816,7 @@ trace = trace_op.flush! - expect(trace.get_tag('foo')).to eq('bar') + expect(trace.send(:meta)['foo']).to eq('bar') end it 'sets tag on trace from a measurement' do @@ -826,7 +826,7 @@ trace = trace_op.flush! - expect(trace.get_tag('foo')).to eq('bar') + expect(trace.send(:meta)['foo']).to eq('bar') end it 'sets tag on trace from a nested measurement' do @@ -840,7 +840,7 @@ expect(trace.spans).to have(2).items expect(trace.spans.map(&:name)).to include('parent') - expect(trace.get_tag('foo')).to eq('bar') + expect(trace.send(:meta)['foo']).to eq('bar') end it 'sets metrics' do @@ -849,7 +849,7 @@ trace = trace_op.flush! - expect(trace.get_metric('foo')).to eq(42) + expect(trace.send(:metrics)['foo']).to eq(42) end context 'with partial flushing' do @@ -866,12 +866,12 @@ expect(trace.spans).to have(1).items expect(trace.spans.map(&:name)).to include('parent') - expect(trace.get_tag('foo')).to eq('bar') + expect(trace.send(:meta)['foo']).to eq('bar') final_flush = trace_op.flush! expect(final_flush.spans).to have(1).items expect(final_flush.spans.map(&:name)).to include('grandparent') - expect(final_flush.get_tag('foo')).to eq('bar') + expect(final_flush.send(:meta)['foo']).to eq('bar') end end end From 40c36af4606a83aa97ae76deb756e8d617929019 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Thu, 14 Apr 2022 13:22:54 +0200 Subject: [PATCH 15/21] Ensure TraceSegment attribute values can come from tags --- lib/datadog/tracing/trace_segment.rb | 28 +++--- spec/datadog/tracing/trace_segment_spec.rb | 105 ++++++++++++++++++++- 2 files changed, 116 insertions(+), 17 deletions(-) diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index 249ea7cfc5c..eba0ceb2c36 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -62,20 +62,20 @@ def initialize( @meta = tags || {} @metrics = metrics || {} - # Set well-known tags - self.agent_sample_rate = agent_sample_rate - self.hostname = hostname - self.lang = lang - self.name = (name.frozen? ? name : name.dup) - self.origin = (origin.frozen? ? origin : origin.dup) - self.process_id = process_id - self.rate_limiter_rate = rate_limiter_rate - self.resource = (resource.frozen? ? resource : resource.dup) - self.rule_sample_rate = rule_sample_rate - self.runtime_id = runtime_id - self.sample_rate = sample_rate - self.sampling_priority = sampling_priority - self.service = (service.frozen? ? service : service.dup) + # Set well-known tags, defaulting to getting the values from tags + self.agent_sample_rate = agent_sample_rate || self.agent_sample_rate + self.hostname = hostname || self.hostname + self.lang = lang || self.lang + self.name = (name || self.name).tap { |v| break (v.frozen? ? v : v.dup) } + self.origin = (origin || self.origin).tap { |v| break (v.frozen? ? v : v.dup) } + self.process_id = process_id || self.process_id + self.rate_limiter_rate = rate_limiter_rate || self.rate_limiter_rate + self.resource = (resource || self.resource).tap { |v| break (v.frozen? ? v : v.dup) } + self.rule_sample_rate = rule_sample_rate || self.rule_sample_rate + self.runtime_id = runtime_id || self.runtime_id + self.sample_rate = sample_rate || self.sample_rate + self.sampling_priority = sampling_priority || self.sampling_priority + self.service = (service || self.service).tap { |v| break (v.frozen? ? v : v.dup) } end def any? diff --git a/spec/datadog/tracing/trace_segment_spec.rb b/spec/datadog/tracing/trace_segment_spec.rb index b3971bf47e7..b40cd66cab4 100644 --- a/spec/datadog/tracing/trace_segment_spec.rb +++ b/spec/datadog/tracing/trace_segment_spec.rb @@ -49,15 +49,15 @@ end it do - expect(subject.send(:meta)).to eq({}) + expect(trace_segment.send(:meta)).to eq({}) end it do - expect(subject.send(:metrics)).to eq({}) + expect(trace_segment.send(:metrics)).to eq({}) end end - context 'given' do + context 'given arguments' do context ':agent_sample_rate' do let(:options) { { agent_sample_rate: agent_sample_rate } } let(:agent_sample_rate) { rand } @@ -149,6 +149,105 @@ it { is_expected.to have_attributes(service: be_a_copy_of(service)) } end end + + context 'given tags' do + context ':agent_sample_rate' do + let(:options) { { metrics: { Datadog::Tracing::Metadata::Ext::Sampling::TAG_AGENT_RATE => agent_sample_rate } } } + let(:agent_sample_rate) { rand } + + it { is_expected.to have_attributes(agent_sample_rate: agent_sample_rate) } + end + + context ':hostname' do + let(:options) { { tags: { Datadog::Tracing::Metadata::Ext::NET::TAG_HOSTNAME => hostname } } } + let(:hostname) { 'my.host' } + + it { is_expected.to have_attributes(hostname: be(hostname)) } + end + + context ':lang' do + let(:options) { { tags: { Datadog::Core::Runtime::Ext::TAG_LANG => lang } } } + let(:lang) { 'ruby' } + + it { is_expected.to have_attributes(lang: be(lang)) } + end + + context ':name' do + let(:options) { { tags: { Datadog::Tracing::TraceSegment::TAG_NAME => name } } } + let(:name) { 'job.work' } + + it { is_expected.to have_attributes(name: be_a_copy_of(name)) } + end + + context ':origin' do + let(:options) { { tags: { Datadog::Tracing::Metadata::Ext::Distributed::TAG_ORIGIN => origin } } } + let(:origin) { 'synthetics' } + + it { is_expected.to have_attributes(origin: be_a_copy_of(origin)) } + end + + context ':process_id' do + let(:options) { { tags: { Datadog::Core::Runtime::Ext::TAG_PID => process_id } } } + let(:process_id) { Datadog::Core::Environment::Identity.pid } + + it { is_expected.to have_attributes(process_id: process_id) } + end + + context ':rate_limiter_rate' do + let(:options) do + { metrics: { Datadog::Tracing::Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE => rate_limiter_rate } } + end + let(:rate_limiter_rate) { rand } + + it { is_expected.to have_attributes(rate_limiter_rate: rate_limiter_rate) } + end + + context ':resource' do + let(:options) { { tags: { Datadog::Tracing::TraceSegment::TAG_RESOURCE => resource } } } + let(:resource) { 'generate_report' } + + it { is_expected.to have_attributes(resource: be_a_copy_of(resource)) } + end + + context ':rule_sample_rate' do + let(:options) do + { metrics: { Datadog::Tracing::Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE => rule_sample_rate } } + end + let(:rule_sample_rate) { rand } + + it { is_expected.to have_attributes(rule_sample_rate: rule_sample_rate) } + end + + context ':runtime_id' do + let(:options) { { tags: { Datadog::Core::Runtime::Ext::TAG_ID => runtime_id } } } + let(:runtime_id) { Datadog::Core::Environment::Identity.id } + + it { is_expected.to have_attributes(runtime_id: runtime_id) } + end + + context ':sample_rate' do + let(:options) { { metrics: { Datadog::Tracing::Metadata::Ext::Sampling::TAG_SAMPLE_RATE => sample_rate } } } + let(:sample_rate) { rand } + + it { is_expected.to have_attributes(sample_rate: sample_rate) } + end + + context ':sampling_priority' do + let(:options) do + { tags: { Datadog::Tracing::Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY => sampling_priority } } + end + let(:sampling_priority) { Datadog::Tracing::Sampling::Ext::Priority::USER_KEEP } + + it { is_expected.to have_attributes(sampling_priority: sampling_priority) } + end + + context ':service' do + let(:options) { { tags: { Datadog::Tracing::TraceSegment::TAG_SERVICE => service } } } + let(:service) { 'job-worker' } + + it { is_expected.to have_attributes(service: be_a_copy_of(service)) } + end + end end describe 'forwarded #spans methods' do From f4ff9dfb00b33b5211135afa5f262b4e5326742d Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Thu, 14 Apr 2022 13:55:02 +0200 Subject: [PATCH 16/21] Use TraceSegment instance variables instead of hashes Also makes writers private since TraceSegment is supposed to be immutable from the outside. --- lib/datadog/tracing/trace_segment.rb | 269 ++++++++------------- spec/datadog/tracing/trace_segment_spec.rb | 10 +- 2 files changed, 107 insertions(+), 172 deletions(-) diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index eba0ceb2c36..2a4791ad4cd 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -18,7 +18,21 @@ class TraceSegment attr_reader \ :id, - :spans + :spans, + :agent_sample_rate, + :hostname, + :id, + :lang, + :name, + :origin, + :process_id, + :rate_limiter_rate, + :resource, + :rule_sample_rate, + :runtime_id, + :sample_rate, + :sampling_priority, + :service if RUBY_VERSION < '2.2' # nil.dup only fails in Ruby 2.1 # Ensures #initialize can call nil.dup safely @@ -63,19 +77,21 @@ def initialize( @metrics = metrics || {} # Set well-known tags, defaulting to getting the values from tags - self.agent_sample_rate = agent_sample_rate || self.agent_sample_rate - self.hostname = hostname || self.hostname - self.lang = lang || self.lang - self.name = (name || self.name).tap { |v| break (v.frozen? ? v : v.dup) } - self.origin = (origin || self.origin).tap { |v| break (v.frozen? ? v : v.dup) } - self.process_id = process_id || self.process_id - self.rate_limiter_rate = rate_limiter_rate || self.rate_limiter_rate - self.resource = (resource || self.resource).tap { |v| break (v.frozen? ? v : v.dup) } - self.rule_sample_rate = rule_sample_rate || self.rule_sample_rate - self.runtime_id = runtime_id || self.runtime_id - self.sample_rate = sample_rate || self.sample_rate - self.sampling_priority = sampling_priority || self.sampling_priority - self.service = (service || self.service).tap { |v| break (v.frozen? ? v : v.dup) } + @agent_sample_rate = agent_sample_rate || agent_sample_rate_tag + @hostname = hostname || hostname_tag + @lang = lang || lang_tag + @name = (name || name_tag).tap { |v| break (v.frozen? ? v : v.dup) } + @origin = (origin || origin_tag).tap { |v| break (v.frozen? ? v : v.dup) } + @process_id = process_id || process_id_tag + @rate_limiter_rate = rate_limiter_rate || rate_limiter_rate_tag + @resource = (resource || resource_tag).tap { |v| break (v.frozen? ? v : v.dup) } + @rule_sample_rate = rule_sample_rate_tag || rule_sample_rate + @runtime_id = runtime_id || runtime_id_tag + @sample_rate = sample_rate || sample_rate_tag + @sampling_priority = sampling_priority || sampling_priority_tag + @service = (service || service_tag).tap { |v| break (v.frozen? ? v : v.dup) } + + clear_known_tags end def any? @@ -98,204 +114,121 @@ def size @spans.size end - def agent_sample_rate - metrics[Metadata::Ext::Sampling::TAG_AGENT_RATE] + # If an active trace is present, forces it to be retained by the Datadog backend. + # + # Any sampling logic will not be able to change this decision. + # + # @return [void] + def keep! + self.sampling_priority = Sampling::Ext::Priority::USER_KEEP end - def agent_sample_rate=(value) - if value.nil? - metrics.delete(Metadata::Ext::Sampling::TAG_AGENT_RATE) - return - end - - metrics[Metadata::Ext::Sampling::TAG_AGENT_RATE] = value + # If an active trace is present, forces it to be dropped and not stored by the Datadog backend. + # + # Any sampling logic will not be able to change this decision. + # + # @return [void] + def reject! + self.sampling_priority = Sampling::Ext::Priority::USER_REJECT end - def hostname - meta[Metadata::Ext::NET::TAG_HOSTNAME] + def sampled? + sampling_priority == Sampling::Ext::Priority::AUTO_KEEP \ + || sampling_priority == Sampling::Ext::Priority::USER_KEEP end - def hostname=(value) - if value.nil? - meta.delete(Metadata::Ext::NET::TAG_HOSTNAME) - return - end - - meta[Metadata::Ext::NET::TAG_HOSTNAME] = value - end + protected - def lang - meta[Core::Runtime::Ext::TAG_LANG] - end + attr_reader \ + :root_span_id, + :meta, + :metrics - def lang=(value) - if value.nil? - meta.delete(Core::Runtime::Ext::TAG_LANG) - return - end + private - meta[Core::Runtime::Ext::TAG_LANG] = value + attr_writer \ + :agent_sample_rate, + :hostname, + :id, + :lang, + :name, + :origin, + :process_id, + :rate_limiter_rate, + :resource, + :rule_sample_rate, + :runtime_id, + :sample_rate, + :sampling_priority, + :service + + def agent_sample_rate_tag + metrics[Metadata::Ext::Sampling::TAG_AGENT_RATE] end - def name - meta[TAG_NAME] + def hostname_tag + meta[Metadata::Ext::NET::TAG_HOSTNAME] end - def name=(value) - if value.nil? - meta.delete(TAG_NAME) - return - end - - meta[TAG_NAME] = value + def lang_tag + meta[Core::Runtime::Ext::TAG_LANG] end - def origin - meta[Metadata::Ext::Distributed::TAG_ORIGIN] + def name_tag + meta[TAG_NAME] end - def origin=(value) - if value.nil? - meta.delete(Metadata::Ext::Distributed::TAG_ORIGIN) - return - end - - meta[Metadata::Ext::Distributed::TAG_ORIGIN] = value + def origin_tag + meta[Metadata::Ext::Distributed::TAG_ORIGIN] end - def process_id + def process_id_tag meta[Core::Runtime::Ext::TAG_PID] end - def process_id=(value) - if value.nil? - meta.delete(Core::Runtime::Ext::TAG_PID) - return - end - - meta[Core::Runtime::Ext::TAG_PID] = value - end - - def rate_limiter_rate + def rate_limiter_rate_tag metrics[Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE] end - def rate_limiter_rate=(value) - if value.nil? - metrics.delete(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) - return - end - - metrics[Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE] = value - end - - def resource + def resource_tag meta[TAG_RESOURCE] end - def resource=(value) - if value.nil? - meta.delete(TAG_RESOURCE) - return - end - - meta[TAG_RESOURCE] = value - end - - def rule_sample_rate + def rule_sample_rate_tag metrics[Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE] end - def rule_sample_rate=(value) - if value.nil? - metrics.delete(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) - return - end - - metrics[Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE] = value - end - - def runtime_id + def runtime_id_tag meta[Core::Runtime::Ext::TAG_ID] end - def runtime_id=(value) - if value.nil? - meta.delete(Core::Runtime::Ext::TAG_ID) - return - end - - meta[Core::Runtime::Ext::TAG_ID] = value - end - - def sample_rate + def sample_rate_tag metrics[Metadata::Ext::Sampling::TAG_SAMPLE_RATE] end - def sample_rate=(value) - if value.nil? - metrics.delete(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) - return - end - - metrics[Metadata::Ext::Sampling::TAG_SAMPLE_RATE] = value - end - - def sampling_priority + def sampling_priority_tag meta[Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY] end - def sampling_priority=(value) - if value.nil? - meta.delete(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) - return - end - - meta[Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY] = value - end - - def service + def service_tag meta[TAG_SERVICE] end - def service=(value) - if value.nil? - meta.delete(TAG_SERVICE) - return - end - - meta[TAG_SERVICE] = value - end - - # If an active trace is present, forces it to be retained by the Datadog backend. - # - # Any sampling logic will not be able to change this decision. - # - # @return [void] - def keep! - self.sampling_priority = Sampling::Ext::Priority::USER_KEEP - end - - # If an active trace is present, forces it to be dropped and not stored by the Datadog backend. - # - # Any sampling logic will not be able to change this decision. - # - # @return [void] - def reject! - self.sampling_priority = Sampling::Ext::Priority::USER_REJECT - end - - def sampled? - sampling_priority == Sampling::Ext::Priority::AUTO_KEEP \ - || sampling_priority == Sampling::Ext::Priority::USER_KEEP + def clear_known_tags + metrics.delete(Metadata::Ext::Sampling::TAG_AGENT_RATE) + meta.delete(Metadata::Ext::NET::TAG_HOSTNAME) + meta.delete(Core::Runtime::Ext::TAG_LANG) + meta.delete(TAG_NAME) + meta.delete(Metadata::Ext::Distributed::TAG_ORIGIN) + meta.delete(Core::Runtime::Ext::TAG_PID) + metrics.delete(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) + meta.delete(TAG_RESOURCE) + metrics.delete(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) + meta.delete(Core::Runtime::Ext::TAG_ID) + metrics.delete(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) + meta.delete(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) + meta.delete(TAG_SERVICE) end - - protected - - attr_reader \ - :root_span_id, - :meta, - :metrics end # rubocop:enable Metrics/ClassLength end diff --git a/spec/datadog/tracing/trace_segment_spec.rb b/spec/datadog/tracing/trace_segment_spec.rb index b40cd66cab4..7163d616afd 100644 --- a/spec/datadog/tracing/trace_segment_spec.rb +++ b/spec/datadog/tracing/trace_segment_spec.rb @@ -291,31 +291,33 @@ describe '#sampled?' do subject(:sampled?) { trace_segment.sampled? } + let(:options) { { sampling_priority: sampling_priority } } + let(:sampling_priority) { nil } context 'when sampling priority is not set' do it { is_expected.to be false } end context 'when sampling priority is set to AUTO_KEEP' do - before { trace_segment.sampling_priority = Datadog::Tracing::Sampling::Ext::Priority::AUTO_KEEP } + let(:sampling_priority) { Datadog::Tracing::Sampling::Ext::Priority::AUTO_KEEP } it { is_expected.to be true } end context 'when sampling priority is set to USER_KEEP' do - before { trace_segment.sampling_priority = Datadog::Tracing::Sampling::Ext::Priority::USER_KEEP } + let(:sampling_priority) { Datadog::Tracing::Sampling::Ext::Priority::USER_KEEP } it { is_expected.to be true } end context 'when sampling priority is set to AUTO_REJECT' do - before { trace_segment.sampling_priority = Datadog::Tracing::Sampling::Ext::Priority::AUTO_REJECT } + let(:sampling_priority) { Datadog::Tracing::Sampling::Ext::Priority::AUTO_REJECT } it { is_expected.to be false } end context 'when sampling priority is set to USER_REJECT' do - before { trace_segment.sampling_priority = Datadog::Tracing::Sampling::Ext::Priority::USER_REJECT } + let(:sampling_priority) { Datadog::Tracing::Sampling::Ext::Priority::USER_REJECT } it { is_expected.to be false } end From f92df7ea95cd60f778a4dfc714db3e5e6e526e00 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Thu, 14 Apr 2022 16:38:52 +0200 Subject: [PATCH 17/21] Fix overlooked reference to previous tag instance variable --- lib/datadog/tracing/trace_operation.rb | 7 +- spec/datadog/tracing/trace_operation_spec.rb | 76 +++++++++++++++++++- spec/datadog/tracing/trace_segment_spec.rb | 14 ++++ 3 files changed, 94 insertions(+), 3 deletions(-) diff --git a/lib/datadog/tracing/trace_operation.rb b/lib/datadog/tracing/trace_operation.rb index 54f16a0e3ff..8bdcd39ffd8 100644 --- a/lib/datadog/tracing/trace_operation.rb +++ b/lib/datadog/tracing/trace_operation.rb @@ -67,7 +67,8 @@ def initialize( sampled: nil, sampling_priority: nil, service: nil, - tags: nil + tags: nil, + metrics: nil ) # Attributes @events = events || Events.new @@ -90,6 +91,7 @@ def initialize( # Generic tags set_tags(tags) if tags + set_tags(metrics) if metrics # State @root_span = nil @@ -269,7 +271,8 @@ def fork_clone sampled: @sampled, sampling_priority: @sampling_priority, service: (@service && @service.dup), - tags: @tags.dup + tags: meta.dup, + metrics: metrics.dup ) end diff --git a/spec/datadog/tracing/trace_operation_spec.rb b/spec/datadog/tracing/trace_operation_spec.rb index 96c0abe525e..dda4ee98d64 100644 --- a/spec/datadog/tracing/trace_operation_spec.rb +++ b/spec/datadog/tracing/trace_operation_spec.rb @@ -30,7 +30,9 @@ sample_rate: sample_rate, sampled: sampled, sampling_priority: sampling_priority, - service: service + service: service, + tags: tags, + metrics: metrics } end @@ -46,6 +48,8 @@ let(:sampled) { true } let(:sampling_priority) { Datadog::Tracing::Sampling::Ext::Priority::USER_KEEP } let(:service) { 'billing-api' } + let(:tags) { { 'foo' => 'bar' } } + let(:metrics) { { 'baz' => 42.0 } } end shared_examples 'a span with default events' do @@ -73,6 +77,14 @@ service: nil ) end + + it do + expect(trace_op.send(:meta)).to eq({}) + end + + it do + expect(trace_op.send(:metrics)).to eq({}) + end end context 'given' do @@ -173,6 +185,20 @@ it { expect(trace_op.service).to eq(service) } end + + context ':tags' do + subject(:options) { { tags: tags } } + let(:tags) { { 'foo' => 'bar' } } + + it { expect(trace_op.send(:meta)).to eq({ 'foo' => 'bar' }) } + end + + context ':metrics' do + subject(:options) { { metrics: metrics } } + let(:metrics) { { 'baz' => 42.0 } } + + it { expect(trace_op.send(:metrics)).to eq({ 'baz' => 42.0 }) } + end end end @@ -1770,6 +1796,14 @@ def span ) end + it 'maintains the same tags' do + expect(new_trace_op.send(:meta)).to eq({ 'foo' => 'bar' }) + end + + it 'maintains the same metrics' do + expect(new_trace_op.send(:metrics)).to eq({ 'baz' => 42.0 }) + end + it 'maintains the same events' do old_events = trace_op.send(:events) new_events = new_trace_op.send(:events) @@ -1832,6 +1866,14 @@ def span ) end + it 'maintains the same tags' do + expect(new_trace_op.send(:meta)).to eq({ 'foo' => 'bar' }) + end + + it 'maintains the same metrics' do + expect(new_trace_op.send(:metrics)).to eq({ 'baz' => 42.0 }) + end + context 'and :parent_span_id has been defined' do let(:options) { { parent_span_id: parent_span_id } } let(:parent_span_id) { Datadog::Core::Utils.next_id } @@ -1872,6 +1914,14 @@ def span service: be_a_copy_of(service) ) end + + it 'maintains the same tags' do + expect(new_trace_op.send(:meta)).to eq({ 'foo' => 'bar' }) + end + + it 'maintains the same metrics' do + expect(new_trace_op.send(:metrics)).to eq({ 'baz' => 42.0 }) + end end context 'that has started' do @@ -1903,6 +1953,14 @@ def span service: be_a_copy_of(service) ) end + + it 'maintains the same tags' do + expect(new_trace_op.send(:meta)).to eq({ 'foo' => 'bar' }) + end + + it 'maintains the same metrics' do + expect(new_trace_op.send(:metrics)).to eq({ 'baz' => 42.0 }) + end end context 'that has finished' do @@ -1934,6 +1992,14 @@ def span service: be_a_copy_of(service) ) end + + it 'maintains the same tags' do + expect(new_trace_op.send(:meta)).to eq({ 'foo' => 'bar' }) + end + + it 'maintains the same metrics' do + expect(new_trace_op.send(:metrics)).to eq({ 'baz' => 42.0 }) + end end end @@ -1976,6 +2042,14 @@ def span ) end + it 'maintains the same tags' do + expect(new_trace_op.send(:meta)).to eq({ 'foo' => 'bar' }) + end + + it 'maintains the same metrics' do + expect(new_trace_op.send(:metrics)).to eq({ 'baz' => 42.0 }) + end + context 'and :parent_span_id has been defined' do let(:options) { { parent_span_id: parent_span_id } } let(:parent_span_id) { Datadog::Core::Utils.next_id } diff --git a/spec/datadog/tracing/trace_segment_spec.rb b/spec/datadog/tracing/trace_segment_spec.rb index 7163d616afd..11c90e354c5 100644 --- a/spec/datadog/tracing/trace_segment_spec.rb +++ b/spec/datadog/tracing/trace_segment_spec.rb @@ -148,6 +148,20 @@ it { is_expected.to have_attributes(service: be_a_copy_of(service)) } end + + context ':tags' do + let(:options) { { tags: tags } } + let(:tags) { { 'foo' => 'bar' } } + + it { expect(trace_segment.send(:meta)).to eq({ 'foo' => 'bar' }) } + end + + context ':metrics' do + let(:options) { { metrics: metrics } } + let(:metrics) { { 'foo' => 42.0 } } + + it { expect(trace_segment.send(:metrics)).to eq({ 'foo' => 42.0 }) } + end end context 'given tags' do From 8b8fc7b6bbd58248a2ac96081e8b1a9df3cee113 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Thu, 14 Apr 2022 16:47:16 +0200 Subject: [PATCH 18/21] Address linter issues --- lib/datadog/tracing/trace_segment.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index 2a4791ad4cd..5b870cb7f22 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -21,7 +21,6 @@ class TraceSegment :spans, :agent_sample_rate, :hostname, - :id, :lang, :name, :origin, @@ -47,6 +46,8 @@ def dup using RefineNil end + # rubocop:disable Metrics/CyclomaticComplexity + # rubocop:disable Metrics/PerceivedComplexity def initialize( spans, agent_sample_rate: nil, @@ -93,6 +94,8 @@ def initialize( clear_known_tags end + # rubocop:enable Metrics/PerceivedComplexity + # rubocop:enable Metrics/CyclomaticComplexity def any? @spans.any? From 3f6372690f39ffe82d0d5c366b9688eb8607230a Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Thu, 14 Apr 2022 17:40:06 +0200 Subject: [PATCH 19/21] Attempt symplifying initializer --- lib/datadog/tracing/trace_segment.rb | 96 +++++++++++++++++++--------- 1 file changed, 66 insertions(+), 30 deletions(-) diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index 5b870cb7f22..56e79ab9764 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -18,20 +18,7 @@ class TraceSegment attr_reader \ :id, - :spans, - :agent_sample_rate, - :hostname, - :lang, - :name, - :origin, - :process_id, - :rate_limiter_rate, - :resource, - :rule_sample_rate, - :runtime_id, - :sample_rate, - :sampling_priority, - :service + :spans if RUBY_VERSION < '2.2' # nil.dup only fails in Ruby 2.1 # Ensures #initialize can call nil.dup safely @@ -78,25 +65,75 @@ def initialize( @metrics = metrics || {} # Set well-known tags, defaulting to getting the values from tags - @agent_sample_rate = agent_sample_rate || agent_sample_rate_tag - @hostname = hostname || hostname_tag - @lang = lang || lang_tag - @name = (name || name_tag).tap { |v| break (v.frozen? ? v : v.dup) } - @origin = (origin || origin_tag).tap { |v| break (v.frozen? ? v : v.dup) } - @process_id = process_id || process_id_tag - @rate_limiter_rate = rate_limiter_rate || rate_limiter_rate_tag - @resource = (resource || resource_tag).tap { |v| break (v.frozen? ? v : v.dup) } - @rule_sample_rate = rule_sample_rate_tag || rule_sample_rate - @runtime_id = runtime_id || runtime_id_tag - @sample_rate = sample_rate || sample_rate_tag - @sampling_priority = sampling_priority || sampling_priority_tag - @service = (service || service_tag).tap { |v| break (v.frozen? ? v : v.dup) } - - clear_known_tags + @agent_sample_rate = agent_sample_rate + @hostname = hostname + @lang = lang + @name = name.tap { |v| break (v.frozen? ? v : v.dup) } + @origin = origin.tap { |v| break (v.frozen? ? v : v.dup) } + @process_id = process_id + @rate_limiter_rate = rate_limiter_rate + @resource = resource.tap { |v| break (v.frozen? ? v : v.dup) } + @rule_sample_rate = rule_sample_rate + @runtime_id = runtime_id + @sample_rate = sample_rate + @sampling_priority = sampling_priority + @service = service.tap { |v| break (v.frozen? ? v : v.dup) } end # rubocop:enable Metrics/PerceivedComplexity # rubocop:enable Metrics/CyclomaticComplexity + def agent_sample_rate + @agent_sample_rate ||= agent_sample_rate_tag + end + + def hostname + @hostname ||= hostname_tag + end + + def lang + @lang ||= lang_tag + end + + def name + @name ||= name_tag.tap { |v| break (v.frozen? ? v : v.dup) } + end + + def origin + @origin ||= origin_tag.tap { |v| break (v.frozen? ? v : v.dup) } + end + + def process_id + @process_id ||= process_id_tag + end + + def rate_limiter_rate + @rate_limiter_rate ||= rate_limiter_rate_tag + end + + def resource + @resource ||= resource_tag.tap { |v| break (v.frozen? ? v : v.dup) } + end + + def rule_sample_rate + @rule_sample_rate ||= rule_sample_rate_tag + end + + def runtime_id + @runtime_id ||= runtime_id_tag + end + + def sample_rate + @sample_rate ||= sample_rate_tag + end + + def sampling_priority + @sampling_priority ||= sampling_priority_tag + end + + def service + @service ||= service_tag.tap { |v| break (v.frozen? ? v : v.dup) } + end + def any? @spans.any? end @@ -152,7 +189,6 @@ def sampled? attr_writer \ :agent_sample_rate, :hostname, - :id, :lang, :name, :origin, From b70c4017770fc320f3922866ecf3b25b48cccb2c Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Thu, 14 Apr 2022 17:48:40 +0200 Subject: [PATCH 20/21] Simplify TraceSegment initializer The previous attempt had the issue of always falling back to the RHS of or-assignments when the values are `nil`. --- lib/datadog/core/utils/safe_dup.rb | 27 +++++++ lib/datadog/tracing/trace_segment.rb | 109 ++++++++------------------- 2 files changed, 57 insertions(+), 79 deletions(-) create mode 100644 lib/datadog/core/utils/safe_dup.rb diff --git a/lib/datadog/core/utils/safe_dup.rb b/lib/datadog/core/utils/safe_dup.rb new file mode 100644 index 00000000000..13ab86a77fb --- /dev/null +++ b/lib/datadog/core/utils/safe_dup.rb @@ -0,0 +1,27 @@ +# typed: false + +module Datadog + module Core + module Utils + # Helper methods for safer dup + module SafeDup + if RUBY_VERSION < '2.2' # nil.dup only fails in Ruby 2.1 + # Ensures #initialize can call nil.dup safely + module RefineNil + refine NilClass do + def dup + self + end + end + end + + using RefineNil + end + + def self.frozen_or_dup(v) + v.frozen? ? v : v.dup + end + end + end + end +end diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index 56e79ab9764..d4c4133b523 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -1,6 +1,7 @@ # typed: true require 'datadog/core/runtime/ext' +require 'datadog/core/utils/safe_dup' require 'datadog/tracing/sampling/ext' require 'datadog/tracing/metadata/ext' @@ -18,20 +19,20 @@ class TraceSegment attr_reader \ :id, - :spans - - if RUBY_VERSION < '2.2' # nil.dup only fails in Ruby 2.1 - # Ensures #initialize can call nil.dup safely - module RefineNil - refine NilClass do - def dup - self - end - end - end - - using RefineNil - end + :spans, + :agent_sample_rate, + :hostname, + :lang, + :name, + :origin, + :process_id, + :rate_limiter_rate, + :resource, + :rule_sample_rate, + :runtime_id, + :sample_rate, + :sampling_priority, + :service # rubocop:disable Metrics/CyclomaticComplexity # rubocop:disable Metrics/PerceivedComplexity @@ -65,75 +66,25 @@ def initialize( @metrics = metrics || {} # Set well-known tags, defaulting to getting the values from tags - @agent_sample_rate = agent_sample_rate - @hostname = hostname - @lang = lang - @name = name.tap { |v| break (v.frozen? ? v : v.dup) } - @origin = origin.tap { |v| break (v.frozen? ? v : v.dup) } - @process_id = process_id - @rate_limiter_rate = rate_limiter_rate - @resource = resource.tap { |v| break (v.frozen? ? v : v.dup) } - @rule_sample_rate = rule_sample_rate - @runtime_id = runtime_id - @sample_rate = sample_rate - @sampling_priority = sampling_priority - @service = service.tap { |v| break (v.frozen? ? v : v.dup) } + @agent_sample_rate = agent_sample_rate || agent_sample_rate_tag + @hostname = hostname || hostname_tag + @lang = lang || lang_tag + @name = Core::Utils::SafeDup.frozen_or_dup(name || name_tag) + @origin = Core::Utils::SafeDup.frozen_or_dup(origin || origin_tag) + @process_id = process_id || process_id_tag + @rate_limiter_rate = rate_limiter_rate || rate_limiter_rate_tag + @resource = Core::Utils::SafeDup.frozen_or_dup(resource || resource_tag) + @rule_sample_rate = rule_sample_rate_tag || rule_sample_rate + @runtime_id = runtime_id || runtime_id_tag + @sample_rate = sample_rate || sample_rate_tag + @sampling_priority = sampling_priority || sampling_priority_tag + @service = Core::Utils::SafeDup.frozen_or_dup(service || service_tag) + + clear_known_tags end # rubocop:enable Metrics/PerceivedComplexity # rubocop:enable Metrics/CyclomaticComplexity - def agent_sample_rate - @agent_sample_rate ||= agent_sample_rate_tag - end - - def hostname - @hostname ||= hostname_tag - end - - def lang - @lang ||= lang_tag - end - - def name - @name ||= name_tag.tap { |v| break (v.frozen? ? v : v.dup) } - end - - def origin - @origin ||= origin_tag.tap { |v| break (v.frozen? ? v : v.dup) } - end - - def process_id - @process_id ||= process_id_tag - end - - def rate_limiter_rate - @rate_limiter_rate ||= rate_limiter_rate_tag - end - - def resource - @resource ||= resource_tag.tap { |v| break (v.frozen? ? v : v.dup) } - end - - def rule_sample_rate - @rule_sample_rate ||= rule_sample_rate_tag - end - - def runtime_id - @runtime_id ||= runtime_id_tag - end - - def sample_rate - @sample_rate ||= sample_rate_tag - end - - def sampling_priority - @sampling_priority ||= sampling_priority_tag - end - - def service - @service ||= service_tag.tap { |v| break (v.frozen? ? v : v.dup) } - end - def any? @spans.any? end From 1ee09fe0462ac549ec80f665b2985ad97a15c785 Mon Sep 17 00:00:00 2001 From: Loic Nageleisen Date: Thu, 14 Apr 2022 17:51:12 +0200 Subject: [PATCH 21/21] Keep known tags in TraceSegment Even though they are not used anymore after initialization, they are also harmless since they're publicly invisible. --- lib/datadog/tracing/trace_segment.rb | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/lib/datadog/tracing/trace_segment.rb b/lib/datadog/tracing/trace_segment.rb index d4c4133b523..a07fbdf1546 100644 --- a/lib/datadog/tracing/trace_segment.rb +++ b/lib/datadog/tracing/trace_segment.rb @@ -79,8 +79,6 @@ def initialize( @sample_rate = sample_rate || sample_rate_tag @sampling_priority = sampling_priority || sampling_priority_tag @service = Core::Utils::SafeDup.frozen_or_dup(service || service_tag) - - clear_known_tags end # rubocop:enable Metrics/PerceivedComplexity # rubocop:enable Metrics/CyclomaticComplexity @@ -203,22 +201,6 @@ def sampling_priority_tag def service_tag meta[TAG_SERVICE] end - - def clear_known_tags - metrics.delete(Metadata::Ext::Sampling::TAG_AGENT_RATE) - meta.delete(Metadata::Ext::NET::TAG_HOSTNAME) - meta.delete(Core::Runtime::Ext::TAG_LANG) - meta.delete(TAG_NAME) - meta.delete(Metadata::Ext::Distributed::TAG_ORIGIN) - meta.delete(Core::Runtime::Ext::TAG_PID) - metrics.delete(Metadata::Ext::Sampling::TAG_RATE_LIMITER_RATE) - meta.delete(TAG_RESOURCE) - metrics.delete(Metadata::Ext::Sampling::TAG_RULE_SAMPLE_RATE) - meta.delete(Core::Runtime::Ext::TAG_ID) - metrics.delete(Metadata::Ext::Sampling::TAG_SAMPLE_RATE) - meta.delete(Metadata::Ext::Distributed::TAG_SAMPLING_PRIORITY) - meta.delete(TAG_SERVICE) - end end # rubocop:enable Metrics/ClassLength end