Skip to content

Commit

Permalink
[core] Fix parsing of distributed tracing headers (#719)
Browse files Browse the repository at this point in the history 
* [core] Fix parsing of distributed tracing headers

* make sampling priority parsing a little more robust

* Use #to_hash instead of #to_h

* Move Ext::DistributedTracing::MAX_ID to Span::EXTERNAL_MAX_ID

* Fixed missing reference

* add missing underscore

* Fix broken tests
  • Loading branch information
@brettlangdon
brettlangdon authored Mar 20, 2019
1 parent f9279e9 commit a06d40f
Show file tree
Hide file tree
Showing 4 changed files with 257 additions and 126 deletions.
18 changes: 14 additions & 4 deletions lib/ddtrace/propagation/distributed_headers.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ def valid?
return true if origin == 'synthetics' && trace_id

# Sampling priority and origin are optional.
trace_id && parent_id
# DEV: We want to explicitly return true/false here
trace_id && parent_id ? true : false
end

def trace_id
Expand All @@ -29,11 +30,19 @@ def parent_id

def sampling_priority
hdr = header(HTTP_HEADER_SAMPLING_PRIORITY)

# It's important to make a difference between no header,
# and a header defined to zero.
return unless hdr
return if hdr.nil?

# Convert header to an integer
value = hdr.to_i
return if value < 0

# Ensure the parsed number is the same as the original string value
# e.g. We want to make sure to throw away `'nan'.to_i == 0`
# DEV: Ruby `.to_i` will return `0` if a number could not be parsed
return unless value.to_s == hdr.to_s

value
end

Expand All @@ -53,7 +62,8 @@ def header(name)

def id(header)
value = header(header).to_i
return if value.zero? || value >= Span::MAX_ID
# Zero or greater than max allowed value of 2**64
return if value.zero? || value > Span::EXTERNAL_MAX_ID
value < 0 ? value + 0x1_0000_0000_0000_0000 : value
end
end
Expand Down
4 changes: 4 additions & 0 deletions lib/ddtrace/span.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ class Span
# and IDs need to be easy to port across various languages and platforms.
MAX_ID = 2**63

# While we only generate 63-bit integers due to limitations in other languages, we support
# parsing 64-bit integers for distributed tracing since an upstream system may generate one
EXTERNAL_MAX_ID = 2**64

attr_accessor :name, :service, :resource, :span_type,
:start_time, :end_time,
:span_id, :trace_id, :parent_id,
Expand Down
239 changes: 239 additions & 0 deletions spec/ddtrace/propagation/distributed_headers_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,243 @@ def env_header(name)
end
end
end

describe '#trace_id' do
context 'no trace_id header' do
it { expect(headers.trace_id).to be_nil }
end

context 'incorrect header' do
[
'X-DATADOG-TRACE-ID-TYPO',
'X-DATDOG-TRACE-ID',
'X-TRACE-ID',
'TRACE-ID'
].each do |header|
# '100' is a valid value
let(:env) { { env_header(header) => '100' } }

it { expect(headers.trace_id).to be_nil }
end
end

context 'trace_id in header' do
[
['123', 123],
['0', nil],
['a', nil],
['-1', 18446744073709551615],
['-8809075535603237910', 9637668538106313706],
['ooops', nil],

# Boundaries of what we generate
[Datadog::Span::MAX_ID.to_s, Datadog::Span::MAX_ID],
[(Datadog::Span::MAX_ID + 1).to_s, Datadog::Span::MAX_ID + 1],

# Max allowed values
[Datadog::Span::EXTERNAL_MAX_ID.to_s, Datadog::Span::EXTERNAL_MAX_ID],
[(Datadog::Span::EXTERNAL_MAX_ID + 1).to_s, nil]
].each do |value, expected|
context "set to #{value}" do
let(:env) { { env_header(Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID) => value } }

it { expect(headers.trace_id).to eq(expected) }
end
end
end
end

describe '#parent_id' do
context 'no parent_id header' do
it { expect(headers.parent_id).to be_nil }
end

context 'incorrect header' do
[
'X-DATADOG-PARENT-ID-TYPO',
'X-DATDOG-PARENT-ID',
'X-PARENT-ID',
'PARENT-ID'
].each do |header|
# '100' is a valid value
let(:env) { { env_header(header) => '100' } }

it { expect(headers.parent_id).to be_nil }
end
end

context 'parent_id in header' do
[
['123', 123],
['0', nil],
['a', nil],
['-1', 18446744073709551615],
['-8809075535603237910', 9637668538106313706],
['ooops', nil],

# Boundaries of what we generate
[Datadog::Span::MAX_ID.to_s, Datadog::Span::MAX_ID],
[(Datadog::Span::MAX_ID + 1).to_s, Datadog::Span::MAX_ID + 1],

# Max allowed values
[Datadog::Span::EXTERNAL_MAX_ID.to_s, Datadog::Span::EXTERNAL_MAX_ID],
[(Datadog::Span::EXTERNAL_MAX_ID + 1).to_s, nil]
].each do |value, expected|
context "set to #{value}" do
let(:env) { { env_header(Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID) => value } }

it { expect(headers.parent_id).to eq(expected) }
end
end
end
end

describe '#sampling_priority' do
context 'no sampling priorityheader' do
it { expect(headers.sampling_priority).to be_nil }
end

context 'incorrect header' do
[
'X-DATADOG-SAMPLING-PRIORITY-TYPO',
'X-DATDOG-SAMPLING-PRIORITY',
'X-SAMPLING-PRIORITY',
'SAMPLING-PRIORITY'
].each do |header|
# '100' is a valid value
let(:env) { { env_header(header) => '100' } }

it { expect(headers.sampling_priority).to be_nil }
end
end

context 'sampling_priority in header' do
[
# Allowed values
['-1', -1],
['0', 0],
['1', 1],
['2', 2],

# Outside of bounds, but still allowed since a number
['-2', -2],
['3', 3],
['999', 999],

# Not a number
['ooops', nil]
].each do |value, expected|
context "set to #{value}" do
let(:env) { { env_header(Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY) => value } }

it { expect(headers.sampling_priority).to eq(expected) }
end
end
end
end

describe '#valid?' do
context 'with headers' do
[
# Trace id and Parent id with no other headers - valid
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '456' },
true
],

# All acceptable values for sampling priority - valid
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '456',
Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY => '-1' },
true
],
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '456',
Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY => '0' },
true
],
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '456',
Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY => '1' },
true
],
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '456',
Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY => '2' },
true
],

# Invalid Trace id - invalid
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => 'a',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '456',
Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY => '0' },
false
],

# Invalid Parent id - invalid
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => 'a',
Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY => '0' },
false
],

# Invalid sampling priority - valid
# DEV: This is valid because sampling priority isn't required
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '456',
Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY => 'nan' },
true
],

# Trace id and Parent id both 0 - invalid
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '0',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '0',
Datadog::Ext::DistributedTracing::HTTP_HEADER_SAMPLING_PRIORITY => '0' },
false
],

# Typos in header names - invalid
[
{ 'X-DATADOG-TRACE-ID-TYPO' => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '456' },
false
],
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
'X-DATADOG-PARENT-ID-TYPO' => '456' },
false
],

# Parent id is not required when origin is 'synthetics' - valid
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '0',
Datadog::Ext::DistributedTracing::HTTP_HEADER_ORIGIN => 'synthetics' },
true
],
# Invalid when not 'synthetics' - invalid
[
{ Datadog::Ext::DistributedTracing::HTTP_HEADER_TRACE_ID => '123',
Datadog::Ext::DistributedTracing::HTTP_HEADER_PARENT_ID => '0',
Datadog::Ext::DistributedTracing::HTTP_HEADER_ORIGIN => 'not-synthetics' },
false
]
].each do |test_headers, expected|
context test_headers.to_s do
let(:env) { Hash[test_headers.map { |k, v| [env_header(k), v] }] }

it { expect(headers.valid?).to eq(expected) }
end
end
end
end
end
Loading

0 comments on commit a06d40f

Please sign in to comment.