Skip to content

Commit

Permalink
Move partial trace tagging condition to TraceFormatter
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lloeki committed Apr 12, 2022
1 parent 90614bb commit 92ee73c
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 4 deletions.
2 changes: 1 addition & 1 deletion lib/datadog/tracing/trace_operation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 7 additions & 0 deletions lib/ddtrace/transport/trace_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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, distributed tracing)
root_span || trace.spans.last
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/datadog/tracing/trace_operation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 22 additions & 2 deletions spec/ddtrace/transport/trace_formatter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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

Expand All @@ -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
Expand Down

0 comments on commit 92ee73c

Please sign in to comment.