Support for 128 bits trace ids #507

hypnoce · 2018-07-27T12:23:37Z

Which problem is this PR solving?

Fixes #495 : support for 128 bits trace ids.

Short description of the changes

Added environment variable JAEGER_TRACEID_128BIT
Added TraceId and SpanId classes
Added withTraceId128Bit method to Configuration
Added withTraceId128Bit method to Tracer.Builder
Added useTraceId128Bit property to Tracer

Inspired from jaegertracing/jaeger-client-csharp#94

codecov · 2018-07-27T12:30:33Z

Codecov Report

Merging #507 into master will increase coverage by 0.35%.
The diff coverage is 98.55%.

@@             Coverage Diff              @@
##             master     #507      +/-   ##
============================================
+ Coverage     89.12%   89.48%   +0.35%     
- Complexity      521      538      +17     
============================================
  Files            67       68       +1     
  Lines          1895     1940      +45     
  Branches        243      249       +6     
============================================
+ Hits           1689     1736      +47     
+ Misses          133      130       -3     
- Partials         73       74       +1

Impacted Files	Coverage Δ	Complexity Δ
...egertracing/internal/propagation/TextMapCodec.java	`94.54% <100%> (+0.54%)`	`36 <2> (+5)`	⬆️
...io/jaegertracing/internal/JaegerObjectFactory.java	`100% <100%> (ø)`	`4 <1> (ø)`	⬇️
...jaegertracing/zipkin/internal/V2SpanConverter.java	`87.83% <100%> (ø)`	`21 <0> (ø)`	⬇️
.../src/main/java/io/jaegertracing/Configuration.java	`95.51% <100%> (+0.09%)`	`44 <1> (+2)`	⬆️
...o/jaegertracing/internal/propagation/HexCodec.java	`91.48% <100%> (+18.98%)`	`18 <4> (+6)`	⬆️
...ertracing/internal/propagation/B3TextMapCodec.java	`85.93% <100%> (+0.93%)`	`19 <17> (ø)`	⬇️
...n/java/io/jaegertracing/internal/JaegerTracer.java	`90.67% <100%> (+0.25%)`	`25 <1> (+1)`	⬆️
...ertracing/zipkin/internal/ThriftSpanConverter.java	`85.71% <100%> (+0.15%)`	`21 <0> (ø)`	⬇️
...nternal/exceptions/TraceIdOutOfBoundException.java	`100% <100%> (ø)`	`1 <1> (?)`
...a/io/jaegertracing/internal/JaegerSpanContext.java	`97.61% <93.33%> (+0.64%)`	`26 <9> (+4)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ee913a...bda3c16. Read the comment docs.

yurishkuro · 2018-07-27T18:48:10Z

one meta comment/question: is the encapsulation afforded by SpanId type worth an extra memory allocation? With TraceId is kind of unavoidable, since some sort of object is always required (even as byte[])

hypnoce · 2018-07-27T18:51:56Z

This is something that I though of. I followed the c# implementation (even though c# handles better 'value types' in terms of memory than java for now).
I have no opinion on this one. Just that it creates a semantic around ids and is in par with the implementation of trace id.

yurishkuro · 2018-07-27T18:53:46Z

fwiw, census also uses a dedicated type https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/trace/SpanId.java

hypnoce · 2018-07-30T17:03:21Z

@yurishkuro let me know if you find anything that must be adapted in this PR. Thanks !

isaachier

My main concern here is backwards-compatibility, especially for span ids. I'm not sure how to change this without breaking trace ids anyway.

isaachier · 2018-08-06T12:10:40Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

@@ -68,15 +68,15 @@ public String getBaggageItem(String key) {
    return this.baggage;
  }

-  public long getTraceId() {
+  public TraceId getTraceId() {


This probably breaks backwards-compatibility. Is there a way to preserve the return type? I realize for TraceId this may be difficult, but why change long to SpanId unnecessarily?

I believe it makes the API cleaner to have SpanId and TraceId as returned type.
I think keeping long for span and TraceId for traces is a short term solution that might change if spanIds become 128 bits for any reason. I would rather encapsulate now than later. What do you think ?

We can also change the method name and have both.

isaachier · 2018-08-06T12:25:24Z

jaeger-core/src/test/java/io/jaegertracing/ConfigurationTest.java

@@ -184,11 +203,11 @@ public void testPropagationB3Only() {
    System.setProperty(Configuration.JAEGER_PROPAGATION, "b3");
    System.setProperty(Configuration.JAEGER_SERVICE_NAME, "Test");

-    long traceId = 1234;
-    long spanId = 5678;
+    TraceId traceId = new TraceId(1234);


Please try a bigger TraceId value, preferably > 64 bits. I'd like to see propagation works for large numbers too.

I just noticed that we propagate 128 bits ids in B3 formats. Not sure how the receiver of the trace context (ie the brave library) will react to this.

isaachier · 2018-08-06T12:26:56Z

jaeger-core/src/test/java/io/jaegertracing/internal/SpanIdTest.java

@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2016, Uber Technologies, Inc


Please change year to 2018 seeing as this file is new. For other files that exist already, you can do <year>-2018 based on what was there originally.

isaachier · 2018-08-06T12:40:03Z

I read the comments here and see @yurishkuro suggested the dedicate span id type. So I'll leave that up to him.

@yurishkuro are you sure Java always allocates new objects and never relies on escape analysis? I'm seeing online that it is used rarely, but this seems like an ideal use case.

hypnoce · 2018-08-06T12:45:00Z

Escape analysis will only work in really special cases that I believe won't work here since the SpanId object is attached to a field of another object that is not itself escaped. Basically SpanId leaves the stack.

isaachier · 2018-08-06T12:46:40Z

@hypnoce I get your point. I just mean that it is really just wrapping a long so it ought to be treated as one by the JIT compiler. I'd be surprised that it has to go to the heap each time that value is used.

hypnoce · 2018-08-06T12:51:33Z

Totally see your point. I'm not really sure how the JVM is able to optimise such objects. For sure, 'value-types' (http://openjdk.java.net/jeps/169) will be a perfect solution for this in future releases of Java.

yurishkuro · 2018-08-06T16:11:33Z

The OT proposal to expose trace/span IDs as strings has been accepted and will be coming down the line as two new methods, asTraceId/asSpanId. So my preference would be to hide the internal trace/span IDs completely, that would allow us to pick the most efficient types, namely long traceIdLow, traceIdHigh, spanId

hypnoce · 2018-08-07T08:27:23Z

So you are saying that the method should return strings and not internal id representations ?

yurishkuro · 2018-08-07T22:22:44Z

Yes, but I am referring to the new methods like SpanContext.toTraceId(). If we had those methods, I question the need for getTraceId(), which kinda becomes impossible without an artificial wrapper if internally the trace ID is represented as two longs.

hypnoce · 2018-09-13T22:16:49Z

@yurishkuro
Sorry for the delay. I had a busy month for a release.
Do you prefer to wait for the new methods in the opentracing API and return a String ? Or merge as is and update once the OT api is updated ?

yurishkuro · 2018-09-13T22:55:29Z

We do not have to wait for the new methods, they are not going to affect the internal representation. My main objection is to using heap-allocated types:

      TraceId traceId,
      SpanId spanId,
      SpanId parentId,

We should think about how we want to represent the IDs efficiently and strategically. The options are:

as in this PR (the most inefficient, I think)
as longs / pairs of longs
as strings
as byte arrays

They have their pros and cons:

longs require no memory allocations
strings are how we receive and send IDs over the wire most of the time
byte arrays will be mostly useful when spans are serialized to protobuf (in thrift we still need longs)

We can also realize that except for the root span, the rest of the spans will either get trace ID from the parent span in-process (i.e. as already allocated object), or from serialized representation (ie. a string or a byte array - doing longs requires unmarshalling).

I think to unblock this PR, longs are the simplest path forward. The only breaking change would be getTraceId() would need to return a string instead of long.

hypnoce · 2018-09-13T23:17:45Z

So the getTraceId will allocate a String each time ?
Also, converting the span to thrift requires long. Either we need to expose the high/low ids of the trace or convert back the string to high/low ids when serializing to thrift (longs => string => longs).

I would also point out that string representation is the worst in terms of memory, a String taking at least as much space as 2 longs on 32 bits arch (length + empty array = 4 + 12 = 16 + at least 1 byte per character)

yurishkuro · 2018-09-19T06:19:01Z

I'm ok with getTraceId() returning string - we should deprecate the method altogether.

yes, the trace ID can be represented by hi/lo longs, same as in Go client.

hypnoce · 2018-10-03T23:48:04Z

@yurishkuro I've updated the PR according to our previous discussion. Let me know if you have any comment !

yurishkuro

overall lgtm, comments are mostly stylistic.

yurishkuro · 2018-10-04T01:19:43Z

jaeger-core/src/main/java/io/jaegertracing/internal/exceptions/TraceIdOutOfBoundException.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2016, Uber Technologies, Inc


2018, The Jaeger Authors

yurishkuro · 2018-10-04T01:27:58Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TextMapCodec.java

    // TODO(isaachier): When we drop Java 1.6 support, use Long.parseUnsignedLong instead of using BigInteger.
    return new JaegerSpanContext(
-        new BigInteger(parts[0], 16).longValue(),
+        new BigInteger(traceId, 16).longValue(),


Can we use hexCodec to parse?

Here I wanted to be consistent with the parsing of the spanId and parentId.
Do you want to replace all usage of BigInteger by HexCodec ?

yurishkuro · 2018-10-05T03:58:50Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerObjectFactory.java

      long spanId,
      long parentId,
      byte flags,
      Map<String, String> baggage,
      String debugId) {
-    return new JaegerSpanContext(traceId, spanId, parentId, flags, baggage, debugId, this);
+    return new JaegerSpanContext(traceIdLow, traceIdHigh, spanId, parentId, flags, baggage, debugId, this);


nit: might be my OCD, but feels like it should be (high, low), not (low, high). Would match HexCodec.toLowerHex(hi, lo).

jaeger-thrift/src/test/java/io/jaegertracing/thrift/internal/senders/UdpSenderTest.java

jaeger-zipkin/src/main/java/io/jaegertracing/zipkin/internal/V2SpanConverter.java

jaeger-zipkin/src/test/java/io/jaegertracing/zipkin/ZipkinSenderTest.java

yurishkuro

Lgtm, great job!

pavolloffay · 2018-10-08T08:28:43Z

jaeger-core/src/main/java/io/jaegertracing/Configuration.java

+  /**
+   *  Opt-in to use 128 bit traceIds. By default, uses 64 bits.
+   */
+  public static final String JAEGER_TRACEID_128BIT = JAEGER_PREFIX + "TRACEID_128BIT";


It should be documented in readme

There is currently no structured documentation in the README for the env variable. The only thing the current documentation : jaegertracing/documentation#157
Do you want to create a place where all the env variable are listed and documented ?

jpkrohling

I have only a couple of minor non-blocker comments, LGTM.

jpkrohling · 2018-10-08T09:18:36Z

jaeger-core/src/main/java/io/jaegertracing/internal/JaegerSpanContext.java

+      final int padLeftLength = 16 - hexStringLow.length();
+      char[] zeroPadLeft = new char[padLeftLength];
+      Arrays.fill(zeroPadLeft, '0');
+      return hexStringHigh + new String(zeroPadLeft) + hexStringLow;


This doesn't look 100% right to me: you are allocating a new array of zeroes every time you need to get the trace ID and the low value is lower than 16. Better would be something like:

"0000000000000000".substring(hexStringLow.length()) + hexStringLow;

The zeroes are then treated as constant, which should have a better performance than dynamically allocating an array every time.

jpkrohling · 2018-10-08T09:30:02Z

jaeger-core/src/main/java/io/jaegertracing/internal/exceptions/TraceIdOutOfBoundException.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2018, Uber Technologies, Inc


If this is a new source file, this should be assigned to "The Jaeger Tracing Authors". See https://github.com/jaegertracing/jaeger-client-java/blob/master/CONTRIBUTING.md#license-and-certificate-of-origin

jpkrohling · 2018-10-08T09:31:52Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/HexCodec.java

+   */
+  static Long higherHexToUnsignedLong(String higherHex) {
+    if (higherHex.length() > 32 || higherHex.length() < 1) {
+      return null;


I think it would be worth a log message somewhere about this...

Done at debug Level. I though warn would be also good since it's a situation that should arrive rarely or that can cause tracing to malfunction.
Let me know what you think.

jpkrohling · 2018-10-08T09:35:29Z

jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TextMapCodec.java

        new BigInteger(parts[1], 16).longValue(),
        new BigInteger(parts[2], 16).longValue(),
        new BigInteger(parts[3], 16).byteValue());
  }

+  private static long high(String hexString) {


It looks like you expect to receive a full id (high + low) in hex here and you are getting only the first 16 chars. If that's indeed the case, please add a javadoc.

If you are instead expecting only the high to be in this hexString, then add also a log message in the condition below (> 16), as it might indicate that the data the tracer is receiving is bad in some way.

Added a javadoc

Added environment variable JAEGER_TRACEID_128BIT Added withTraceId128Bit method to Configuration Added withTraceId128Bit method to Tracer.Builder Added useTraceId128Bit property to Tracer Signed-off-by: JACQUES Francois <[email protected]>

hypnoce · 2018-10-13T12:24:42Z

I’ve made some changes according to the last comments.

jpkrohling · 2018-10-15T07:47:34Z

Thank you!

jpkrohling changed the title ~~Fixes #495 : support for 128 bits trace ids.~~ Support for 128 bits trace ids. Jul 27, 2018

jpkrohling changed the title ~~Support for 128 bits trace ids.~~ Support for 128 bits trace ids Jul 27, 2018

isaachier suggested changes Aug 6, 2018

View reviewed changes

hypnoce mentioned this pull request Oct 3, 2018

Document 128bit trace ID support on Java Client as available jaegertracing/documentation#157

Merged

yurishkuro reviewed Oct 5, 2018

View reviewed changes

yurishkuro approved these changes Oct 5, 2018

View reviewed changes

pavolloffay approved these changes Oct 8, 2018

View reviewed changes

jpkrohling approved these changes Oct 8, 2018

View reviewed changes

Fixes #495 : support for 128 bits trace ids.

bda3c16

Added environment variable JAEGER_TRACEID_128BIT Added withTraceId128Bit method to Configuration Added withTraceId128Bit method to Tracer.Builder Added useTraceId128Bit property to Tracer Signed-off-by: JACQUES Francois <[email protected]>

jpkrohling approved these changes Oct 15, 2018

View reviewed changes

jpkrohling merged commit 88a8497 into jaegertracing:master Oct 15, 2018

pavolloffay mentioned this pull request May 10, 2019

Automatically generate changelog #629

Closed

		@@ -0,0 +1,51 @@
		/*
		* Copyright (c) 2016, Uber Technologies, Inc

		@@ -0,0 +1,24 @@
		/*
		* Copyright (c) 2016, Uber Technologies, Inc

		@@ -0,0 +1,24 @@
		/*
		* Copyright (c) 2018, Uber Technologies, Inc

Support for 128 bits trace ids #507

Support for 128 bits trace ids #507

Conversation

hypnoce commented Jul 27, 2018 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jul 27, 2018 • edited Loading

Codecov Report

yurishkuro commented Jul 27, 2018

hypnoce commented Jul 27, 2018

yurishkuro commented Jul 27, 2018

hypnoce commented Jul 30, 2018

isaachier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachier commented Aug 6, 2018

hypnoce commented Aug 6, 2018

isaachier commented Aug 6, 2018

hypnoce commented Aug 6, 2018

yurishkuro commented Aug 6, 2018

hypnoce commented Aug 7, 2018

yurishkuro commented Aug 7, 2018

hypnoce commented Sep 13, 2018

yurishkuro commented Sep 13, 2018

hypnoce commented Sep 13, 2018 • edited Loading

yurishkuro commented Sep 19, 2018

hypnoce commented Oct 3, 2018

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypnoce Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypnoce commented Oct 13, 2018 • edited Loading

jpkrohling commented Oct 15, 2018

hypnoce commented Jul 27, 2018 •

edited

Loading

codecov bot commented Jul 27, 2018 •

edited

Loading

hypnoce commented Sep 13, 2018 •

edited

Loading

hypnoce Oct 5, 2018 •

edited

Loading

hypnoce commented Oct 13, 2018 •

edited

Loading