-
Notifications
You must be signed in to change notification settings - Fork 343
Initial opentracing-java API #13
Changes from 6 commits
53fb140
60d9019
f76c758
ad4252e
87a0c6d
fd5564d
7bc6ae1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
/** | ||
* Copyright 2016 The OpenTracing Authors | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except | ||
* in compliance with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License | ||
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express | ||
* or implied. See the License for the specific language governing permissions and limitations under | ||
* the License. | ||
*/ | ||
package opentracing; | ||
|
||
/** | ||
* Span represents an active, un-finished span in the opentracing system. | ||
* | ||
* <p>Spans are created by the {@link Tracer} interface. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: more specific? ex There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
*/ | ||
public interface Span { | ||
|
||
/** | ||
* Sets the end timestamp and records the span. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The problem is that timestamps in java are measured in millis accuracy. To Anyway it is a different topic, in OT status quo, endTs sounds like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, I think it may have been you that mentioned that the risk of
time moving backwards is an issue when doing timestamp based intervals
vs absolute + duration offset (back in the day)
I don't want this conversation to hold up mick's work as it is a
different topic which spun out of me enumerating how this is done
today in Brave. If you feel this is important enough to continue,
probably a top-level issue is best.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm leaving the addition of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough
|
||
* | ||
* <p>This should be the last call made to any span instance, and to do otherwise leads to | ||
* undefined behavior. | ||
*/ | ||
void finish(); | ||
|
||
/** | ||
* Set a key:value tag on the Span. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the behavior of multiple calls with the same key is implementation specific There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. leaving it specified. it can be relaxed if implementations of opentracing-java can't/won't adhere to the current statement. |
||
* | ||
* <p>Tag values can be of arbitrary types, however the treatment of complex types is dependent on | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like complex types aren't possible.. do we need this doc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
* the underlying tracing system implementation. It is expected that most tracing systems will | ||
* handle primitive types like strings and numbers. If a tracing system cannot understand how to | ||
* handle a particular value type, it may ignore the tag, but shall not panic. | ||
* | ||
* <p>If there is a pre-existing tag set for {@code key}, it is overwritten. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor point: this behavior (same key, new value) is formally undefined in some other impls. No strong feelings but wanted to point out the discrepancy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed. |
||
*/ | ||
// overloaded 3x to support the BasicType concern | ||
Span setTag(String key, String value); | ||
|
||
/** Same as {@link #setTag(String, String)}, but for boolean values. */ | ||
Span setTag(String key, boolean value); | ||
|
||
/** Same as {@link #setTag(String, String)}, but for numeric values. */ | ||
Span setTag(String key, Number value); | ||
|
||
/** | ||
* Set a Baggage item, represented as a simple string:string pair. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see you have left out the unpalatable HTTP-header-inspired restrictions on the |
||
* | ||
* Note that newly-set Baggage items are only guaranteed to propagate to future children of the given Span. | ||
*/ | ||
Span setBaggageItem(String key, String value); | ||
|
||
/** Get a Baggage item by key. */ | ||
String getBaggageItem(String key); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. returns null if baggage isn't supported or an item with this key isn't present? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
|
||
/** * | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's some things like this, which should be easy enough to scrub. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
* Add a new log event to the Span, accepting an event name string and an optional structured payload argument. | ||
* If specified, the payload argument may be of any type and arbitrary size, | ||
* though implementations are not required to retain all payload arguments | ||
* (or even all parts of all payload arguments). | ||
* | ||
* The timestamp of this log event is the current time. | ||
**/ | ||
Span log(String eventName, /* @Nullable */ Object payload); | ||
|
||
/** | ||
* Add a new log event to the Span, accepting an event name string and an optional structured payload argument. | ||
* If specified, the payload argument may be of any type and arbitrary size, | ||
* though implementations are not required to retain all payload arguments | ||
* (or even all parts of all payload arguments). | ||
* | ||
* The timestamp is specified manually here to represent a past log event. | ||
* The timestamp in microseconds in UTC time. | ||
**/ | ||
Span log(long instantMicroseconds, String eventName, /* @Nullable */ Object payload); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer timestampMicroseconds to instantMicroseconds as the former word is used in the javadoc There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed (but note it changes in #14) |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
/** | ||
* Copyright 2016 The OpenTracing Authors | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except | ||
* in compliance with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License | ||
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express | ||
* or implied. See the License for the specific language governing permissions and limitations under | ||
* the License. | ||
*/ | ||
package opentracing; | ||
|
||
|
||
/** | ||
* Tracer is a simple, thin interface for Span creation, and Span propagation into different transport formats. | ||
*/ | ||
public interface Tracer { | ||
|
||
/** | ||
* Create, start, and return a new Span with the given `operationName`. | ||
* An optional parent Span can be specified used to incorporate the newly-returned Span into an existing trace. | ||
* | ||
* <p>Example: | ||
* <pre>{@code | ||
Tracer tracer = ... | ||
|
||
Span feed = tracer.buildSpan("GetFeed") | ||
.start(); | ||
|
||
Span http = tracer.buildSpan("HandleHTTPRequest") | ||
.withParent(feed) | ||
.withTag("user_agent", req.UserAgent) | ||
.withTag("lucky_number", 42) | ||
.start(); | ||
}</pre> | ||
*/ | ||
SpanBuilder buildSpan(String operationName); | ||
|
||
/** 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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);" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
* | ||
*/ | ||
<T> void inject(Span span, T carrier); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does an impl signal that a carrier is unsupported? (I.e., should we define some standard exception types?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IllegalStateException. Since this is a programmatic error. Will add to apidoc. |
||
|
||
/** Returns a SpanBuilder provided | ||
* a “carrier” object from which to extract identifying information needed by the new Span instance. | ||
* | ||
* If the carrier object has no such span stored within it, a new Span is created. | ||
* | ||
* Unless there’s an error, it returns a SpanBuilder. | ||
* The Span generated from the builder can be used in the host process like any other. | ||
* | ||
* (Note that some OpenTracing implementations consider the Spans on either side of an RPC to have the same identity, | ||
* and others consider the caller to be the parent and the receiver to be the child) | ||
*/ | ||
<T> SpanBuilder join(T carrier); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the exception front, there are a few possibilities for join()... here's what happened in Python: https://github.com/opentracing/opentracing-python/blob/master/opentracing/propagation.py#L24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (InvalidCarrierException is N/A here given the lack of a format argument) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. InvalidCarrierException --> IllegalStateexception The latter can be argued that it should be a checked exception. will add to apidoc. |
||
|
||
interface SpanBuilder { | ||
|
||
/** Specify the operationName. | ||
* | ||
* If the operationName has already been set (implicitly or explicitly) an IllegalStateException will be thrown. | ||
*/ | ||
SpanBuilder withOperationName(String operationName); | ||
|
||
/** Specify the parent span | ||
* | ||
* If the parent has already been set an IllegalStateException will be thrown. | ||
*/ | ||
SpanBuilder withParent(Span parent); | ||
|
||
/** Specify a timestamp the Span actually started from. | ||
* | ||
* If the timestamp has already been set an IllegalStateException will be thrown. | ||
*/ | ||
SpanBuilder withTimestamp(long microseconds); | ||
|
||
/** Same as {@link Span#setTag(String, String)}, but for the span being built. */ | ||
SpanBuilder withTag(String key, String value); | ||
|
||
/** Same as {@link Span#setTag(String, String)}, but for the span being built. */ | ||
SpanBuilder withTag(String key, boolean value); | ||
|
||
/** Same as {@link Span#setTag(String, String)}, but for the span being built. */ | ||
SpanBuilder withTag(String key, Number value); | ||
|
||
/** Returns the started Span. */ | ||
Span start(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer overloading start(long microseconds) vs withTimestamp().start() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that seems to defeat the purpose of the Builder pattern. Starting a child span is by far the most frequent usage, so would we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No. I believe @adriancole suggested this, and why it made sense to me, is because start and timestamp are close concepts. having a choice between calling
It's not what's frequent, but what intuitively makes sense or reads well, ie fluent api. That said, it could be argued that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, overriding the timestamp is a corner case, and thus we should choose the consistent API (i.e., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, have changed code to |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore if unhelpful.. I prefer "in-flight" to "active, un-finished"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed