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.

Sign up for GitHub

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

feat(otel): add support for span links #3572

Merged
merged 13 commits into from
May 3, 2024
Merged

feat(otel): add support for span links #3572

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f6ff8d6
feat(otel): add support for span links
mabdinur Apr 4, 2024
d2e8aab
fix import
mabdinur Apr 9, 2024
5c386ab
fix links test
mabdinur Apr 9, 2024
1abf37f
update span operation
mabdinur Apr 15, 2024
30b29a8
use map instead of each to return dd link objects
mabdinur Apr 15, 2024
374d5e9
add span op test
mabdinur Apr 16, 2024
7e9fe1e
ensure link ids have the correct type
mabdinur Apr 19, 2024
59e9c5c
fix trace_id packing and types
mabdinur Apr 23, 2024
da32ee5
reduce diff
mabdinur Apr 24, 2024
dbca88c
fix tests and address comments
mabdinur Apr 25, 2024
76ad69b
Merge branch 'master' into munir/add-links-to-otel-api
marcotc May 1, 2024
656475e
Update gemfiles/*
marcotc May 1, 2024
683dd7b
remove gemfile changes, sync changes with main, fix tests
mabdinur May 3, 2024
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
17 changes: 17 additions & 0 deletions lib/datadog/opentelemetry/sdk/span_processor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# frozen_string_literal: true

require_relative 'trace/span'
require_relative '../../tracing/span_link'
require_relative '../../tracing/trace_digest'

module Datadog
module OpenTelemetry
Expand Down Expand Up @@ -87,6 +89,21 @@ def start_datadog_span(span)
datadog_span.set_error([nil, span.status.description]) unless span.status.ok?
datadog_span.set_tags(span.attributes)

unless span.links.nil?
datadog_span.links = span.links.map do |link|
Datadog::Tracing::SpanLink.new(
mabdinur marked this conversation as resolved.
Show resolved Hide resolved
Datadog::Tracing::TraceDigest.new(
trace_id: link.span_context.trace_id.unpack1('Q'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
trace_id: link.span_context.trace_id.unpack1('Q'),
trace_id: to_datadog_id(link.span_context.trace_id),

Can you wrap unpack1('Q') into a method, so that we know what it does in the future?

For example, this is how we do the inverse operation:

dd-trace-rb/lib/datadog/opentelemetry/sdk/propagator.rb

Lines 84 to 88 in 53953ac

# 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this code to use hex_trace_id and hex_span_id field on the otel SpanContext. This way we ensure the otel library encodes/decodes bytes in a consistent manner (I believe they use Array.pack('H*') for trace ids and Array.pack('Q') for span ids).

span_id: link.span_context.span_id.unpack1('Q'),
trace_flags: (link.span_context.trace_flags&.sampled? ? 1 : 0),
trace_state: link.span_context.tracestate&.to_s,
span_remote: link.span_context.remote?,
),
attributes: link.attributes
)
end
end

datadog_span
end

Expand Down
17 changes: 6 additions & 11 deletions lib/datadog/tracing/span_operation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,8 @@ class SpanOperation

# Span attributes
# NOTE: In the future, we should drop the me
attr_reader \
:end_time,
:id,
:name,
:parent_id,
:resource,
:service,
:start_time,
:trace_id,
:type
attr_accessor :links
attr_reader :end_time, :id, :name, :parent_id, :resource, :service, :start_time, :trace_id, :type
mabdinur marked this conversation as resolved.
Show resolved Hide resolved

attr_accessor \
:status
Expand All @@ -49,7 +41,8 @@ def initialize(
start_time: nil,
tags: nil,
trace_id: nil,
type: nil
type: nil,
links: nil
mabdinur marked this conversation as resolved.
Show resolved Hide resolved
)
# Ensure dynamically created strings are UTF-8 encoded.
#
Expand All @@ -66,6 +59,7 @@ def initialize(
@trace_id = trace_id || Tracing::Utils::TraceId.next_id

@status = 0
@links = links || []

# start_time and end_time track wall clock. In Ruby, wall clock
# has less accuracy than monotonic clock, so if possible we look to only use wall clock
Expand Down Expand Up @@ -452,6 +446,7 @@ def build_span
status: @status,
type: @type,
trace_id: @trace_id,
links: @links,
service_entry: parent.nil? || (service && parent.service != service)
)
end
Expand Down
4 changes: 3 additions & 1 deletion sig/datadog/tracing/span_operation.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ module Datadog
include Metadata::Errors
prepend Metadata::Analytics

attr_reader links: untyped

attr_reader end_time: untyped

attr_reader id: untyped
Expand All @@ -26,7 +28,7 @@ module Datadog

attr_accessor status: untyped

def initialize: (untyped name, ?child_of: untyped?, ?events: untyped?, ?on_error: untyped?, ?parent_id: ::Integer, ?resource: untyped, ?service: untyped?, ?start_time: untyped?, ?tags: untyped?, ?trace_id: untyped?, ?type: untyped?) -> void
def initialize: (untyped name, ?child_of: untyped?, ?events: untyped?, ?on_error: untyped?, ?parent_id: ::Integer, ?resource: untyped, ?service: untyped?, ?start_time: untyped?, ?tags: untyped?, ?trace_id: untyped?, ?type: untyped?, ?links: untyped?) -> void

def name=: (untyped name) -> untyped

Expand Down
36 changes: 36 additions & 0 deletions spec/datadog/opentelemetry_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,42 @@
expect(span.end_time).to eq(timestamp)
end
end

context 'with span_links' do
let(:sc1) { OpenTelemetry::Trace::SpanContext.new(trace_id: 36893488147419103231, span_id: 2) }
let(:sc2) do
OpenTelemetry::Trace::SpanContext.new(
trace_id: [1234534].pack('Q'),
span_id: [67890].pack('Q'),
trace_flags: OpenTelemetry::Trace::TraceFlags::SAMPLED,
tracestate: OpenTelemetry::Trace::Tracestate.from_string('otel=blahxd')
)
end
let(:links) do
[
OpenTelemetry::Trace::Link.new(sc1, { 'key' => 'val', '1' => true }),
OpenTelemetry::Trace::Link.new(sc2, { 'key2' => true, 'list' => [1, 2] }),
]
end
let(:options) { { links: links } }

it 'sets span links' do
start_span.finish
expect(span.links.size).to eq(2)

expect(span.links[0].trace_id).to eq(sc1.trace_id)
expect(span.links[0].span_id).to eq(sc1.span_id)
expect(span.links[0].trace_flags).to eq(0)
expect(span.links[0].trace_state).to eq('')
expect(span.links[0].attributes).to eq({ 'key' => 'val', '1' => true })

expect(span.links[1].trace_id).to eq(sc2.trace_id)
expect(span.links[1].span_id).to eq(sc2.span_id)
expect(span.links[1].trace_flags).to eq(1)
expect(span.links[1].trace_state).to eq(sc2.tracestate.to_s)
expect(span.links[1].attributes).to eq({ 'key2' => true, 'list' => [1, 2] })
end
end
end

describe '#start_root_span' do
Expand Down
3 changes: 2 additions & 1 deletion spec/datadog/tracing/span_operation_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@
start_time: nil,
status: 0,
trace_id: kind_of(Integer),
type: nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to add a test to the given an option group for links:

dd-trace-rb/spec/datadog/tracing/span_operation_spec.rb

Lines 103 to 289 in 7e9fe1e

context 'given an option' do
shared_examples 'a string property' do |nillable: true|
let(:options) { { property => value } }
context 'set to a String' do
let(:value) { 'test string' }
it { is_expected.to have_attributes(property => value) }
end
context 'set to a non-UTF-8 String' do
let(:value) { 'ascii'.encode(Encoding::ASCII) }
it { is_expected.to have_attributes(property => value) }
it { expect(span_op.public_send(property).encoding).to eq(Encoding::UTF_8) }
end
context 'invoking the public setter' do
subject! { span_op.public_send("#{property}=", value) }
context 'with a string' do
let(:value) { 'test string' }
it { expect(span_op).to have_attributes(property => value) }
end
context 'with a string that is not in UTF-8' do
let(:value) { 'ascii'.encode(Encoding::ASCII) }
it { expect(span_op).to have_attributes(property => value) }
it { expect(span_op.public_send(property).encoding).to eq(Encoding::UTF_8) }
end
end
if nillable
context 'set to nil' do
let(:value) { nil }
# Allow property to be explicitly set to nil
it { is_expected.to have_attributes(property => nil) }
end
else
context 'set to nil' do
let(:value) { nil }
it { expect { subject }.to raise_error(ArgumentError) }
end
end
end
context ':on_error' do
let(:options) { { on_error: block } }
let(:block) { proc { raise error } }
let(:error) { error_class.new('error message') }
let(:error_class) { stub_const('TestError', Class.new(StandardError)) }
context 'that is nil' do
let(:on_error) { nil }
context 'and #measure raises an error' do
subject(:measure) { span_op.measure { raise error } }
before { allow(span_op).to receive(:set_error) }
it 'propagates the error' do
expect { measure }.to raise_error(error)
expect(span_op).to have_received(:set_error).with(error)
end
end
end
context 'that is a block' do
let(:on_error) { block }
it 'yields to the error block and raises the error' do
expect do
expect do |b|
options[:on_error] = b.to_proc
span_op.measure(&block)
end.to yield_with_args(
a_kind_of(described_class),
error
)
end.to raise_error(error)
# It should not set an error, as this overrides behavior.
expect(span_op).to_not have_error
end
end
context 'that is not a Proc' do
let(:on_error) { 'not a proc' }
it 'fallbacks to default error handler and log a debug message' do
expect(Datadog.logger).to receive(:debug).at_least(:once)
expect do
span_op.measure(&block)
end.to raise_error(error)
end
end
end
describe ':name' do
it_behaves_like 'a string property', nillable: false do
let(:property) { :name }
# :name is not a keyword argument, but positional.
# We swap those two here.
let(:options) { {} }
let(:name) { value }
end
end
describe ':parent_id' do
let(:options) { { parent_id: parent_id } }
context 'that is nil' do
let(:parent_id) { nil }
it { is_expected.to have_attributes(parent_id: 0) }
end
context 'that is an Integer' do
let(:parent_id) { instance_double(Integer) }
it { is_expected.to have_attributes(parent_id: parent_id) }
end
end
describe ':resource' do
it_behaves_like 'a string property' do
let(:property) { :resource }
end
end
describe ':service' do
it_behaves_like 'a string property' do
let(:property) { :service }
end
end
describe ':start_time' do
let(:options) { { start_time: start_time } }
let(:start_time) { instance_double(Time) }
context 'that is nil' do
let(:start_time) { nil }
it { is_expected.to have_attributes(start_time: nil) }
end
context 'that is a Time' do
let(:start_time) { instance_double(Time) }
it { is_expected.to have_attributes(start_time: start_time) }
end
end
describe ':tags' do
let(:options) { { tags: tags } }
context 'that is nil' do
let(:tags) { nil }
it_behaves_like 'a root span operation'
end
context 'that is a Hash' do
let(:tags) { { 'custom_tag' => 'custom_value' } }
it_behaves_like 'a root span operation'
it { expect(span_op.get_tag('custom_tag')).to eq(tags['custom_tag']) }
end
end
describe ':trace_id' do
let(:options) { { trace_id: trace_id } }
context 'that is nil' do
let(:trace_id) { nil }
it { is_expected.to have_attributes(trace_id: kind_of(Integer)) }
end
context 'that is an Integer' do
let(:trace_id) { Datadog::Tracing::Utils::TraceId.next_id }
it { is_expected.to have_attributes(trace_id: trace_id) }
end
end
describe ':type' do
it_behaves_like 'a string property' do
let(:property) { :type }
end
end
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added test coverage for the links field in span_operation

type: nil,
links: []
)
end

Expand Down
Loading