-
Notifications
You must be signed in to change notification settings - Fork 324
[6to7] Add support for 128-bit TraceId #4918
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
Conversation
2ac08b4 to
ee8b38f
Compare
bb627e9 to
b025e32
Compare
bantonsson
left a comment
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.
Great work. Left some comments and ramblings. I'll get around to looking at the propagation next.
dd-trace-core/src/main/java/datadog/trace/core/propagation/TraceIdWithOriginal.java
Outdated
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/DD128bTraceId.java
Outdated
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/DD128bTraceId.java
Outdated
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/IdGenerationStrategy.java
Outdated
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/IdGenerationStrategy.java
Outdated
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/IdGenerationStrategy.java
Outdated
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/config/TracerConfig.java
Outdated
Show resolved
Hide resolved
dd-trace-core/src/main/java/datadog/trace/core/propagation/B3HttpCodec.java
Outdated
Show resolved
Hide resolved
ec14449 to
1de538b
Compare
dd-trace-api/src/main/java/datadog/trace/api/DD128bTraceId.java
Outdated
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/DD128bTraceId.java
Outdated
Show resolved
Hide resolved
dd-trace-core/src/main/java/datadog/trace/core/propagation/W3CHttpCodec.java
Outdated
Show resolved
Hide resolved
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/PTagsFactory.java
Show resolved
Hide resolved
dd-trace-api/src/main/java/datadog/trace/api/DD128bTraceId.java
Outdated
Show resolved
Hide resolved
cdad9ad to
19b8601
Compare
19b8601 to
6030e16
Compare
4ec59bc to
6e670dc
Compare
…it to 128-bit TraceId It will prevent returning invalid TraceId when truncating 128-bit TraceId.
Having same hashcode for unequal objects can lead to performance issues.
Enable caching by default Only B3 without padding feature and original string will have to compute hex string without cache.
8593279 to
95f05ae
Compare
| this.str = s = Long.toUnsignedString(this.lowOrderBits); | ||
| } | ||
| return 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.
I'm quite sure I'm missing something here... but I'm really curious and I want to understand it, why you create an additional variable there instead of just operating with the class property? something like:
@Override
public String toString() {
if (this.str == null) {
this.str = Long.toUnsignedString(this.lowOrderBits);
}
return this.str;
}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.
Hey @odin-delrio, as the author of the original code in DDTraceId I can say that it's about helping the compiler. The javac compiler is rather dumb in the respect that it will do exactly what you tell it.
In the code in this PR, there will be one read from the str field, and one store to the str field (if it was null), while the code you proposed will have two reads from the str field no matter what (and of course one store if it was null). Local variables are free compared to objects and field access.
Yes, this will be optimized away by the JIT eventually, but being mindful of the cost of field access and method calls is ingrained in my way of thinking.
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.
hey @bantonsson, thanks a lot for the detailed explanation ❤️!!! (I was not even sure if I would get a response to a random question like this, but I was reading the code and my curiosity made me raise the question).
I didn't know that reading from a field implied a higher cost than reading from a local variable, now it makes sense to me.
|
How are we supposed to get these 128-bit TraceIds back out? The |
What Does This Do
This PR introduces the support for 128-bit TraceId.
A flag was introduced
trace.128.bit.traceid.generation.enabledto generate 64-bit or 128-bit TraceIds.Motivation
Provide better interoperability with external tracing systems.
Additional Notes
I will keep iterating on the
DDTraceIdAPI to get it smaller and cleaner as possible.I think I can also improve cached string for ID, caching only padded representation (
B3hasTraceIdWithOriginalimplementation that can help with its custom format cache).Two others issues will be filled about:
And another PR is dedicated to bring 128-bit TraceId logging support: #4938