Implement Trace Identifiers. #280

carlosalberto · 2018-06-12T18:25:05Z

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 · 2018-06-12T18:28:46Z

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

coveralls · 2018-06-12T18:29:36Z

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

yurishkuro · 2018-06-12T18:48:49Z

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

cc @pavolloffay @jpkrohling @objectiser

jpkrohling · 2018-06-13T07:46:54Z

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 · 2018-06-13T07:56:31Z

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

hypnoce · 2018-06-13T11:41:15Z

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 · 2018-06-13T13:23:25Z

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

hypnoce · 2018-06-13T14:32:24Z

@objectiser Indeed it's a specification of Java Beans. They serve a specific purpose. Nevertheless, this convention is not consitently respected in the API

opentracing-java/opentracing-api/src/main/java/io/opentracing/SpanContext.java

Line 35 in 5884626

Iterable<Map.Entry<String, String>> baggageItems();

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 · 2018-06-20T18:00:17Z

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

carlosalberto · 2018-06-20T18:02:12Z

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

tylerbenson · 2018-06-21T04:26:58Z

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 · 2018-06-21T04:47:26Z

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

tylerbenson · 2018-06-21T04:54:03Z

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

felixbarny · 2018-06-21T12:05:37Z

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 · 2018-07-04T01:47:51Z

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 · 2018-07-25T17:59:16Z

Hey all,

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

opentracing-api/src/main/java/io/opentracing/SpanContext.java

hypnoce · 2018-07-26T19:44:10Z

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 · 2018-07-26T23:39:49Z

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

carlosalberto · 2018-07-27T01:40:10Z

@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 · 2018-07-27T01:41:04Z

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

carlosalberto · 2018-08-28T04:58:52Z

After opentracing/specification#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

objectiser · 2018-08-29T08:54:58Z

opentracing-api/src/main/java/io/opentracing/SpanContext.java

+     *
+     * Globally unique. Every span in a trace shares this ID.
+     *
+     * The empty string is a valid return value, while null is not.


Do we need to clarify what an empty string represents? i.e. no active trace instance.
Also do we need other text that indicates the return value of toTraceId and toSpanId are either both empty strings (no active trace) or both have ids? For example, I don't think it would be valid to have an empty trace id but a value returned from span id?

Oh, that's a good point. IMHO it would make sense to mention that the empty string ought to represent missing Trace Identifiers info (and, if this gets approved, make also the note about Span ID & Trace ID working together).

Sounds like a good plan?

cc @yurishkuro @tedsuo

iirc the original idea was to allow empty string to tracers that are unable to provide a single trace id (e.g. "do not support them", or more likely "this span has many trace IDs").

I would recommend we keep the rules simple, and just say that either method may return an empty string, meaning that the method is not supported for that span.

I doubt implementations will provide one but not the other, but there's no need to make it a rule.

pavolloffay · 2018-09-04T08:58:29Z

opentracing-api/src/main/java/io/opentracing/SpanContext.java

+     *
+     * The empty string is a valid return value, while null is not.
+     *
+     * @return the trace ID for this context.


Could you please also add something like , or empty if the trace ID does not exist?

carlosalberto · 2018-09-05T22:37:48Z

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

pavolloffay · 2018-09-06T07:14:46Z

opentracing-noop/src/main/java/io/opentracing/noop/NoopSpanBuilder.java

@@ -80,6 +80,5 @@ public Span startManual() {
        return NoopSpanImpl.INSTANCE;
    }

-    @Override


why is this removed?

Damn, my bad.

objectiser · 2018-09-06T07:38:13Z

opentracing-mock/src/main/java/io/opentracing/mock/MockSpan.java

        public long traceId() { return traceId; }
        public long spanId() { return spanId; }

+


nit: should remove this line

carlosalberto · 2018-09-06T14:41:07Z

@pavolloffay @objectiser Small fixes done ;)

carlosalberto · 2018-09-10T16:43:02Z

Hey all!

Shall we merge this PR?

hypnoce · 2018-09-17T19:17:30Z

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 · 2018-09-26T17:03:31Z

@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 · 2018-09-26T17:05:34Z

@tedsuo thanks for the update

carlosalberto · 2018-09-27T10:58:10Z

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 ;)

yurishkuro · 2018-09-12T13:48:01Z

opentracing-api/src/main/java/io/opentracing/SpanContext.java

+     *
+     * Globally unique. Every span in a trace shares this ID.
+     *
+     * An empty String will be returned if there is no method implementation


"no method implementation" reads weird. So is "no trace id present".

Maybe we can simply say "empty string may be returned if the tracer does not support this functionality" and give examples of noop tracer and metrics decorator.

yurishkuro · 2018-09-12T13:51:26Z

opentracing-api/src/main/java/io/opentracing/SpanContext.java

+    /**
+     * Return the ID of the trace.
+     *
+     * Globally unique. Every span in a trace shares this ID.


Globally unique is incorrect. It is impossible to guarantee (random clashes are acceptable), so we should not state is as a requirement, or we should use SHOULD.

I would only say that it SHOULD be the same for all spans in a single trace.

yurishkuro · 2018-09-12T13:54:21Z

opentracing-api/src/main/java/io/opentracing/SpanContext.java

+     * Unique within a trace. Each span within a trace contains a different ID.
+     *
+     * An empty String will be returned if there is no method implementation
+     * or there is no span ID present. null is an invalid value return value.


Similar comments about wording

yurishkuro · 2018-09-12T13:54:50Z

opentracing-api/src/main/java/io/opentracing/SpanContext.java

+    /**
+     * Return the ID of the associated Span.
+     *
+     * Unique within a trace. Each span within a trace contains a different ID.


SHOULD, not is.

carlosalberto · 2018-10-10T12:48:14Z

@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 ;)

yurishkuro · 2018-10-12T14:27:06Z

opentracing-api/src/main/java/io/opentracing/SpanContext.java

+     *
+     * Observe there is no restriction regarding the format of the returned value, as this
+     * should simply be the equivalent of calling toString() on the native ID object
+     * used by the tracer.


This wording is problematic. It says "should ... ToString" which in many cases is the wrong thing to do, e.g. for int64 IDs. If we want to make a recommendation, it would be that the string format SHOULD match the representation of the IDs in http headers.

The javadoc here should ideally be repeating word for word the specification.

Oh, good point and good example. So I feel that we should remove this clarification then (if later we add something in the specification itself, we can add it here). Would that make sense to you?

Yes, can remove it altogether

carlosalberto · 2018-10-12T15:26:03Z

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

yurishkuro · 2018-10-12T21:38:39Z

🚢

carlosalberto · 2018-10-14T18:15:58Z

Merged :)

* Deprecate the StringTag.set() overload taking a StringTag. (#262) * Implement Trace Identifiers. (#280) * Bump JaCoCo to use a Java 10 friendly version (#306) * Remove finish span on close (#301) * Deprecate finishSpanOnClose on activation. * Add ScopeManager.activeSpan() and Tracer.activateSpan(). * Clarify the API changes and deprecations. * Add an error reporting sample to opentracing-testbed. * Simple layer on top of ByteBuffer for BINARY format. (#276) * Add generic typed setTag/withTag (#311) * Allow injecting into maps of type Map<String,Object> (#310) * Add simple registerIfAbsent to global tracer (#289) * Split Inject and Extract Builtin interfaces (#316) * Deprecate ScopeManager.active() (#326) * Make Tracer extends Closable. (#329) * Do not make isRegistered() synchronized. (#333) * Deprecate AutoFinishScopeManager (#335)

Implement Trace Identifiers.

ffb9a6b

carlosalberto mentioned this pull request Jun 22, 2018

ActiveSpan is too Opaque #208

Closed

3 tasks

tedsuo mentioned this pull request Jul 11, 2018

RFC: Trace Identifiers opentracing/specification#123

Open

5 tasks

carlosalberto added 2 commits July 25, 2018 17:19

Merge branch 'v0.32.0' into trace_identifiers_addition

e90a34f

Rename trace identifiers to avoid possible collisions.

854f1fb

yurishkuro reviewed Jul 25, 2018

View reviewed changes

opentracing-api/src/main/java/io/opentracing/SpanContext.java Show resolved Hide resolved

Add a comment about returning empty string and never null.

3125b29

objectiser reviewed Aug 29, 2018

View reviewed changes

pavolloffay reviewed Sep 4, 2018

View reviewed changes

Clarify the meaning of empty strings for trace identifiers.

04daa73

pavolloffay reviewed Sep 6, 2018

View reviewed changes

objectiser reviewed Sep 6, 2018

View reviewed changes

objectiser approved these changes Sep 6, 2018

View reviewed changes

carlosalberto added 2 commits September 6, 2018 16:33

Restore the @OverRide for NoopSpanBuilder.toString()

59a7b73

Remove an extra space in MockSpan.

3af7040

pavolloffay approved these changes Sep 12, 2018

View reviewed changes

felixbarny mentioned this pull request Sep 13, 2018

Support for headers besides b3 stagemonitor/stagemonitor#420

Open

yurishkuro reviewed Oct 8, 2018

View reviewed changes

carlosalberto added 2 commits October 10, 2018 14:21

Use better wording uniqueness/availability for the inline docs.

4dfcca9

Clarify format/return value of the identifiers.

2dbf622

yurishkuro approved these changes Oct 12, 2018

View reviewed changes

Remove the clarification on the format.

ab1f036

carlosalberto merged commit 05e90e5 into opentracing:v0.32.0 Oct 14, 2018

austinlparker mentioned this pull request Oct 15, 2018

Should SpanContext use Java toTraceId/toSpanId signature? opentracing/opentracing-csharp#109

Open

objectiser mentioned this pull request Oct 23, 2018

SpanContext can put in MDC opentracing-contrib/java-spring-cloud#179

Closed

carlosalberto mentioned this pull request Oct 23, 2018

0.32.0 Release #314

Open

carlosalberto deleted the trace_identifiers_addition branch March 7, 2019 01:32

		public long traceId() { return traceId; }
		public long spanId() { return spanId; }

Implement Trace Identifiers. #280

Implement Trace Identifiers. #280

Conversation

carlosalberto commented Jun 12, 2018

CodingFabian commented Jun 12, 2018

coveralls commented Jun 12, 2018 • edited Loading

yurishkuro commented Jun 12, 2018

jpkrohling commented Jun 13, 2018

objectiser commented Jun 13, 2018

hypnoce commented Jun 13, 2018

objectiser commented Jun 13, 2018

hypnoce commented Jun 13, 2018 • edited Loading

carlosalberto commented Jun 20, 2018

carlosalberto commented Jun 20, 2018

tylerbenson commented Jun 21, 2018

yurishkuro commented Jun 21, 2018

tylerbenson commented Jun 21, 2018

felixbarny commented Jun 21, 2018 • edited Loading

carlosalberto commented Jul 4, 2018

carlosalberto commented Jul 25, 2018

hypnoce commented Jul 26, 2018

tylerbenson commented Jul 26, 2018

carlosalberto commented Jul 27, 2018

carlosalberto commented Jul 27, 2018

carlosalberto commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Sep 6, 2018

carlosalberto commented Sep 10, 2018

hypnoce commented Sep 17, 2018

tedsuo commented Sep 26, 2018

hypnoce commented Sep 26, 2018

carlosalberto commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto commented Oct 12, 2018

yurishkuro commented Oct 12, 2018

carlosalberto commented Oct 14, 2018

coveralls commented Jun 12, 2018 •

edited

Loading

hypnoce commented Jun 13, 2018 •

edited

Loading

felixbarny commented Jun 21, 2018 •

edited

Loading