diff --git a/lib/ddtrace/propagation/distributed_headers.rb b/lib/ddtrace/propagation/distributed_headers.rb index d29e0653ff..0503746f4d 100644 --- a/lib/ddtrace/propagation/distributed_headers.rb +++ b/lib/ddtrace/propagation/distributed_headers.rb @@ -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 @@ -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 @@ -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 diff --git a/lib/ddtrace/span.rb b/lib/ddtrace/span.rb index b175895311..82cdabd3cd 100644 --- a/lib/ddtrace/span.rb +++ b/lib/ddtrace/span.rb @@ -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, diff --git a/spec/ddtrace/propagation/distributed_headers_spec.rb b/spec/ddtrace/propagation/distributed_headers_spec.rb index 56db985ee8..279650ce8f 100644 --- a/spec/ddtrace/propagation/distributed_headers_spec.rb +++ b/spec/ddtrace/propagation/distributed_headers_spec.rb @@ -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 diff --git a/test/propagation/distributed_headers_test.rb b/test/propagation/distributed_headers_test.rb deleted file mode 100644 index 18bf7f4a4c..0000000000 --- a/test/propagation/distributed_headers_test.rb +++ /dev/null @@ -1,122 +0,0 @@ -require 'helper' -require 'ddtrace/tracer' -require 'ddtrace/span' -require 'ddtrace/propagation/distributed_headers' - -class DistributedHeadersTest < Minitest::Test - def test_valid_without_sampling_priority # rubocop:disable Metrics/MethodLength - test_cases = { - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => '456' } => true, - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => '456', - 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '0' } => true, - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => '456', - 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '1' } => true, - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => '456', - 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '999' } => true, - { 'HTTP_X_DATADOG_TRACE_ID' => 'a', - 'HTTP_X_DATADOG_PARENT_ID' => '456', - 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '0' } => false, - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => '456', - 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => 'ooops' } => true, # corner case, 0 is valid for a sampling priority - { 'HTTP_X_DATADOG_TRACE_ID' => '0', - 'HTTP_X_DATADOG_PARENT_ID' => '0' } => false, - { 'HTTP_X_DATADOG_TRACE_TYPO' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => '456' } => false, - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => 'b', - 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '0' } => false, - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_TYPO' => '456' } => false, - # Parent id is not required when origin is synthetics - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => '0', - 'HTTP_X_DATADOG_ORIGIN' => 'not-synthetics' } => false, - { 'HTTP_X_DATADOG_TRACE_ID' => '123', - 'HTTP_X_DATADOG_PARENT_ID' => '0', - 'HTTP_X_DATADOG_ORIGIN' => 'synthetics' } => true - } - - test_cases.each do |env, expected| - dh = Datadog::DistributedHeaders.new(env) - if dh.valid? - assert_equal(expected, true, "with #{env} valid? should be true") - else - assert_equal(expected, false, "with #{env} valid? should be false") - end - end - end - - def test_trace_id - test_cases = { - { 'HTTP_X_DATADOG_TRACE_ID' => '123' } => 123, - { 'HTTP_X_DATADOG_TRACE_ID' => '0' } => nil, - { 'HTTP_X_DATADOG_TRACE_ID' => '-1' } => 18446744073709551615, - { 'HTTP_X_DATADOG_TRACE_ID' => '-8809075535603237910' } => 9637668538106313706, - { 'HTTP_X_DATADOG_TRACE_ID' => 'ooops' } => nil, - { 'HTTP_X_DATADOG_TRACE_TYPO' => '1' } => nil, - { 'HTTP_X_DATADOG_TRACE_ID' => Datadog::Span::MAX_ID.to_s } => nil, - { 'HTTP_X_DATADOG_TRACE_ID' => (Datadog::Span::MAX_ID - 1).to_s } => Datadog::Span::MAX_ID - 1 - } - - test_cases.each do |env, expected| - dh = Datadog::DistributedHeaders.new(env) - if expected - assert_equal(expected, dh.trace_id, "with #{env} trace_id should return #{expected}") - else - assert_nil(dh.trace_id, "with #{env} trace_id should return nil") - end - end - end - - def test_parent_id - test_cases = { - { 'HTTP_X_DATADOG_PARENT_ID' => '123' } => 123, - { 'HTTP_X_DATADOG_PARENT_ID' => '0' } => nil, - { 'HTTP_X_DATADOG_PARENT_ID' => 'a' } => nil, - { 'HTTP_X_DATADOG_PARENT_ID' => '' } => nil, - { 'HTTP_X_DATADOG_PARENT_ID' => '-1' } => 18446744073709551615, - { 'HTTP_X_DATADOG_PARENT_ID' => '-8809075535603237910' } => 9637668538106313706, - { 'HTTP_X_DATADOG_PARENT_ID' => 'ooops' } => nil, - { 'HTTP_X_DATADOG_PARENT_TYPO' => '1' } => nil, - { 'HTTP_X_DATADOG_PARENT_ID' => Datadog::Span::MAX_ID.to_s } => nil, - { 'HTTP_X_DATADOG_PARENT_ID' => (Datadog::Span::MAX_ID - 1).to_s } => Datadog::Span::MAX_ID - 1 - } - - test_cases.each do |env, expected| - dh = Datadog::DistributedHeaders.new(env) - if expected - assert_equal(expected, dh.parent_id, "with #{env} parent_id should return #{expected}") - else - assert_nil(dh.parent_id, "with #{env} parent_id should return nil") - end - end - end - - def test_sampling_priority - test_cases = { - { 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '0' } => 0, - { 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '1' } => 1, - { 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '2' } => 2, - { 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '999' } => 999, - { 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => '-1' } => nil, - { 'HTTP_X_DATADOG_SAMPLING_PRIORITY' => 'ooops' } => 0, - # nil cases below are very important to test, they are valid real-world use cases - {} => nil, - { 'HTTP_X_DATADOG_SAMPLING_TYPO' => '1' } => nil - } - - test_cases.each do |env, expected| - dh = Datadog::DistributedHeaders.new(env) - if expected - assert_equal(expected, dh.sampling_priority, "with #{env} sampling_priority should return #{expected}") - else - assert_nil(dh.sampling_priority, "with #{env} sampling_priority should return nil") - end - end - end -end