In discrete steps, Simplify the API #7
Conversation
Rationale: The SpanContext holds context to a subtree in a Trace's tree of nodes. It is not created outside of a Trace and is not context to a Trace. SpanContext's attributes are - position of the subtree within the Trace's tree, specifically this is the parent Span's ID. This ID is not exposed in the API, but is required state for functionality. - a set of tags that all Spans underneath it are created with - an ability to be serialised: providing support for distributed context propagation.
Rationale: After TraceContext was renamed to SpanContext, the more intuitive and accurate classname "TraceContext" was available.
Rationale: TraceContext was renamed to SpanContext. While it might be appropriate to rename TraceContextCodec to SpanContextCodec, it's more more intuitive to simply call it TraceCodec. After all, there is an expectation that all SpanContexts within a given trace are encoded the same way.
Rationale:
This is only a convenience method.
The real way of doing this is
childContext = tracer.newChildContext(mySpan.traceContext);
childSpan = tracer.newSpanWithContext(childContext);
Adding it as a convenience method here complicates the serialisation of the Span.
Needing a convenience method suggests the real way of doing it isn't intuitive enough, so let's
try to address that first.
Rationale: None. Someone needs to explain to me the need to create from a SpanContext a child SpanContext without having a Span inbetween.
…sage, payload)` method. Rationale: This is the simplest solution, and an appropriate plumbing API, that solves the use-case of info/error and parameterised message formatting. OpenTracing API may adopt a porcelain API for those info/error logging-style methods down the road once the plumbing api has been battle tested a bit more.
| Span error(String message, Object... args); | ||
| * @todo | ||
| **/ | ||
| Span event(String message, Map<String,String> payload); |
There was a problem hiding this comment.
The api of payload is still unclear to me
ref: opentracing/opentracing.io#30 (comment)
There was a problem hiding this comment.
ack. before that comment, I'd guess @Nullable Object payload
There was a problem hiding this comment.
@adriancole that's the idea... it is an arbitrary object that implementations can record in total, in part, or not at all.
|
@michaelsembwever thank you for sending this out. Per gitter, we are not on the same page. It is apparent that either the goals of OpenTracing haven't been communicated well or you don't agree with them. :) Especially given the comments that have come up in this PR, what you propose here is not an API simplification: it removes entire swaths of semantic functionality. If that's intended, call a spade a spade. If it's not, imagine writing (or actually write if you have the time!) example calling code that
... all without peeking through to the OpenTracing implementation. As for naming (less important given the scope of the problems above):
|
Quite possibly. I am late to the game, but so will every subsequent person after me. My understanding of OpenTracing is
The second point implies that only the OpenTracing language-specific library is required as a compile-time dependency. Tracing platform specific instrumentation libraries become run-time dependencies. This permits the application to
The OpenTracing SPI then, as I've understood it, only concerns itself with what is imposed upon the application code. And the simpler that SPI is the greener the field we create. The OpenTracing Specification on the other hand will need to impose constraints upon the instrumentation libraries that the application code need not be concerned about. ( ¹ It's also possible to use an instrumentation library that delegates on to multiple other instrumentation libraries, useful for example when blue-green deploying tracing platforms. )
There's a difference between "peeking through" the OpenTracing implementation, and using that OpenTracing SPI. The first list of bulleted examples cover things that instrumentation libraries need to do, not things the application code needs to do. So there is the OpenTracing specification, the OpenTracing SPI, the tracing platform, and then the tracing instrumentation libraries to each platform. So I would say: don't fall into the trap of thinking we must define all the OpenTracing Specification into the OpenTracing SPI. That is, the SPI isn't the universal documentation (nor a set of constraints) for the different instrumentation libraries to adhere to; that's the role of the Specification. In this manner of thinking, there will also be other constraints impose by the specific tracing platforms upon their instrumentation libraries that are not of concern to either OpenTracing's Specification or its SPI. Going beyond this I can see that the OpenTracing API consists of three separate parts
I hinted partially to this in #7 (comment) I think the public API and the internal API are not important things to define in the first pass of things.
We're on the same page here, and that discussion continues in this PR. The other bulleted points in your comment I'll reply to in context to the code and the previous ongoing discussions. |
| * the current service. | ||
| * @return a new child Span in "started" state. | ||
| */ | ||
| Span startChild(String operationName); |
There was a problem hiding this comment.
From @bensigelman #7 (comment)
- creates a child Span from a parent Span when the Tracer is not accessible (c.f. your removal of
Span.startChild),
I do not understand how it is possible that a Tracer is not accessible, nor the point of creating a Span in any environment when no Tracer exists.
There was a problem hiding this comment.
@bensigelman None of the examples raised by @yurishkuro nor anything else i've seen demonstrates any need for the method span.startChild(..) when processing spans postmortem from a different system.
|
I'm torn between a point-by-point response to all of the various issues here and something higher-level. I'll go for the latter since it'll be less noisy for everyone involved... I am thankful that you sent this out, as this review and some of the gitter traffic demonstrates (to me) that the current set of OpenTracing primitives apparently leads to mass confusion: surely a fault of the API/scheme, not the masses. :) Or maybe it's just missing documentation, but my instinct tells me that the problem is deeper than that. I am going to try to send out a PR that basically gets rid of Sound reasonable? |
|
Thanks, Ben. In turn, I will post a PR with documentation of the Common Use Cases we've discussed when designing the API, with code examples of how each use case is solved by the current API. |
|
Sounds reasonable @bensigelman |
|
@yurishkuro Can you push these as documentation into opentracing.github.io ? This really would be invaluable information for newcomers. |
|
@michaelsembwever already in PR opentracing/opentracing.io#34 |
|
@yurishkuro thanks for the examples. They all match my previous understandings of what we're trying to achieve. So far I haven't been able to find any reasons why the API simplifications offered in the file changes here can't or shouldn't be adopted. Although I'm interested in @bensigelman's proposed work to remove TraceContext/SpanContext altogether, and can rebase these simplifications on top of that if it's accepted. The examples do add context to two separate discussions in this PR: the getter method in SpanContext and the codec class. I'll add my thoughts to these existing discussions. |
|
Summarising the status of the PR so far.
Actions to take.
Other suggestions that have been raised in this PR.
Is this clear? Does everyone agree? |
|
I sent out opentracing/opentracing-go#40 (for better or worse, @michaelsembwever if people like the direction of my RFC above, we can get rid of a lot of stuff here. And also save a lot of time and energy arguing about names that would no longer exist :) PS: @michaelsembwever sorry I didn't respond to all of the various minor points above where you @bensigelman'd me -- I thought it would be wasted effort for both of us since so much of the debated material is presently in jeopardy :) |
|
That's fine @bensigelman , i'll be happy to see the work here come to fruition any way possible. As listed in my previous comment everything that this PR proposes is eventuating one way or another, plus more, and that makes me happy. I'd only hoped that you got many more of these types of reviews opened before you started considering anything vaguely resembling a 1.0 release. |
Remove TraceContextSource, as methods from it now belong in Tracer. Remove Tracer.joinTrace(..) as Span.startChild(..) already does this. Simplify the SpanPropagator. Having to split responsibilities for span and trace identification versus trace attributes is not something the end-user cares about. ref: - opentracing/opentracing.io#42 - #7
|
Closing this PR, the first step in a simpler API it superseded with #8 |
Remove TraceContextSource, as methods from it now belong in Tracer. Remove Tracer.joinTrace(..) as Span.startChild(..) already does this. ref: - opentracing/opentracing.io#42 - #7
- simply initial api (just one way of doing things) - avoid Heisenberg tracing References: - opentracing/opentracing-java#7 (comment) - #47
This PR contains a sequence of commits to simplify the API.
Class renames:
Removing porcelain api:
event(message, payload)).Look in the individual commits for the rationale to each.
It's clearer to see the value of the changes by looking at the HEAD of the branch rather than evaluating the patch.