Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ensure the correct ids are serialized and propagated #3709

Merged
merged 8 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 5 additions & 10 deletions lib/datadog/opentelemetry/sdk/propagator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,11 @@ def extract(
digest = @datadog_propagator.extract(carrier)
return context unless digest

trace_id = to_otel_id(digest.trace_id)
span_id = to_otel_id(digest.span_id)
# Converts the {Numeric} Datadog id object to OpenTelemetry's byte array format.
# 128-bit unsigned, big-endian integer
trace_id = [digest.trace_id >> 64, digest.trace_id & 0xFFFFFFFFFFFFFFFF].pack('Q>Q>')
# 64-bit unsigned, big-endian integer
span_id = [digest.span_id].pack('Q>')

if digest.trace_state || digest.trace_flags
trace_flags = ::OpenTelemetry::Trace::TraceFlags.from_byte(digest.trace_flags)
Expand Down Expand Up @@ -78,14 +81,6 @@ def extract(
def fields
[]
end

private

# Converts the {Numeric} Datadog id object to OpenTelemetry's byte array format.
# This method currently converts an unsigned 64-bit Integer to a binary String.
def to_otel_id(dd_id)
Array(dd_id).pack('Q')
end
end
end
end
Expand Down
7 changes: 5 additions & 2 deletions lib/datadog/opentelemetry/sdk/span_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ def create_matching_datadog_span(span, parent_context)
if parent_context.trace
Tracing.send(:tracer).send(:call_context).activate!(parent_context.ensure_trace)
else
Tracing.continue_trace!(nil)
otel_trace_id = span.context.hex_trace_id.to_i(16)
Tracing.continue_trace!(Datadog::Tracing::TraceDigest.new(trace_id: otel_trace_id, span_remote: false))
end

datadog_span = start_datadog_span(span)
Expand All @@ -85,7 +86,6 @@ def start_datadog_span(span)
name, kwargs = span_arguments(span, attributes)

datadog_span = Tracing.trace(name, **kwargs)

datadog_span.set_error([nil, span.status.description]) unless span.status.ok?
datadog_span.set_tags(span.attributes)

Expand Down Expand Up @@ -143,6 +143,9 @@ def span_arguments(span, attributes)

kwargs[:tags] = attributes.to_h

# DEV: The datadog span must have the same ID as the OpenTelemetry span
kwargs[:id] = span.context.hex_span_id.to_i(16)

[name, kwargs]
end

Expand Down
2 changes: 2 additions & 0 deletions lib/datadog/tracing.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ def trace(
start_time: nil,
tags: nil,
type: nil,
id: nil,
&block
)

Expand All @@ -35,6 +36,7 @@ def trace(
start_time: start_time,
tags: tags,
type: type,
id: id,
&block
)
end
Expand Down
5 changes: 3 additions & 2 deletions lib/datadog/tracing/span_operation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ def initialize(
tags: nil,
trace_id: nil,
type: nil,
links: nil
links: nil,
id: nil
)
# Ensure dynamically created strings are UTF-8 encoded.
#
Expand All @@ -60,7 +61,7 @@ def initialize(
self.type = type
self.resource = resource

@id = Tracing::Utils.next_id
@id = id.nil? ? Tracing::Utils.next_id : id
@parent_id = parent_id || 0
@trace_id = trace_id || Tracing::Utils::TraceId.next_id

Expand Down
10 changes: 7 additions & 3 deletions lib/datadog/tracing/trace_operation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ def measure(
start_time: nil,
tags: nil,
type: nil,
id: nil,
&block
)
# Don't allow more span measurements if the
Expand All @@ -197,7 +198,8 @@ def measure(
service: service,
start_time: start_time,
tags: tags,
type: type
type: type,
id: id
)

# Start span measurement
Expand All @@ -212,7 +214,8 @@ def build_span(
service: nil,
start_time: nil,
tags: nil,
type: nil
type: nil,
id: nil
)
begin
# Resolve span options:
Expand Down Expand Up @@ -249,7 +252,8 @@ def build_span(
start_time: start_time,
tags: tags,
trace_id: trace_id,
type: type
type: type,
id: id
)
rescue StandardError => e
Datadog.logger.debug { "Failed to build new span: #{e}" }
Expand Down
11 changes: 9 additions & 2 deletions lib/datadog/tracing/tracer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ def initialize(
# @param [Time] start_time time which the span should have started.
# @param [Hash<String,String>] tags extra tags which should be added to the span.
# @param [String] type the type of the span. See {Datadog::Tracing::Metadata::Ext::AppTypes}.
# @param [Integer] the id of the new span.
mabdinur marked this conversation as resolved.
Show resolved Hide resolved
# @return [Object] If a block is provided, returns the result of the block execution.
# @return [Datadog::Tracing::SpanOperation] If no block is provided, returns the active,
# unfinished {Datadog::Tracing::SpanOperation}.
Expand All @@ -130,6 +131,7 @@ def trace(
start_time: nil,
tags: nil,
type: nil,
id: nil,
&block
)
return skip_trace(name, &block) unless enabled
Expand Down Expand Up @@ -162,6 +164,7 @@ def trace(
tags: tags,
type: type,
_trace: trace,
id: id,
&block
)
end
Expand All @@ -178,7 +181,8 @@ def trace(
start_time: start_time,
tags: tags,
type: type,
_trace: trace
_trace: trace,
id: id
)
end
end
Expand Down Expand Up @@ -375,6 +379,7 @@ def start_span(
tags: nil,
type: nil,
_trace: nil,
id: nil,
&block
)
trace = _trace || start_trace(continue_from: continue_from)
Expand All @@ -391,6 +396,7 @@ def start_span(
service: service,
tags: resolve_tags(tags),
type: type,
id: id,
&block
)
else
Expand All @@ -403,7 +409,8 @@ def start_span(
service: service,
start_time: start_time,
tags: resolve_tags(tags),
type: type
type: type,
id: id
)

span.start(start_time)
Expand Down
21 changes: 17 additions & 4 deletions spec/datadog/opentelemetry_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,17 @@
expect(parent).to be_root_span
expect(child.parent_id).to eq(parent.id)
end

it 'the underlying datadog spans has the same ids as the otel spans' do
existing_span.finish
start_span.finish
# Verify Span IDs are the same
expect(existing_span.context.hex_span_id.to_i(16)).to eq(parent.id)
expect(start_span.context.hex_span_id.to_i(16)).to eq(child.id)
# Verify Trace IDs are the same
expect(existing_span.context.hex_trace_id.to_i(16)).to eq(parent.trace_id)
expect(start_span.context.hex_trace_id.to_i(16)).to eq(child.trace_id)
end
end
end

Expand Down Expand Up @@ -698,12 +709,14 @@
::OpenTelemetry.propagation.inject(carrier)
end
let(:carrier) { {} }
let(:trace_id) { Datadog::Tracing.active_trace.id }
def headers
{
'x-datadog-parent-id' => Datadog::Tracing.active_span.id.to_s,
'x-datadog-sampling-priority' => '1',
'x-datadog-tags' => '_dd.p.dm=-0,_dd.p.tid=' +
high_order_hex_trace_id(Datadog::Tracing.active_trace.id),
'x-datadog-tags' => '_dd.p.dm=-0' + (
trace_id < 2**64 ? '' : ",_dd.p.tid=#{high_order_hex_trace_id(Datadog::Tracing.active_trace.id)}"
),
'x-datadog-trace-id' => low_order_trace_id(Datadog::Tracing.active_trace.id).to_s,
}
end
Expand Down Expand Up @@ -802,7 +815,7 @@ def headers
context 'with TraceContext headers' do
let(:carrier) do
{
'traceparent' => '00-00000000000000001111111111111111-2222222222222222-01'
'traceparent' => '00-11111111111111111111111111111111-2222222222222222-01'
}
end

Expand All @@ -817,7 +830,7 @@ def headers
otel_tracer.in_span('otel') {}
end

expect(span.trace_id).to eq(0x00000000000000001111111111111111)
expect(span.trace_id).to eq(0x11111111111111111111111111111111)
expect(span.parent_id).to eq(0x2222222222222222)
end
end
Expand Down
16 changes: 12 additions & 4 deletions spec/datadog/profiling/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,8 @@ def samples_from_pprof(pprof_data)
sample.location_id.map { |location_id| decode_frame_from_pprof(decoded_profile, location_id) },
pretty_sample_types.zip(sample.value).to_h,
sample.label.map do |it|
[
string_table[it.key].to_sym,
it.num == 0 ? string_table[it.str] : it.num,
]
key = string_table[it.key].to_sym
[key, (it.num == 0 ? string_table[it.str] : ProfileHelpers.maybe_fix_label_range(key, it.num))]
end.to_h,
).freeze
end
Expand Down Expand Up @@ -103,6 +101,16 @@ def build_stack_recorder(
timeline_enabled: timeline_enabled,
)
end

def self.maybe_fix_label_range(key, value)
if [:'local root span id', :'span id'].include?(key) && value < 0
# pprof labels are defined to be decoded as signed values BUT the backend explicitly interprets these as unsigned
# 64-bit numbers so we can still use them for these ids without having to fall back to strings
value + 2**64
else
value
end
end
end

RSpec.configure do |config|
Expand Down
14 changes: 14 additions & 0 deletions spec/datadog/tracing/span_operation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,20 @@
end
end

describe ':id' do
let(:options) { { id: id } }

context 'that is nil' do
let(:id) { nil }
it { is_expected.to have_attributes(id: kind_of(Integer)) }
end

context 'that is an Integer' do
let(:id) { instance_double(Integer) }
it { is_expected.to have_attributes(id: id) }
end
end

describe ':resource' do
it_behaves_like 'a string property' do
let(:property) { :resource }
Expand Down
5 changes: 4 additions & 1 deletion spec/datadog/tracing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
start_time: start_time,
tags: tags,
type: type,
id: id,
&block
)
end
Expand All @@ -81,6 +82,7 @@
let(:start_time) { double('start_time') }
let(:tags) { double('tags') }
let(:type) { double('type') }
let(:id) { double(1) }
let(:block) { -> {} }

it 'delegates to the tracer' do
Expand All @@ -93,7 +95,8 @@
service: service,
start_time: start_time,
tags: tags,
type: type
type: type,
id: id
) { |&b| expect(b).to be(block) }
.and_return(returned)
expect(trace).to eq(returned)
Expand Down
Loading