Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Initial opentracing-java API #13

Merged
merged 7 commits into from
Mar 9, 2016
80 changes: 80 additions & 0 deletions opentracing/src/main/java/opentracing/Span.java
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.
Copy link
Contributor

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

*
* <p>Spans are created by the {@link Tracer} interface.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: more specific? ex {@link Tracer#buildSpan}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

*/
public interface Span {

/**
* Sets the end timestamp and records the span.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm leaving the addition of finish(timestamp) as outside the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*
* <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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the behavior of multiple calls with the same key is implementation specific

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like complex types aren't possible.. do we need this doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 key here... intentional? It is a deviation from the spec in any case.

*
* 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


/** *
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed (but note it changes in #14)

}
94 changes: 94 additions & 0 deletions opentracing/src/main/java/opentracing/Tracer.java
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

*
*/
<T> void inject(Span span, T carrier);
Copy link
Contributor

Choose a reason for hiding this comment

The 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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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

class UnsupportedFormatException(Exception):
    """UnsupportedFormatException should be used when the provided format
    value is unknown or disallowed by the Tracer.
    See Tracer.inject() and Tracer.join().
    """
    pass


class InvalidCarrierException(Exception):
    """InvalidCarrierException should be used when the provided carrier
    instance does not match what the `format` argument requires.
    See Tracer.inject() and Tracer.join().
    """
    pass


class TraceCorruptedException(Exception):
    """TraceCorruptedException should be used when the underlynig trace state
    is seemingly present but not well-formed.
    See Tracer.inject() and Tracer.join().
    """
    pass

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(InvalidCarrierException is N/A here given the lack of a format argument)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InvalidCarrierException --> IllegalStateexception
TraceCorruptedException --> IllegalArgumentException

The latter can be argued that it should be a checked exception.
I'd like to wait and see what experience shows first.

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer overloading start(long microseconds) vs withTimestamp().start()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

Choose a reason for hiding this comment

The 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 start(parent) too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so would we add start(parent) too?

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 start() or start(timestamp) intuitively makes sense. ie start now or start at this timestamp. (rationale: timestamp is associated to the start, while parent belongs to the span.) that's part of designing a fluent api.

Starting a child span is by far the most frequent usage …

It's not what's frequent, but what intuitively makes sense or reads well, ie fluent api.

That said, it could be argued that start(timestamp) isn't intuitive enough, and i've no strong opinion to that. withStartTimestamp(..) is slightly more intuitive if you ask me. so i'd be happy to return the code if this was the argument raised.

Copy link
Contributor

Choose a reason for hiding this comment

The 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., withTimestamp(...)) rather than the shorthand API (i.e., overloading start(timestamp)). No strong feelings, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, have changed code to withStartTimestamp(..)

}
}