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

Fix manual log injection for 128 bit trace_id #2974

Merged
merged 1 commit into from
Jul 24, 2023
Merged

Fix manual log injection for 128 bit trace_id #2974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 9 additions & 12 deletions lib/datadog/tracing/correlation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ class Identifier
:span_resource,
:span_service,
:span_type,
:trace_id,
Copy link
Member

Choose a reason for hiding this comment

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

I think removing trace_id from the public api of this class is problematic.

I think a better solution is to allow trace_id to continue to be called, but return a string when Datadog.configuration.tracing.trace_id_128_bit_logging_enabled is true. This means users that opt into this new feature get different behavior, but only after enabling this option setting.

:trace_name,
:trace_resource,
:trace_service,
Expand Down Expand Up @@ -65,22 +64,20 @@ def to_log_format
attributes << "#{LOG_ATTR_ENV}=#{env}" unless env.nil?
attributes << "#{LOG_ATTR_SERVICE}=#{service}"
attributes << "#{LOG_ATTR_VERSION}=#{version}" unless version.nil?
attributes << "#{LOG_ATTR_TRACE_ID}=#{logging_trace_id}"
attributes << "#{LOG_ATTR_TRACE_ID}=#{trace_id}"
attributes << "#{LOG_ATTR_SPAN_ID}=#{span_id}"
attributes.join(' ')
end
end

private

def logging_trace_id
@logging_trace_id ||=
if Datadog.configuration.tracing.trace_id_128_bit_logging_enabled &&
!Tracing::Utils::TraceId.to_high_order(@trace_id).zero?
Kernel.format('%032x', trace_id)
else
Tracing::Utils::TraceId.to_low_order(@trace_id)
end
# DEV-2.0: This public method was returning an Integer, but with 128 bit trace id it would return a String.
def trace_id
if Datadog.configuration.tracing.trace_id_128_bit_logging_enabled &&
!Tracing::Utils::TraceId.to_high_order(@trace_id).zero?
Kernel.format('%032x', @trace_id)
else
Tracing::Utils::TraceId.to_low_order(@trace_id)
end
end
Comment on lines +75 to 81
Copy link
Member

Choose a reason for hiding this comment

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

I think this calculation should be repeated for multiple calls of Identifier#trace_id.
I suggest either moving this to #initialize and saving the result to trace_id, or caching it here @trace_id ||= if Datadog.configuration.tracing.trace_id_128_bit_logging_enabled....

end

Expand Down
70 changes: 70 additions & 0 deletions spec/datadog/tracing/correlation_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,5 +424,75 @@ def have_attribute(attribute)
end
end
end

describe '#trace_id' do
context 'is defined' do
context 'when 128 bit trace id logging is not enabled' do
before do
allow(Datadog.configuration.tracing).to receive(:trace_id_128_bit_logging_enabled).and_return(false)
end

context 'when given 64 bit trace id' do
it 'returns to lower 64 bits of trace id' do
trace_id = 0xaaaaaaaaaaaaaaaa
expected_trace_id = 0xaaaaaaaaaaaaaaaa

identifier = described_class.new(trace_id: trace_id)

expect(identifier.trace_id).to eq(expected_trace_id)
end
end

context 'when given 128 bit trace id' do
it 'returns to lower 64 bits of trace id' do
trace_id = 0xaaaaaaaaaaaaaaaaffffffffffffffff
expected_trace_id = 0xffffffffffffffff

identifier = described_class.new(trace_id: trace_id)

expect(identifier.trace_id).to eq(expected_trace_id)
end
end
end

context 'when 128 bit trace id logging is enabled' do
before do
allow(Datadog.configuration.tracing).to receive(:trace_id_128_bit_logging_enabled).and_return(true)
end

context 'when given 64 bit trace id' do
it 'returns lower 64 bits of trace id' do
trace_id = 0xaaaaaaaaaaaaaaaa
expected_trace_id = 0xaaaaaaaaaaaaaaaa

identifier = described_class.new(trace_id: trace_id)

expect(identifier.trace_id).to eq(expected_trace_id)
end
end

context 'when given > 64 bit trace id' do
it 'returns the entire trace id in hex encoded and zero padded format' do
trace_id = 0x00ffffffffffffffaaaaaaaaaaaaaaaa

identifier = described_class.new(trace_id: trace_id)

expect(identifier.trace_id).to eq('00ffffffffffffffaaaaaaaaaaaaaaaa')
end
end
end

context 'when given > 64 bit trace id but high order is 0' do
it 'returns to lower 64 bits of trace id' do
trace_id = 0x00000000000000000aaaaaaaaaaaaaaaa
expected_trace_id = 0xaaaaaaaaaaaaaaaa

identifier = described_class.new(trace_id: trace_id)

expect(identifier.trace_id).to eq(expected_trace_id)
end
end
end
end
end
end