opentracing / specification

A place to document (and discuss) the OpenTracing specification. 🛑 This project is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io/spec
Apache License 2.0
1.17k stars 182 forks source link

Change method siganture to ToTraceID and ToSpanID #125

Closed tedsuo closed 5 years ago

tedsuo commented 5 years ago

This update to the Trace Identifiers RFC addresses issues identified in the last round of exploration. It changes the method signatures from TraceID and SpanID to ToTraceID ToSpanID. A small additional clarification on future formats is also included.

Why ToTraceID instead of TraceID?

The method names ToTraceID and ToSpanID were chosen over TraceID and SpanID for two reasons:

1) To avoid conflicts with tracer implementations, which often use TraceID and SpanID to expose their identifiers in their native format. 2) To communicate to the user that a format conversion may be occuring, and thus there may be some cost to calling these methods.

yurishkuro commented 5 years ago

@tedsuo there were also discussions on some PRs about the range of values returned from those methods, like having them to be a hex string, or being URL-safe. I don't think we had a detailed discussion about that.

Arguments for hex string - the premise of this whole RFC was the agreement reached at W3C WG between all vendors that they should be able to express their trace/span IDs as opaque, 16 and 8 byte arrays respectively. This was different from the original design of SpanContext when there was no such agreement and there as a concern that some vendors wouldn't be able to expose the IDs (btw maybe worth adding this explanation to the RFC). So given the W3C agreement as a premise, why not follow its format recommendation as well and be more prescriptive about the outputs of ToTraceID/ToSpanID ?

tedsuo commented 5 years ago

@yurishkuro I think that is an interesting point. Do existing systems all conform to the new W3C standard for identifiers format?

There are separate design decision around adding direct access to W3C Trace-Context data. It would require at least the following:

My suggestion would be that we add ToTraceID and ToSpanID as loosely defined as possible. That could still mean setting a maximum expected length and defining a character set. But if we want to expose the Trace-Context data format specifically, it should be a separate RFC, as the requirements for that are very different.

I'm fine with adding this interface now, and a second one later. But if we want to instead drop this RFC in favor of just having a TraceContext accessor, we should move very quickly on making a proposal for that.

In any case, I would suggest keeping this PR tightly scoped to just updating the naming convention, since that's a separate issue which I feel we have resolved. We can make a follow up PRs discussing content restrictions on the returned value, etc.

yurishkuro commented 5 years ago

So my concern was not about adopting W3C proposal, but about setting the expectations on the output values. These expectations may take the form of a recommendation, rather than a requirement. For example, one of the reasons people want trace IDs is to log them. Logs are often formatted in JSON. So if suddenly the output value of ToTraceID is a string with a double-quote in it (for the sake of argument), then it makes JSON more difficult.

If the output format is only recommendation, consumers still need to take appropriate measures to encode the values properly in whatever medium they are serializing them (e.g. URL-encoding, etc.) But at least a recommendation of a hex string makes the encoding no-op in most cases, and even allows straightforward conversion to binary. Once W3C finalizes the proposal (and it doesn't seem likely that hex format will be overturned there), most OT tracers will be already exposing IDs in the compatible format.

Anyway, I am fine with not prescribing any string format at this stage.