Opentelemetry-java: Too many wrapper objects?

[anuraaga]

Is your feature request related to a problem? Please describe.

Currently there are many wrapper objects like TraceId, SpanId. In other languages these may be cheap but in Java, allocations do affect app performance and instrumentation needs to take efforts to reduce the burden on an app. Lately there's some buzz on the interwebs about this topic folks may have seen :)

Describe the solution you'd like

These are mostly attributes of a span's context. It seems like SpanContext could store the values like the long IDs directly and provide any needed helpers like hex encoding.

Has this been given any thought? I found this but it doesn't seem to provide concrete motivation for wrappers.

https://github.com/open-telemetry/opentelemetry-java/issues/12

SpanId seems particularly aggregious, it's effectively just a Long.

https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/SpanId.java

Consider how much effort JVMs have put in to avoiding boxing as much as possible, it seems a bit weird to me that we are boxing everything by default.

[bogdandrutu]

I agree with your concern, but we try to not expose the internal representation of the SpanId for example. We should think about an API that does not expose the internal details but may achieve what you suggest.

[carlosalberto]

Something interesting here is how we would proceed with cases where TraceId and Spanid exist by themselves (such as in the sampling API).

[bogdandrutu]

@carlosalberto one option is to expose all methods on the SpanContext instead of exposing them on their own objects. I would like to see a prototype before making a decision

[trask]

this would help efficiency of auto-instrumentation interop with manual instrumentation as well, where we currently have to wrap all of these objects individually

[jkwatson]

@carlosalberto one option is to expose all methods on the SpanContext instead of exposing them on their own objects. I would like to see a prototype before making a decision

I'd be up for prototyping this idea. Do you have an idea of how to limit the scope of the prototype? This will be a very large change, in general.

[bogdandrutu]

@trask

this would help efficiency of auto-instrumentation interop with manual instrumentation as well, where we currently have to wrap all of these objects individually

Is there any way you can avoid wrapping and shading the API classes? Not an expert but it seems that a lot of wrapping/shading happens in auto-instrumentation.

[bogdandrutu]

@jkwatson for me a successful prototype:

• New SpanContext public API
• One propagator to see how can we efficiently decode from wire
• IdGenerator in SDK - just how the interface looks like
• How SpanBuilderSDK can construct a new SpanContext

The change does not have to build, I want to see only these 4 files (SpanContext.java/SpanBuilderSdk.java/w3cprop/idgenerator)

[jkwatson]

@jkwatson for me a successful prototype:

• New SpanContext public API
• One propagator to see how can we efficiently decode from wire
• IdGenerator in SDK - just how the interface looks like
• How SpanBuilderSDK can construct a new SpanContext

The change does not have to build, I want to see only these 4 files (SpanContext.java/SpanBuilderSdk.java/w3cprop/idgenerator)

Cool. I started poking at it this a bit yesterday, and ran into exactly those 4 high-level items. I'll make a run at it soon.

[trask]

@trask

this would help efficiency of auto-instrumentation interop with manual instrumentation as well, where we currently have to wrap all of these objects individually

Is there any way you can avoid wrapping and shading the API classes? Not an expert but it seems that a lot of wrapping/shading happens in auto-instrumentation.

this design document hopefully gives context why these are needed, happy to answer additional questions about it

[jkwatson]

I don't think it's really a good approach, but I did the experiment to see what it would look like if we just used raw byte arrays for spanId and traceId. The fact that arrays are 100% mutable makes it a non-viable path, I think. See #1371 for the experiment.

[jkwatson]

I made #1371 very slightly better by only exposing copies of the byte arrays on the SpanContext API.

[jkwatson]

So, I've been doing a lot of thinking about this in the past couple of days.
A couple of givens:

1) Both on the input (context extraction), and the output side (export, context injection) of the SpanContext, spanId and traceId need to be individually accessible and generatable.
2) SpanId is pretty easy, as it's just 8 bytes, which fits snugly into a long.
3) TraceId is a problem, as it's 16 bytes, and Java has no native 16-byte immutable datatypes, requiring either creating a wrapper object, or copying on every access.

One option would be to use read-only java.nio.ByteBuffers to store this data, but creating a readonly ByteBuffer to wrap the byte[] involves copying the incoming byte[] as well, and involves a bunch of extra allocation cruft, for such small data structures.

I think the only immutable java-native datatype that fits the bill to hold traceId is a String, which is inherently immutable.

So the only way I can see this working feasibly is to use a String representation for spanId and traceId. Then, I see two options:

1) We cram the raw bytes directly into a String, using the String as the byte-wrapper. Upside: pretty simple, downside: people will probably assume that the String is the base64 encoded String, and try to print it out, yielding ugliness everywhere.
2) We use the base64-encoded representation of the bytes as our internal representation, and eat the cost when needing to covert to or from raw bytes. The upside of this, is, of course, those wire formats that want this as their representation get a super cheap representation with no conversion needed.

There is probably another option that I haven't considered, but that's where I am right now.

[jkwatson]

One more crazy thought, while I'm brainstorming on this... If we used off-heap direct ByteBuffers to store the ids, then we get immutability via the copy from the heap byte[] into the off-heap bytes. I haven't done a ton of off-heap programming, and it would impose a bit of a burden to make sure enough off-heap memory is allocated to the JVM at startup. Probably a non-starter, but a good thought-experiment anyway.

[anuraaga]

Hex-encoded String seems nice because it's what everyone is used to seeing in UI, etc (I suspect). It also probably has beneficial effects when marshalling with text headers where the string just gets used as is, all the text formats out there use hex encoding of the IDs.

[anuraaga]

Just for reference, prior art in Brave

https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/handler/MutableSpan.java#L130
https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/TraceContext.java

Interestingly, TraceContext stores longs mostly for traditional reasons but we are thinking of changing those to strings by default too - most people don't really care about the longs so we went with string in the newer MutableSpan.

[jkwatson]

some comments about traceid/spanid: https://github.com/open-telemetry/opentelemetry-java/pull/1374#issuecomment-680234892

[anuraaga]

Since we have issues for the non-ID stuff, I think we can close this, @jkwatson sound good?

[jkwatson]

Do we have an issue for AttributeValue?

[anuraaga]

We have #1580 for it I think.

[jkwatson]

We have #1580 for it I think.

perfect. I'll close this, then! Thanks everyone for helping make this happen!