Initial opentracing-java API #13
Conversation
This is mostly a port from python, except for a couple notes. * Adds an IllegalArgumentException to `fromXXX`, if the input is malformed. * Adds `EncodedTraceContext` type as Java cannot return multiple results. Any other changes are unintentional. See https://github.com/opentracing/api-go/tree/master/opentracing
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
Simplify the API: removing TraceContext and TraceContextSource
…ge, 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. reference: #9
- Only span creation method is in Tracer.startSpan(..) - Add Tracer's inject(..) and join(..) methods reference: #10
|
|
||
| /** Takes two arguments: | ||
| * a Span instance, and | ||
| * a “carrier” object in which to inject that Span for cross-process propagation. |
There was a problem hiding this comment.
the word "carrier" is very vague when in practical terms it is very often http headers or some other side-channel (like RPC envelope). might be best to make an example of a carrier.
"For example. tracer.inject(span, httpRequestHeaders);"
|
Looks like an accurate, minimal specification of OT as defined. |
when ready
michaelsembwever
force-pushed
the
interfaces
branch
from
5ede6ec to
3056c7b
Compare
| public interface Span { | ||
|
|
||
| /** | ||
| * Sets the end timestamp and records the span. |
There was a problem hiding this comment.
Per the start() debate, we do need some way to override the finish timestamp in order to do after-the-fact translation of span-like logs, etc. So, void finish(long timestamp)?
There was a problem hiding this comment.
Per the start() debate, we do need some way to override the finish
timestamp in order to do after-the-fact translation of span-like logs, etc.
So, void finish(long timestamp)?Brave, which is based on duration not endTs, as this in the span-scoped
tracer:
/**
- Completes the span, which took {@code duration} microseconds.
*/
public void finishSpan(long duration)
There was a problem hiding this comment.
I am imagining – perhaps incorrectly – that the caller of the explicit-timestamp version has a raw timestamp rather than a duration on hand (i.e., that they are basically doing log processing / translation). FWIW, I think that tracing impls are better off tracking duration since it's not subject to clock-skew cruelty.
There was a problem hiding this comment.
I am imagining – perhaps incorrectly – that the caller of the explicit-timestamp version has a raw timestamp rather than a duration on hand (i.e., that they are basically doing log processing / translation). FWIW, I think that tracing impls are better off tracking duration since it's not subject to clock-skew cruelty.
This isn't just for log scraping. this is for any case when OpenTracing
isn't used to create events. Many events are structured and have both start
and endTs, but duration is often recorded separately for more precision.
The problem is that timestamps in java are measured in millis accuracy. To
accurately align nanos ticks with current time is non-trivial and rarely
implemented. duration practice is to measure with nanos ticks. That's the
rationale anyway, and why zipkin/brave has duration.
Anyway it is a different topic, in OT status quo, endTs sounds like the
right choice?
There was a problem hiding this comment.
There was a problem hiding this comment.
I definitely don't feel strongly about finish-time vs duration... I'm making an educated guess about what's most convenient most of the time, but if others feel strongly, I do not :)
There was a problem hiding this comment.
i'm leaving the addition of finish(timestamp) as outside the scope of this PR.
There was a problem hiding this comment.
|
I made a bunch of comments... anything that requires new code (as opposed to changes) I am fine doing in a follow-on; it would be helpful to have something in |
michaelsembwever
force-pushed
the
interfaces
branch
from
3056c7b to
cae53ac
Compare
|
All points addressed. |
|
@michaelsembwever I would prefer that we drop the comments about the required carriers before merging into |
michaelsembwever
force-pushed
the
interfaces
branch
from
cae53ac to
7bc6ae1
Compare
|
(LGTM!) |
Initial opentracing-java API
Last chance to review before all this work hits master.
This is a combination of #4 , #10 , #12 , #8 , and #9 coming into master.
The initial implementation behind this sits in #11
ping @adriancole @bensigelman @yurishkuro