An RFC for SpanContext, References, as well as changes to carriers #32

bhs · 2016-07-13T05:40:11Z

I would love feedback about the API here... once we converge, we can decide what to do about the rest of this (github) repo (the impl helpers, etc).

Please look at #31 and opentracing/opentracing.io#99 for background and motivation.

I'll add a few PR comments inline as well.

bhs · 2016-07-13T05:40:44Z

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

+ * @see io.opentracing.Tracer#inject(SpanContext, Format, Object)
+ * @see io.opentracing.Tracer#extract(Format, Object)
+ */
+public class BuiltinFormats<R, W> implements Format<R, W> {


@yurishkuro I went with BuiltinFormats instead of Formats -- seemed to better emphasize that non-builtin extensions were feasible.

bhs · 2016-07-13T05:50:08Z

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

-   * undefined behavior.
-   */
-  void finish();
+    /**


(indenting was off throughout this file)

codefromthecrypt · 2016-07-13T05:55:17Z

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

+
+/**
+ * BuiltinFormats encapsulates the inject()/extract() carrier formats that all Tracer implementations must support.
+ *


probably worth it to add docs for these generic params.. like..

* @param <R> foo used by {@link foo#bar} to bar things * @param <W> baz used by {@link baz#qux} to qux things

michaelsembwever · 2016-07-13T10:27:36Z

Without the Format stuff from #31 i'm +1 on merging.

With the Format stuff it's currently a -1 from me, unless further discussion helps me see the light.

yurishkuro · 2016-07-13T15:03:50Z

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

+    private Type type;
+    private SpanContext referee;
+
+    public enum Type {


I am ok with this being an enum, as a list of build-in reference types, but the type field in the Reference class needs to be of a comparable type, not this enum. Otherwise this is not extensible without OT API release.

we talked about this briefly off github -- I've gone with Comparable where reference types are needed and am using Strings for the builtin ones.

bhs · 2016-07-13T20:28:20Z

Meta-comment: I'm going to try and split the (controversial) Format stuff out of this PR inasmuch as that's possible. We can then talk about the SpanContext and Reference aspects in isolation. I'll try to make those changes in the next 4 hours or so.

The big picture in this commit: - Get rid of the contentious stuff around Format for this PR - Just define reference *types*; no need for a Reference class given Java's SpanBuilder approach - Add an HttpHeader* carrier so we can discuss that properly - Cleanups

bhs · 2016-07-14T00:09:49Z

Thanks so much for the comments, all, PTAL.

codefromthecrypt · 2016-07-14T01:34:39Z

my non-binding opinion is that the type complexity of Format<R, W> isn't worth putting into the base api. It would end up being wrapped I think.

Complex apis in java usually breed libraries that hide their complexity. If we want to avoid some side-project called OpenTracing-Lite, I'd suggest highly that we simplify as much as possible the front-door of this api.

codefromthecrypt · 2016-07-14T03:50:30Z

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

       */
-      SpanBuilder withParent(Span parent);
+      SpanBuilder addReference(Comparable referenceType, SpanContext referencedContext);


if using Comparable, we should provide rationale for the choice, for example, why ordering matters. It is distracting otherwise.

A guess is that we are expecting an implementation to have multiple parents and need to know the parent vs adopted vs step-parent? I suppose we are expecting the implementation to retain the order?

Another note, Comparable has a type parameter <T> which would lead to this method having a compiler warning.

Yeah, I'll just change to Object. I was responding to an earlier comment about the downside of using the (now obliterated) Reference.Type enum, as enums by definition are not externally extensible. I don't think the use case for a global ordering nearly compelling enough... we really just want equality comparisons.

tinkerware · 2016-07-14T19:58:06Z

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

       */
-      SpanBuilder withParent(Span parent);
+      SpanBuilder addReference(Object referenceType, SpanContext referencedContext);


I understand reference types need to be extensible, but is there a need to make them arbitrary types? Why not String?

If all we need is equals(), I guess I would ask why String?

If we had a Symbol type in Java, it would be that. Failing that, isn't it better that we at least pick something that can be switch'ed on and has value semantics?

good point on switching. enum couldn't be used bc enums aren't extensible. for switch, we're left with ints or strings, strings require java 7+ (which I think this library does anyway)

this is a String now.

bhs · 2016-07-14T20:14:17Z

@adriancole @michaelsembwever @yurishkuro @tinkerware

Since the (controversial) Format stuff has been split off from this PR, are there any objections to the remaining content? It seemed like we'd converged on the SpanContext / Reference / extract() aspects presented here.

I'm presently fixing the rest of the (github) repo to work along with these changes... it does require some surgery here and there but most of the diff will be uncontroversial. It's also far less important since OT callers don't depend on any of the impl helpers directly, of course.

Setting expectations: I'd like to merge this PR in 24-36h or so unless substantial new issues are raised.

tinkerware · 2016-07-14T20:24:12Z

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

   */
-  <T> void inject(Span span, T carrier);
+  <C> void inject(SpanContext spanContext, C carrier);


What's the point to the C type parameter when it does not constrain anything? If we are saying that a more elaborate format API is later work, we can just leave the carrier type Object for now, though it seems to me that there could at least be a marker interface for carriers.

I agree, this C makes it look like there's more to the carrier type than just an Object. +1 to drop it.

This was inherited from the code at master...

https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/Tracer.java#L53

Aside from making the documentation more explicit (@param <C> ...), I can't provide any particular justification. I think this is code @michaelsembwever wrote, so I might be missing part of the rationale. (The documentation aspect is not trivial, though...)

It helps implementations. See AbstractTracer for example.
It also creates a stub to explain Carrier types.

No biggie either way for me.

good point. I think that parameter doesn't make sense here since it doesn't bind any other parameters or the return type.

tinkerware · 2016-07-14T20:28:15Z

This seems okay to me, assuming that carrier/format API is not in its final form.

yurishkuro · 2016-07-14T20:52:53Z

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


    Span http = tracer.buildSpan("HandleHTTPRequest")
-                      .withParent(feed)
+                      .asChildOf(parentSpan.context())


@bensigelman similar to Node/Python, we can overload this to asChildOf(parentSpan)

yurishkuro · 2016-07-14T21:02:13Z

lgtm

michaelsembwever · 2016-07-14T22:45:50Z

👍

dawallin · 2016-07-14T23:43:30Z

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

+     * @return the value of the baggage item identified by the given key, or null if no such item could be found
+     */
+    String getBaggageItem(String key);
+}


In go a method to iterate through all baggage items were introduced. Should you have it here as well ?

I would like this to happen in a followup... there are some subtleties that you brought up re immutability that I think are relevant here (namely, if we know that SpanContexts are immutable, the interface can be zero-copy and return an iterator and be threadsafe... otherwise I'd rather do something like what we did in Go and have the SpanContext impl invoke a callback repeatedly).

(ed: I misremembered and thought tinkerware had brought up the immutability issue -- it was @dawallin!)

Much of the opentracing-impl stuff is dated to when we separated tracer state and baggage. I've removed some of it and I think an argument could be made to get rid of the rest, here, too; or maybe turn it into basictracer-java or similar.

Make AbstractSpan abstract again (i.e., let subclasses handle context()).

bhs · 2016-07-15T04:27:58Z

Okay -- I changed a bunch of stuff and fixed the build and tests in the rest of this repo (including NoopTracer, which is probably the only part anyone actually uses at this point).

bhs · 2016-07-15T04:28:45Z

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

+     *
+     * @return the SpanContext that encapsulates Span state that should propagate across process boundaries.
+     */
+    SpanContext getContext();


I changed this to getContext() since this is Java :)

(I'd internalized Go's no-get getter convention, I guess)

going without a getX is not abnormal when there is no setter. A lot of libraries follow this, including zipkin. I prefer is as context()

ps the history of getX setX is JavaBeans. this isn't a java bean, so doesn't need to behold to that. A lot of the api has more to do with builders than beans http://www.informit.com/articles/article.aspx?p=1216151&seqNum=2

bhs · 2016-07-15T04:38:24Z

(and if there are no further concerns, I will merge this sometime tomorrow)

bhs · 2016-07-15T15:40:39Z

@adriancole I dropped the getContext() commit. Thanks for the history lesson, too. :)

bhs · 2016-07-15T22:48:58Z

Okay, as promised, I'm merging this now. Thanks for all of the feedback.

#31 and opentracing/opentracing.io#106 are still open and could impact this API going forward.

Apparently IntelliJ decided to "intelligently" (sic) move some files to the wrong directories, resulting in some noise here. This change re-introduces much of the type-enforcement lost during #32

Ben Sigelman added 2 commits July 12, 2016 21:10

API changes without updated comments

0917928

Add comments and clean things up a little.

5c32439

bhs reviewed Jul 13, 2016
View reviewed changes

Add a trivial comment and fix indenting

3630064

bhs reviewed Jul 13, 2016
View reviewed changes

bhs mentioned this pull request Jul 13, 2016

Introduce carrier format identifiers for parity with other platforms #31

Closed

codefromthecrypt reviewed Jul 13, 2016
View reviewed changes

yurishkuro reviewed Jul 13, 2016
View reviewed changes

yurishkuro mentioned this pull request Jul 13, 2016

Discrepancy: Join can create a new trace in Java, but not in Go/Python #26

Closed

Respond to PR comments

ba3be7b

The big picture in this commit: - Get rid of the contentious stuff around Format for this PR - Just define reference *types*; no need for a Reference class given Java's SpanBuilder approach - Add an HttpHeader* carrier so we can discuss that properly - Cleanups

codefromthecrypt reviewed Jul 14, 2016
View reviewed changes

Ben Sigelman added 3 commits July 13, 2016 21:19

Respond to reviewer comments

592a452

Switch from Iterable to Iterator

a3b54a7

Get some of the non-core-api stuff compiling again

35337f1

tinkerware reviewed Jul 14, 2016
View reviewed changes

Get tests to compile without registration support

392abc5

tinkerware reviewed Jul 14, 2016
View reviewed changes

yurishkuro reviewed Jul 14, 2016
View reviewed changes

dawallin reviewed Jul 14, 2016
View reviewed changes

bhs mentioned this pull request Jul 15, 2016

RFC: SpanContext should be immutable opentracing/opentracing.io#106

Closed

Ben Sigelman added 2 commits July 14, 2016 21:26

Simplify, simplify.

407854c

Much of the opentracing-impl stuff is dated to when we separated tracer state and baggage. I've removed some of it and I think an argument could be made to get rid of the rest, here, too; or maybe turn it into basictracer-java or similar.

Adjust to some typing changes

2f92013

Make AbstractSpan abstract again (i.e., let subclasses handle context()).

bhs force-pushed the bhs/span_context_etc branch from a72e548 to ff2d9ff Compare July 15, 2016 04:27

bhs reviewed Jul 15, 2016
View reviewed changes

Use a String for the reference type

623db61

bhs force-pushed the bhs/span_context_etc branch from 0e5a342 to 623db61 Compare July 15, 2016 15:39

bhs merged commit b0d7b07 into master Jul 15, 2016

bhs deleted the bhs/span_context_etc branch July 15, 2016 22:50

bhs mentioned this pull request Jul 16, 2016

Make LightStep's Tracer fall in line with OpenTracing changes lightstep/lightstep-tracer-java#18

Merged

bhs mentioned this pull request Jul 24, 2016

introduce a Format type in Java #33

Merged

An RFC for SpanContext, References, as well as changes to carriers #32

An RFC for SpanContext, References, as well as changes to carriers #32

Conversation

bhs commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsembwever commented Jul 13, 2016

yurishkuro Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhs commented Jul 13, 2016

bhs commented Jul 14, 2016

codefromthecrypt commented Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt Jul 14, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinkerware Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

codefromthecrypt Jul 15, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhs commented Jul 14, 2016

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

codefromthecrypt Jul 15, 2016 via email

Choose a reason for hiding this comment

tinkerware commented Jul 14, 2016

Choose a reason for hiding this comment

yurishkuro commented Jul 14, 2016

michaelsembwever commented Jul 14, 2016

Choose a reason for hiding this comment

bhs Jul 15, 2016 • edited Loading

Choose a reason for hiding this comment

bhs commented Jul 15, 2016

Choose a reason for hiding this comment

codefromthecrypt Jul 15, 2016 via email

Choose a reason for hiding this comment

codefromthecrypt Jul 15, 2016 via email

Choose a reason for hiding this comment

bhs commented Jul 15, 2016

bhs commented Jul 15, 2016

bhs commented Jul 15, 2016

yurishkuro Jul 13, 2016 •

edited

Loading

codefromthecrypt commented Jul 14, 2016 •

edited

Loading

tinkerware Jul 14, 2016 •

edited

Loading

bhs Jul 15, 2016 •

edited

Loading