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

Jump to bottom

Return string for trace_id and span_id from Correlation::Identifier #3322

Merged
merged 8 commits into from
Jan 26, 2024
Merged

Return string for trace_id and span_id from Correlation::Identifier #3322

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
92fbc34
Return string for trace id
TonyCTHsu Dec 12, 2023
2735aea
Return string for span id
TonyCTHsu Dec 12, 2023
5849dc8
Use `Datadog::Tracing::Utils::TraceId.next_id` in test
TonyCTHsu Dec 12, 2023
996bba3
Fix tracer spec
TonyCTHsu Dec 12, 2023
c8bf1dc
Merge branch '2.0' into tonycthsu/correlation-string
TonyCTHsu Jan 23, 2024
988934b
Remove redundant fields
TonyCTHsu Jan 23, 2024
48c8b77
Fix removed span_id in test
TonyCTHsu Jan 24, 2024
5e1a480
Lint
TonyCTHsu Jan 24, 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
Prev Previous commit
Next Next commit
Remove redundant fields
  • Loading branch information
@TonyCTHsu
TonyCTHsu committed Jan 23, 2024
commit 988934b781ddd149dde788b1efd0405687373f1a
30 changes: 2 additions & 28 deletions lib/datadog/tracing/correlation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ module Datadog
module Tracing
# Contains behavior for managing correlations with tracing
# e.g. Retrieve a correlation to the current trace for logging, etc.
# This class is for usage with log correlation.
# To continue from a trace, users should use TraceDigest instead.
module Correlation
# Represents current trace state with key identifiers
# @public_api
Expand All @@ -21,42 +23,21 @@ class Identifier
:env,
:service,
:span_id,
:span_name,
:span_resource,
:span_service,
:span_type,
:trace_name,
:trace_resource,
:trace_service,
:version

# @!visibility private
def initialize(
env: nil,
service: nil,
span_id: nil,
span_name: nil,
span_resource: nil,
span_service: nil,
span_type: nil,
trace_id: nil,
trace_name: nil,
trace_resource: nil,
trace_service: nil,
version: nil
)
# Dup and freeze strings so they aren't modified by reference.
Copy link
Member

Choose a reason for hiding this comment

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

Let's document that "this class is for usage with log correlation. To continue from a trace, users should use TraceDigest instead."

@env = Core::Utils::SafeDup.frozen_dup(env || Datadog.configuration.env)
@service = Core::Utils::SafeDup.frozen_dup(service || Datadog.configuration.service)
@span_id = (span_id || 0).to_s
Copy link
Member

Choose a reason for hiding this comment

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

@TonyCTHsu I don't remember: why are we converting the numbers to strings?

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcotc's question suggests that we should include that in the release notes somewhere.

Apparently, Ruby integers can natively handle 128-bit numbers, so I agree. It's not clear why we need to convert them to strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcotc @ekump The value returned by those methods are meant to be injected into log entries.

Hence, returning string instead of integer provides finer control for implementing different encoding.

Copy link
Contributor

Choose a reason for hiding this comment

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

The value returned by those methods are meant to be injected into log entries

Is that the only supported use for the return value? This is part of the public API, so should we be considering more generic usage?

Generally speaking, I don't think public API methods should be too opinionated on what is done with their return values (within reason) and we should be returning values as the types that best reflect what they are. This contributes to an API being predictable and intuitive for an engineer to use. If I know trace_ids are 128-bit integers, and I see a method called trace_id I would assume/expect it to return an integer.

It should be the responsibility of the calling method to convert it to a string and/or encode it a particular way unless we have a good reason to do otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

@ekump we have the method #to_log_format which is a helper method you use when you want to inject the correlation into your logs safely, that method is opinionated.

Copy link
Member

@marcotc marcotc Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here an interesting bit, OTel uses 128-bit trace ids as well, but it represents them as 16-char strings.
OTel always passes these 16 bytes around or, if needed to be converted to a printable string, converts it to hex representation.

@span_name = Core::Utils::SafeDup.frozen_dup(span_name)
@span_resource = Core::Utils::SafeDup.frozen_dup(span_resource)
@span_service = Core::Utils::SafeDup.frozen_dup(span_service)
@span_type = Core::Utils::SafeDup.frozen_dup(span_type)
@trace_id = trace_id || 0
@trace_name = Core::Utils::SafeDup.frozen_dup(trace_name)
@trace_resource = Core::Utils::SafeDup.frozen_dup(trace_resource)
@trace_service = Core::Utils::SafeDup.frozen_dup(trace_service)
@version = Core::Utils::SafeDup.frozen_dup(version || Datadog.configuration.version)
end

Expand Down Expand Up @@ -112,14 +93,7 @@ def identifier_from_digest(digest)

Identifier.new(
span_id: digest.span_id,
span_name: digest.span_name,
span_resource: digest.span_resource,
span_service: digest.span_service,
span_type: digest.span_type,
trace_id: digest.trace_id,
trace_name: digest.trace_name,
trace_resource: digest.trace_resource,
trace_service: digest.trace_service
)
end
end
Expand Down
51 changes: 1 addition & 50 deletions spec/datadog/tracing/correlation_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,16 +47,9 @@
env: default_env,
service: default_service,
span_id: '0',
span_name: nil,
span_resource: nil,
span_service: nil,
span_type: nil,
trace_id: '0',
trace_name: nil,
trace_resource: nil,
trace_service: nil,
version: default_version
)
)
end

it 'has frozen copies of strings' do
Expand Down Expand Up @@ -89,28 +82,14 @@
env: default_env,
service: default_service,
span_id: span_id.to_s,
span_name: span_name,
span_resource: span_resource,
span_service: span_service,
span_type: span_type,
trace_id: low_order_trace_id(trace_id).to_s,
trace_name: trace_name,
trace_resource: trace_resource,
trace_service: trace_service,
version: default_version
)
end

it 'has frozen copies of strings' do
expect(identifier.env).to be_a_frozen_copy_of(default_env)
expect(identifier.service).to be_a_frozen_copy_of(default_service)
expect(identifier.span_name).to be_a_frozen_copy_of(span_name)
expect(identifier.span_resource).to be_a_frozen_copy_of(span_resource)
expect(identifier.span_service).to be_a_frozen_copy_of(span_service)
expect(identifier.span_type).to be_a_frozen_copy_of(span_type)
expect(identifier.trace_name).to be_a_frozen_copy_of(trace_name)
expect(identifier.trace_resource).to be_a_frozen_copy_of(trace_resource)
expect(identifier.trace_service).to be_a_frozen_copy_of(trace_service)
expect(identifier.version).to be_a_frozen_copy_of(default_version)
end
end
Expand All @@ -126,14 +105,7 @@
env: default_env,
service: default_service,
span_id: '0',
span_name: nil,
span_resource: nil,
span_service: nil,
span_type: nil,
trace_id: '0',
trace_name: nil,
trace_resource: nil,
trace_service: nil,
version: default_version
)
end
Expand All @@ -153,14 +125,7 @@
env: env,
service: service,
span_id: span_id,
span_name: span_name,
span_resource: span_resource,
span_service: span_service,
span_type: span_type,
trace_id: trace_id,
trace_name: trace_name,
trace_resource: trace_resource,
trace_service: trace_service,
version: version
)
end
Expand All @@ -170,28 +135,14 @@
env: env,
service: service,
span_id: span_id.to_s,
span_name: span_name,
span_resource: span_resource,
span_service: span_service,
span_type: span_type,
trace_id: low_order_trace_id(trace_id).to_s,
trace_name: trace_name,
trace_resource: trace_resource,
trace_service: trace_service,
version: version
)
end

it 'has frozen copies of strings' do
expect(identifier.env).to be_a_frozen_copy_of(env)
expect(identifier.service).to be_a_frozen_copy_of(service)
expect(identifier.span_name).to be_a_frozen_copy_of(span_name)
expect(identifier.span_resource).to be_a_frozen_copy_of(span_resource)
expect(identifier.span_service).to be_a_frozen_copy_of(span_service)
expect(identifier.span_type).to be_a_frozen_copy_of(span_type)
expect(identifier.trace_name).to be_a_frozen_copy_of(trace_name)
expect(identifier.trace_resource).to be_a_frozen_copy_of(trace_resource)
expect(identifier.trace_service).to be_a_frozen_copy_of(trace_service)
expect(identifier.version).to be_a_frozen_copy_of(version)
end
end
Expand Down
Loading