Improvements to Span, SpanContext and introduction of SpanId, TraceId

[indrekj]

No description provided.

[fbogsany]

I’m not sure I understand the value of the TraceId and SpanId classes. They simply wrap integers, increasing the cost of allocating a SpanContext in the default case. Does the abstraction offer significant benefits? Does it impose unnecessary restrictions on vendor implementations?

[fbogsany]

Why does this need to be protected?

[indrekj]

In java implementation they implement the id as an integer but expose only the conversion methods (e.g. toLowerBase16): https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/SpanId.java . I currently did the same thing here. I used protected to be still able to compare two Ids in #==. If we expose the integer representation of the id then it probably could use a better name (e.g. to_i)

[fbogsany]

Why does this need to be protected?

[indrekj]

Does the abstraction offer significant benefits? Does it impose unnecessary restrictions on vendor implementations?

It allows easily to change Id internals. e.g. we could represent trace id with two integers high & low like https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/TraceId.java#L57 and still pass along one object.

I think the main idea is that different actors expect different representations of the Ids. Vendors usually expect ints, but users expect base16.

I as a end-user would expect to easily be able to get the base16 representation (e.g. to use it in my logs). span.context.span_id.to_lower_base16 seems quite convenient.

So far it looks like java and dotnet implementations are doing the wrapping. Dotnet has ToHexString and java has toLowerBase16().

I do see you're point though that this will cause an extra object allocation. I'm open to ideas.

[mwear]

Does the abstraction offer significant benefits? Does it impose unnecessary restrictions on vendor implementations?

It allows easily to change Id internals. e.g. we could represent trace id with two integers high & low like https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/TraceId.java#L57 and still pass along one object.

I think the main idea is that different actors expect different representations of the Ids. Vendors usually expect ints, but users expect base16.

I as a end-user would expect to easily be able to get the base16 representation (e.g. to use it in my logs). span.context.span_id.to_lower_base16 seems quite convenient.

So far it looks like java and dotnet implementations are doing the wrapping. Dotnet has ToHexString and java has toLowerBase16().

I do see you're point though that this will cause an extra object allocation. I'm open to ideas.

Research from other OTel Projects

I took at look at opentelemetry-js and opentelemetry-python to see how they handle ID generation. Here's what I found:

opentelemetry-js generates IDs using a function called crypto.randomBytes. The Ruby equivalent would be SecureRandom.hex, which depending on our needs, could be something worth considering. That is, if rand() is not going to be random enough for some situations. You can see id.ts here. I'll paste a portion below as well:

/**
 * Returns a random 16-byte trace ID formatted/encoded as a 32 lowercase hex
 * characters corresponding to 128 bits.
 */
export function randomTraceId(): string {
  return randomSpanId() + randomSpanId();
}

/**
 * Returns a random 8-byte span ID formatted/encoded as a 16 lowercase hex
 * characters corresponding to 64 bits.
 */
export function randomSpanId(): string {
  return crypto.randomBytes(SPAN_ID_BYTES).toString('hex');
}

For Python, I searched through the repo and open PRs, but surprisingly couldn't find id generation code. I don't think I missed anything; I think that code has yet to be written. I can keep an eye out for when it appears for another data point though.

Actual Feedback

At the expense of an object allocation per Span, I think arguments can be made for or against the SpanId and TraceId classes. On one hand, if users regularly use the conversion methods, it makes sense to include this abstraction. On the other hand, if your average user doesn't use them, we'd be forcing an allocation for each span on them with no benefit.

I don't have a super strong opinion on this, but in general, I would try to limit per span allocations as much as possible. In terms of introducing the SpanId and TraceId abstraction, I would generally go with a minimal implementation first and see how that works out. If it becomes clear there is a need for additional functionality, add it then.

[indrekj]

Closing this as it is not relevant atm. Can be reopen in future if needed.