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.
@TonyCTHsu I don't remember: why are we converting the numbers to strings?
There was a problem hiding this comment.
@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.
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.
@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.
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.
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_idandDatadog::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_idandspan_id) removed.What does this PR do?
Return string for
trace_idandspan_idfromCorrelation::IdentifierRemove 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!