Implement Trace Identifiers.

[carlosalberto]

This is done to implement Trace Identifiers, as covered in the RFC: https://github.com/opentracing/specification/blob/master/rfc/trace_identifiers.md

Probably the most important question is whether we should have traceId() and spanId() instead of tracerIdentifier() and spanIdentifier() respectively - probably the important bit here is that it could (as mentioned in the past) break Tracer implementations

(At this moment, doing this renaming would break MockTracer, as the current methods would have to be renamed to something like longTraceId() and longSpanId(). I'm definitely up for actually breaking it, but I wanted to get some feedback before ;) )

@yurishkuro @tedsuo @codingfabian @wu-sheng

[CodingFabian]

I would bikeshed for traceId() and spanId() because thats the signature of the methods we already implemented ;)

[coveralls]

Coverage increased (+0.8%) to 74.874% when pulling ab1f0364a296d677ae95b7a1da44097afeab3441 on carlosalberto:trace_identifiers_addition into d6c6509d5524f99039e34cbf121df79c46a25147 on opentracing:v0.32.0.

[yurishkuro]

I like the longer name, certainly less intrusive for existing implementations.

cc @pavolloffay @jpkrohling @objectiser

[jpkrohling]

whether we should have traceId() and spanId() instead of tracerIdentifier() and spanIdentifier()

As a consumer of the API, I prefer traceId() instead of tracerIdentifier(), but I wonder why it's not following the standard getTraceId() convention.

I'm definitely up for actually breaking it

+1, especially because we have not reached 1.0 yet.

[objectiser]

I would vote for getTraceId() and getSpanId() - and breaking is fine.

[hypnoce]

If we are ok to break things, I would go for traceId() and spanId() as it is the same naming conventions as the current version of the standard : https://w3c.github.io/distributed-tracing/report-trace-context.html#field-value As for using the get prefix, this is more something esthetic. Both I would suggest choose one option and stick to it throughout the API (my personnal choice here would be without the get prefix).

[objectiser]

@hypnoce get/set is a Java bean convention for properties - and is used in the current Java API, e.g. getBaggageItem, setOperationName, etc.

[hypnoce]

@objectiser Indeed it's a specification of Java Beans. They serve a specific purpose. Nevertheless, this convention is not consitently respected in the API https://github.com/opentracing/opentracing-java/blob/58846260a696f3cc06ae7c4ef1798d95a170300c/opentracing-api/src/main/java/io/opentracing/SpanContext.java#L35

From what I could see throughout the API, I beleive when there is a setter then the Java bean convention is used. If there is only a getter, the method has the same name as the property.

Also, it seems that java is moving out of getters and setters for data objects : http://cr.openjdk.java.net/~briangoetz/amber/datum.html

[carlosalberto]

@wu-sheng @adriancole @felixbarny Something to say on this? ;)

[carlosalberto]

Also pinging @tylerbenson as the author of DataDog's implementation ;)

[tylerbenson]

Datadog's implementation has the following methods:

long getTraceId()
long getSpanId()

So the return type will conflict either way. Considering the interface already has baggageItems() (as @hypnoce points out), I lean towards not prefixing with get. That is also more consistent with other method names in the API like Scope.span() and Span.context().

[yurishkuro]

I would also vote to not use get prefix. And using the long form Identifier avoids unpleasant clashes with existing internal APIs.

[tylerbenson]

I forgot to mention that before... I actually prefer sticking with Id. Long form is kind of grating to me.

[felixbarny]

I'm fine with either of the method names, but would prefer traceId() and spanId() to align with TraceContext. FWIW, the Elastic APM OT bridge does not currently offer ways to access the IDs so for us, it would not break anything.

Glad to see this change coming!

[carlosalberto]

Trying to re-take this discussion: it looks we are slightly more towards traceId() and spanId().

Something to add anybody? Something to add @yurishkuro ? ;) Else, I will update the PR (including MockTracer) in the next days.

[carlosalberto]

Hey all,

Have updated the trace/span identifiers to match the described requirements. Let me know ;)

[hypnoce]

Hey @carlosalberto Thanks for the update.

Trying to re-take this discussion: it looks we are slightly more towards traceId() and spanId().

The to prefix is to avoid breaking the API ?

[tylerbenson]

@carlosalberto I'm confused... I thought we decided on traceId() and spanId(). Where did we decide to add the to prefix?

[carlosalberto]

@tylerbenson Hey Tyler

Sorry for the PR without a previous explanation - so after discussing it on the Cross-Language call last Friday, based the fear of both breaking tracers and to signal users that retrieving such Ids may incur in object allocation, we decided to go this way.

Of course, this is not set in stone, so we can have a few iterations and pondering on this item ;) Let us know.

[carlosalberto]

@hypnoce Yes, and also to signal potencial allocation when converting the ids in whatever format they have natively to String ;)

[carlosalberto]

After https://github.com/opentracing/specification/pull/125 I'm wondering if we could have this one merged (to the v0.32.0 branch, that is, which is where experimental new features are supposed to land).

@yurishkuro @tylerbenson @objectiser @felixbarny @hypnoce

[carlosalberto]

Hey all,

Went ahead and clarified in the docs the meaning of an empty String, but without mention of the need of having both of them present or none (to keep things flexible).

Let me know if this is good to merge

cc @objectiser @yurishkuro @pavolloffay @tedsuo

[carlosalberto]

@pavolloffay @objectiser Small fixes done ;)

[carlosalberto]

Hey all!

Shall we merge this PR?

[hypnoce]

Hi all,

I have a small question regarding the format of the strings returned : do they have to obey the w3c format (hex strings) ? Or it's implementation detail ? Since the w3c is mentioned in the spec, I was wondering.

Thanks

[tedsuo]

@hypnoce there is no restriction. Basically, this should return the “ToString()” equivalent of whatever your tracer’s native ID’s are.

I would like to add w3c specific methods later, but that spec is more complex than just ID’s and it’s not really adopted yet. These accessors are meant to work with any tracing system which supports OT.

[hypnoce]

@tedsuo thanks for the update

[carlosalberto]

Hey all,

Unless there's somebody against it, I will be merging this PR in the next days (it has already been approved by a pair of members of the community, and it has received proper tuning ;)

[carlosalberto]

@yurishkuro Done. Thanks again for the latest feedback. Let me know if this is good to go - else, let me know so I can tune docs a little bit more ;)

[carlosalberto]

@yurishkuro Last edit is done. If this is ok, I will go ahead and merge later today ;)

[yurishkuro]

🚢

[carlosalberto]

Merged :)