-
Notifications
You must be signed in to change notification settings - Fork 373
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.
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
Return string for trace_id
and span_id
from Correlation::Identifier
#3322
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.0 #3322 +/- ##
==========================================
- Coverage 98.10% 98.10% -0.01%
==========================================
Files 1252 1252
Lines 72342 72303 -39
Branches 3387 3387
==========================================
- Hits 70974 70935 -39
Misses 1368 1368 ☔ View full report in Codecov by Sentry. |
@@ -48,7 +48,7 @@ def initialize( | |||
# Dup and freeze strings so they aren't modified by reference. | |||
@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 | |||
@span_id = (span_id || 0).to_s |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -48,7 +48,7 @@ def initialize( | |||
# Dup and freeze strings so they aren't modified by reference. |
There was a problem hiding this comment.
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."
2.0 Upgrade Guide notes
When manually correlating logs with
Datadog::Tracing.correlation.trace_id
andDatadog::Tracing.correlation.span_id
, make sure to check if your usage still compatible after returning string instead of integer.🚨 Breaking changes: Other fields(except
service
,env
,version
,trace_id
andspan_id
) removed.What does this PR do?
Return string for
trace_id
andspan_id
fromCorrelation::Identifier
Remove other fields that are not consider public API.
For Datadog employees:
credentials of any kind, I've requested a review from
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!