Get rid of TraceId/SpanId wrappers, and have the SpanContext hold the ids

[jkwatson]

This hasn't been optimized, but it is all functionally working, according to the tests.

Another prototype for #1314

[codecov]

Codecov Report

Merging #1374 into master will decrease coverage by 0.08%.
The diff coverage is 90.28%.

@@             Coverage Diff              @@
##             master    #1374      +/-   ##
============================================
- Coverage     86.56%   86.47%   -0.09%     
- Complexity     1367     1373       +6     
============================================
  Files           162      162              
  Lines          5231     5272      +41     
  Branches        490      504      +14     
============================================
+ Hits           4528     4559      +31     
- Misses          524      531       +7     
- Partials        179      182       +3     

Impacted Files	Coverage Δ	Complexity Δ
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java	81.89% <0.00%> (ø)	78.00 <0.00> (ø)
.../main/java/io/opentelemetry/sdk/trace/Sampler.java	100.00% <ø> (ø)	0.00 <0.00> (ø)
...java/io/opentelemetry/sdk/trace/data/SpanData.java	100.00% <ø> (ø)	0.00 <0.00> (ø)
...ions/trace/jaeger/sampler/JaegerRemoteSampler.java	68.62% <ø> (ø)	9.00 <0.00> (ø)
...ions/trace/jaeger/sampler/PerOperationSampler.java	0.00% <ø> (ø)	0.00 <0.00> (ø)
...ions/trace/jaeger/sampler/RateLimitingSampler.java	66.66% <ø> (ø)	7.00 <0.00> (ø)
...a/io/opentelemetry/exporters/otlp/SpanAdapter.java	95.83% <66.66%> (ø)	19.00 <0.00> (ø)
...ava/io/opentelemetry/sdk/trace/SpanBuilderSdk.java	92.48% <68.75%> (-3.52%)	47.00 <2.00> (+2.00)	⬇️
...telemetry/exporters/zipkin/ZipkinSpanExporter.java	87.30% <75.00%> (ø)	34.00 <0.00> (ø)
...i/src/main/java/io/opentelemetry/trace/SpanId.java	78.94% <86.66%> (-11.06%)	11.00 <9.00> (-9.00)
... and 18 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 7828283...09a34fa. Read the comment docs.

[anuraaga]

I like the approach - happy to take a detailed look if everyone thinks so too.

[carlosalberto]

An interesting take. I'm curious about performance, of course, and also about how expensive would be be now to check for invalid values (TraceId.isValid(), SpanId.isValid()), for propagation purposes:

    // HttpTraceContext.inject()
    if (span == null || !span.getContext().isValid()) {
      return;
    }

Aside that, looks a good, correct approach.

[jkwatson]

If we want to go this way, this PR still needs a bunch of cleanup. I'd probably defer working on performance optimizations, but I'd like to get rid of a bunch of now-unused stuff, and get the javadoc up-to-date before merging.

[jkwatson]

on the SIG call, it was recommended to use CharSequence, rather than String. I'll amend this draft with that.

[jkwatson]

ok, converted over to CharSequence for the ids.

I ran the SpanPipeLineBenchmark on this implementation to see how it looks. The results are significantly worse (+10%) in comparing the number of overall allocations. The hot path here, I think, is the generation of the random ids. The current implementation on the master branch has a very efficient generation of random longs, wrapping them up in the SpanId and TraceId objects that I got rid of here. Note: the benchmark generates all ids, and doesn't exercise the code path that involves extracting or injecting span context from an external source.

So, as another experiment, I tried having the random ids generator return private wrappers for the Longs that lazily evaluated the CharSequence. This recovered most of the lost allocation performance, but it's still slightly worse.

So, I guess the tradeoff here is whether we think it's worth the tradeoff, simply to get rid of the wrapper objects from the API.

@trask and @anuraaga thoughts, since you were the original requestors of #1314 ?

[anuraaga]

Thanks for the pounding on this, it's fun to think about these issues :)

If possible it'd be nice to copy in the JMH results to see the changes for each iteration.

Also, what JDK are you using to run? The problem with strings is they perform significantly differently in Java 8 or 9+, so we need to be careful. If we run benchs on both it's ideal, but otherwise I'd target 11 given how old 8 is by now.

[anuraaga]

I think you can get a significant improvement by using a recycled buffer, maybe closing the gap you're seeing. Here's an example

https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/internal/RecyclableBuffers.java

It's a bit confusing since it used to be sized to just fit an ID but also got generalized to use when generating trace headers, which would probably be nice to do here too in a separate PR :)

[jkwatson]

definitely a good idea!

[carlosalberto]

If we run benchs on both it's ideal, but otherwise I'd target 11 given how old 8 is by now.

I wouldn't entirely drop Java 8, even if it's performance-wise. Jason Plumb showed a link in the last Java SIG call about how Java 8 still dominates the market, percent-wise, these days.

[carlosalberto]

Interesting optimization but considering how common it is to use the trace id as a string

The most common case for random values is usage of the API without the SDK, i.e. a theoretically no-op scenario (expected to be cheap).

With CharSequence the code gets slightly more complex, so we get something and lose something else, so it seems there's no perfect solution here. @jkwatson do you remember the performance results when this was String based?

[jkwatson]

If we run benchs on both it's ideal, but otherwise I'd target 11 given how old 8 is by now.

I wouldn't entirely drop Java 8, even if it's performance-wise. Jason Plumb showed a link in the last Java SIG call about how Java 8 still dominates the market, percent-wise, these days.

FYI, Java 11 is much, much more allocation-efficient than java 8 with the benchmark I've been running, although I'm sure I don't know why that would be!

[jkwatson]

Interesting optimization but considering how common it is to use the trace id as a string

The most common case for random values is usage of the API without the SDK, i.e. a theoretically no-op scenario (expected to be cheap).

With CharSequence the code gets slightly more complex, so we get something and lose something else, so it seems there's no perfect solution here. @jkwatson do you remember the performance results when this was String based?

I can jump back to that commit hash and check. I'll do that.

[jkwatson]

Interesting optimization but considering how common it is to use the trace id as a string

The most common case for random values is usage of the API without the SDK, i.e. a theoretically no-op scenario (expected to be cheap).
With CharSequence the code gets slightly more complex, so we get something and lose something else, so it seems there's no perfect solution here. @jkwatson do you remember the performance results when this was String based?

I can jump back to that commit hash and check. I'll do that.

The String allocation performance was basically identical to what's in this PR, before @anuraaga 's recommendation to re-use the char buffers. So, currently we're significantly better than the String performance, but that hadn't really had any optimization done on it, either.

[jkwatson]

For those interested in playing along at home, I have a gist with the benchmark output over time (in reverse chronological order, and roughly labeled with the changes that were made): https://gist.github.com/jkwatson/5ee98fd7b140364f0435217148569c09

[jkwatson]

So, we don't really have a full end-to-end benchmark from extraction to OTLP, and the way the project is structured, it would require a new module that depended on a bunch of stuff. I think this would be a valuable addition, though.

I did, however, run the extractor and injector benchmarks that we do have, and the API no-op benchmarks as well.

The extractors are, as you would expect, in general much more efficient from an allocation perspective. The injectors are pretty much the same, before and after (this looks to be because we weren't really allocating anything related to traceid/spanid in the inject process, but just copying chars around between arrays).

However the no-op API is quite a bit worse, because now the random DefaultSpan that is created has to create the String representations of the longs. This could be mitigated by wrapping the longs up like I did in the SDK id generator, but then we're basically back where we started, and the agent codebase would need to deal with those wrapper classes in the API.

[anuraaga]

Thanks for the investigations - I didn't think about the no-op use case. How does it work? I figured no-op would just return invalid spans and not generate any randoms - it's no-op :)

[jkwatson]

I'm not 100% sure why we generate IDs for the default spans. @carlosalberto do you have the context for that decision?

[anuraaga]

I keep on remembering this PR and waiting until #1386 to add more comments, but instead of reremembering all the time, let me go ahead and add them with optimism towards that PR

If the no-op case is satisfied by disabling the random IDs, how do we feel about this? My opinion is to go for it

• Using strings doesn't actually solve the issue of there being wrappers at all, a string is a wrapper around bytes. But because it's in the JDK, the auto-instrumentation shading story becomes much simpler, and because we expect the string to be needed anyways in most cases, it's a wrapper that pulls its weight rather than being a no-op

  • We talked about header propagation, let's not forget logs correlation too with something like log4j where we'd be passing a string. I love having tracing IDs in my logs

• Users first interact with these IDs as strings - I'd even wager (not my life savings :P) that 100% of tracing solutions display them as strings. These users also will interact with the SDK, doing manual instrumentation, correlation, or whatever cool use case they have. It seems very natural and user-friendly to present these IDs as the Strings they're used to from the UI in our API.

• There is a case where the theoretical performance goes down - binary trace propagation, proto-based exporter, no correlation to string-based systems like logs. I think the performance impact is negligible enough (we can verify in benchmarks) and this is a narrow enough use case that we probably don't have to worry too much.

Hope this helps, just wanted to present my thoughts.

[jkwatson]

Using strings doesn't actually solve the issue of there being wrappers at all, a string is a wrapper around bytes.

True, but it's a wrapper that has been hyper-optimized by the JVM and JIT over the last 20 years. :)

I also think that this is a good change. I'm also pretty sure that there are still optimizations that can be done, such as casting to Strings when the implementation is a String.

I think binary trace propagation is still an unknown, since we don't know how the ids need to be serialized at this point. long->bytes might be equivalent to String->bytes for those cases; it's just an unknown right now.

Anyway, yes, I also think this is a good move, if we can get #1386 landed.

[bogdandrutu]

With the proposed changes to the SpanContext API we can still store longs but inside the SpanContext. We will have a small problem with Sampler which accepts a TraceId I don't know if we can do better than passing a CharSequnce there but will think.

[bogdandrutu]

Would name this getBase16TraceId.

I would also add helpers for propagators copyBytesTraceId copyBase16TraceId similar to current methods on TraceId.

Same applies to all the components.

[bogdandrutu]

To avoid allocations it is probably better to have the ctor that allows to pass a charsequnce and a position from where it starts.

CharSequnce traceIdBuf, int traceIdOffset for the current ctor all the offsets will be 0 but for w3c we can avoid calling substring and internally we can still store longs if we want or we can for the moment do the substring.

Similar ctor can be offered for bytes Buffer (not that class probably a byte array as a buffer).

[bogdandrutu]

The thing that I don't like about using CharSequnce is the fact that we don't do the copy of the sequence and it is not guaranteed by the jvm that it is immutable, so can be backed by a mutable storage and all our guarantees are gone. So we either go with full String as @anuraaga proposed or we do what I proposed which is we keep the internal storage as longs but inside the SpanContext

[bogdandrutu]

I would check if CharSequence cs; cs.subSequence(0,16).toString() is a no-op if the initial cs is a string of 16 length, if that is the case my proposed API would work the same for both backing the IDs by string or by longs, then we can evaluate which one we want now.

[carlosalberto]

So we either go with full String as @anuraaga proposed or we do what I proposed which is we keep the internal storage as longs but inside the SpanContext

I'm slightly in favor of keeping the ids as longs inside SpanContext (which is what other SIGs do, like the Python one).

[anuraaga]

I'd still recommend String - String is almost always used, for propagation, and often used multiple times, e.g., logs correlation. System.out.println(span) is a reasonable thing to do during debugging something and that's also string. Strings are what people expect when dealing with IDs why not just reflect that?

[jkwatson]

One thing discussed on Friday was the possibility of exposing the inputs in multiple, immutable ways, perhaps both String and long, then the internals can store the ids in whatever way is most optimal, and then possibly expose multiple views on that internal representation, again perhaps String and longs.

[anuraaga]

and longs.

Does anyone use longs? :P I guess this is more having accessors, or helpers, that return byte arrays in addition to string. This is probably mostly a naming issue. getTraceId can return a string or getLower16TraceId returns a string. I'd argue the former is far less surprising name to users but that seems to have a lukewarm reception so ready to give up on that. But yeah the internal storage is less important than the return types of the API methods.

[jkwatson]

I'm going to take this PR and do some more massaging on it. I'm hoping I can just create the barest minimal API that is currently required to implement what we currently have for propagation, etc. Then, when we add binary propagation, we can add additional API methods to facilitate making that work well. I'll focus first on the API surface, and worry a bit less about the SDK internals in the first pass.

[trask]

Just got to reviewing this, and understand now why you like this 😄.

I think maybe only store the CharSequences in SpanContext (and not additionally the longs).

We can use CharSequence implementations backed by one/two longs for the random generated span/trace id. And since those CharSequence implementations will only be used for random generated ids, I think they can be simple, e.g. don't need to worry about endian stuff, something like below.

And the OTLP exporter can do very efficient (bitwise math / zero allocation) conversion from CharSequence to the one/two longs that it wants during export.

And then we could simplify SpanContext, only exposing:

• CharSequence getTraceId()
• CharSequence getSpanId()

---

class RandomCharSequence32 implements CharSequence {

  private final long hi = random.nextLong();
  private final long lo = random.nextLong();

  @Override
  public int length() {
    return 32;
  }

  @Override
  public char charAt(int index) {
    long val;
    int offset;
    if (index < 16) {
      val = hi;
      offset = index;
    } else {
      val = lo;
      offset = index - 16;
    }
    // shift the bits right
    long shifted = val >>> offset;
    // and then mask against 1111 to get only the right-most 4 bits
    int bits = (int) (shifted & 15);
    return (char) ('0' + bits);
  }

  @Override
  public CharSequence subSequence(int start, int end) {
    char[] chars = new char[end - start];
    for (int i = start; i < end; i++) {
      chars[i] = charAt(start + i);
    }
    return new String(chars);
  }

  @Override
  public String toString() {
    char[] chars = new char[32];
    for (int i = 0; i < 32; i++) {
      chars[i] = charAt(i);
    }
    return new String(chars);
  }
}

[jkwatson]

Just got to reviewing this, and understand now why you like this 😄.
:)

I think maybe only store the CharSequences in SpanContext (and not additionally the longs).

Yes. That was mostly an experiment in "what if" to store both. In the end, the advantage of this approach is to hide all the details.

2 questions:

Do you prefer exposing CharSequence or String for the hex representation?
Do you prefer accepting CharSequence or String for the hex-based creation?

[trask]

Sorry, pondering this whole thing still......

Two longs are a very efficient way to store a 32-digit hex string (16 bytes vs 64 bytes), and we can still expose a very efficient (zero-allocation) CharSequence hex representation on top of them.

Strings are also wrapper objects (with an extra layer: String -> pointer to char array -> char array).

What about keeping TraceId/SpanId and having them implement CharSequence?

[trask]

One of the advantages of eliminating TraceId/SpanId is the auto-instrumentation bridge.

But we can get the same performance benefit by turning TraceId/SpanId into interfaces: open-telemetry/opentelemetry-java-instrumentation#975.

There is still some bridge complexity that would go away if we eliminate TraceId/SpanId, but I don't think that's too compelling (@iNikem probably disagrees 😄).

[iNikem]

My ideal is strongly typed without bridge :)

[anuraaga]

I think we can't really avoid a String field. Every time we inject into logs, we have to return an actual String since MDC, log4j context injection, etc only accept String, not CharSequence, and if we're generating that from longs every time that's quite a lot of cost. For a synchronous server this might not be a big deal as context is usually only mounted once per request, but for reactive servers context tends to be mounted hundreds of times per request. Excessive ThreadLocal lookups is already a problem with reactive servers (comes up with spring webflux a lot recently), adding allocations to that could be brutal.

So even if we stick with having longs, I really don't see a way around a lazy inited String somewhere to efficiently support log injection. In that case, the benefit of lazy init is mostly for internal spans with log injection disabled where Strings are never used. I don't know if this is the case to optimize for and my recommendation would be not storing longs to save the 16+8 bytes for the presumably more common use case of requiring Strings for the IDs at some point in the life of the span, and possibly many times.

The above basically sums up that I think if the ID is returned as CharSequence, it's still important to make sure CharSequence.toString() is efficient at least across multiple invocations for the same ID. In which case, the API could just as well return String directly.

As for the wrappers, the spec now clearly defines that the IDs need to be accessible as hex or binary. So I think having these accessors directly on the SpanContext instead of the wrappers seems good to go either way, don't see much of a benefit to having the wrappers.

Also let me repeat my prior art :) #1314 (comment)

And I found another case that will be very happy to have efficient String, opentracing shim :)

https://github.com/open-telemetry/opentelemetry-java/blob/master/opentracing_shim/src/main/java/io/opentelemetry/opentracingshim/SpanContextShim.java#L70

I think there's enough shared knowledge in the ecosystem to see how important it is to have String IDs in Java (ideally in OTLP too, but oh well - I'll continue to repeat my schpiel just for kicks ;) ).

[jkwatson]

My thoughts on the current state of things:

It seems like we are left with several options wrt. the issue of the agent having to do extra wrapping of these classes.

1. Keep the API as-is
   • CON: users of the agent (and authors) will have to deal with the pain of extra wrapping/allocations.
   • PRO: It's a decent API that feels like idiomatic Java
2. Convert the TraceId/SpanId to interfaces
   • CON: The creational methods will need to be moved into static inner classes, or outside of the class entirely
   • PRO: Aside from the creation patterns, it's a decent API that feels like idiomatic Java
3. Get rid of the TraceId/SpanId instances and move them entirely into the SpanContext
   • CON: Creation of the SpanContext potentially becomes messier, if we want to minimize transformations between representations.
   • CON: The contract between the SpanContext and the IdGenerator becomes messier, if we want to minimize transformations between representations.
   • PRO: no more extra wrapper allocations necessary
   • PRO: The SpanContext can hide all implementation and storage details from the user.

Overall considerations:

• There are really 3 representations of the IDs that seem relevant: longs, byte arrays and Strings. longs are excellent for generation-efficiency and memory efficiency, byte arrays are useful for binary wire formats, Strings are useful for text wire formats. Converting between any two of them incurs a cost, potentially in both allocations and cycles.
• We require that our in-memory storage is immutable for the entire lifecycle of a trace, within the process.
  • long is inherently immutable
  • String is inherently immutable
  • byte[] is inherently mutable
  • CharSequence has no mutability guarantees.

So, how do we make a decision on this? If were forced to make the call, I would choose option 2 (convert to interfaces), and figure out how to optimize representational transformations as a separate concern.

[tylerbenson]

I also support option 2 with the suggestion that the interface documentation indicate that the implementations should be immutable.

[jkwatson]

I also support option 2 with the suggestion that the interface documentation indicate that the implementations should be immutable.

yeah...that's also an interesting consideration. anyone could bring their own implementation and really make a mess of things if it wasn't immutable. I wonder if documentation is good enough in this case. [I tend to think it is..but others might be more wary]

[trask]

MDC, log4j context injection, etc only accept String

This is a good point.

Are we going to be injecting context into MDC by default though? I think maybe(?) the preferred route is going to be using the new Log SDK:

(https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/logs/overview.md)

which has the nice benefit of not having to keep the various MDC maps synchronized with our Context values.

[bogdandrutu]

@jkwatson having any of these interfaces is a problem because a propagator or id generator needs to generate some objects then they will need to implement the interfaces.

An alternative is to do 3, make SC an abstract class or interface and provide multiple implementation (backed by longs by strings by etc.)

[bogdandrutu]

I also think we are coming from different worlds, in my experience I never used log correlation (but if that is 90% of the time things may be reconsider)

[trask]

having any of these interfaces is a problem because a propagator or id generator needs to generate some objects then they will need to implement the interfaces.

@bogdandrutu I'm not following this concern. There would be factory methods that propagators and id generators can use to create the standard instances of these interfaces (they don't need to implement the interfaces).

[jkwatson]

@jkwatson having any of these interfaces is a problem because a propagator or id generator needs to generate some objects then they will need to implement the interfaces.

An alternative is to do 3, make SC an abstract class or interface and provide multiple implementation (backed by longs by strings by etc.)

I'm not sure why having it be an interface, with the API providing default implementations is a problem. I don't think any of that would change...rather than using a long-based constructor, we would provide creation methods to return the default implementations, which would work for 99.9% of the use-cases. I think it would work similarly to what I've done in #1589 .

[trask]

I also think we are coming from different worlds, in my experience I never used log correlation (but if that is 90% of the time things may be reconsider)

I expect that auto-instrumentation will have log correlation enabled by default. I'm just not sure if it will be MDC-based or OpenTelemetry Log SDK-based.

[bogdandrutu]

I'm not sure why having it be an interface, with the API providing default implementations is a problem. I don't think any of that would change...rather than using a long-based constructor, we would provide creation methods to return the default implementations, which would work for 99.9% of the use-cases. I think it would work similarly to what I've done in #1589 .

Then, why do we want to do this:

1. Agent still needs to shade default implementations, no benefit
2. Same amount of allocations, no benefit
3. Even more wrappers and public classes/interfaces, even worse

[trask]

1. Agent still needs to shade default implementations, no benefit

See open-telemetry/opentelemetry-java-instrumentation#975

[trask]

I expect that auto-instrumentation will have log correlation enabled by default. I'm just not sure if it will be MDC-based or OpenTelemetry Log SDK-based.

Just re-iterating the point that @anuraaga made above, that MDC <-> Context synchronization will perform very poorly without (at least caching) a string representation of trace id and span id.

[bogdandrutu]

Just re-iterating the point that @anuraaga made above, that MDC <-> Context synchronization will perform very poorly without (at least caching) a string representation of trace id and span id.

This is a separate concern, we can simply use Strings in the current implementation instead of longs and be done with this issue.

[jkwatson]

I'm not sure why having it be an interface, with the API providing default implementations is a problem. I don't think any of that would change...rather than using a long-based constructor, we would provide creation methods to return the default implementations, which would work for 99.9% of the use-cases. I think it would work similarly to what I've done in #1589 .

Then, why do we want to do this:

1. Agent still needs to shade default implementations, no benefit
2. Same amount of allocations, no benefit
3. Even more wrappers and public classes/interfaces, even worse

Discussion with @trask has led me to believe that as long as there is an interface, the agent will have an easier time with the bridging, as it can provide a bridge implementation that would work. I'm prepared to be wrong on that, but that's my understanding from this issue in the instrumentation repo: open-telemetry/opentelemetry-java-instrumentation#975

[trask]

This is a separate concern, we can simply use Strings in the current implementation instead of longs and be done with this issue.

Why not also drop TraceId and SpanId classes in this case?

[bogdandrutu]

@trask see my comment:

An alternative is to do 3, make SC an abstract class or interface and provide multiple implementation (backed by longs by strings by etc.)

[trask]

An alternative is to do 3, make SC an abstract class or interface and provide multiple implementation (backed by longs by strings by etc.)

👍

Do we need multiple implementations? Is it ok to just back it by Strings (not longs) since we need the Strings anyways (at least for MDC Context synchronization), and converting String to long is only needed during binary propagation / OTLP export, and can be done on the fly pretty efficiently (zero allocation at least)?

[jkwatson]

Discussion during the APAC Tuesday meeting yielded the following decisions (for the next step):

1. Get rid of SpanId/TraceId wrappers
2. SpanContext will store base16 Strings internally, and accept them for creation purposes
3. IdGenerator will generate base16 String ids
4. SpanContext will expose both the base16 Strings, plus methods to get the byte[] (this is a spec requirement)
5. Open Question: do we validate IdGenerator created ids at runtime in the hot path, or not?
6. For the next step, we'll keep the SpanContext as an abstract class; we'll revisit conversion to an interface after.

[jkwatson]

Note: I have opened up a clean PR for further discussion: #1594

[jkwatson]

closing with #1594 as the official PR