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
76 changes: 76 additions & 0 deletions opentracing/src/main/java/opentracing/Span.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* 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;

/**
* Represents an in-flight span in the opentracing system.
*
* <p>Spans are created by the {@link Tracer#buildSpan} interface.
*/
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.

*/
// 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.
*
* Returns null if no entry found, or baggage is not supported in the current implementation.
*/
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


/**
* 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 timestampMicroseconds, String eventName, /* @Nullable */ Object payload);
}
105 changes: 105 additions & 0 deletions opentracing/src/main/java/opentracing/Tracer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/**
* 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

*
* A “carrier” object is some sort of http or rpc envelope, for example HeaderGroup (from Apache HttpComponents).
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add the OT-required carriers to Java at some point, though FWIW I would like to make those required carriers clearer. opentracing/opentracing.io#73

If the importance of "required carriers" is not obvious, I can explain...

Copy link
Contributor

Choose a reason for hiding this comment

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

And to be clear: Map<String, String> and ByteBuffer are not interoperable with the required carriers in the other platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These required carriers are in #11

The non-interoperable i presume you mean is at the "format" level.
I don't see this is a problem as long as it's clearly documented with java that these are the required carriers and are otherwise interoperable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest leaving out the comment about required carriers fro now... if the required carriers in Java don't have the same structural shape as the required carriers in other languages, I don't see how an OT implementation that is intended to work for both Java and other OT platforms can inject() in Java using Map<String, String> and join() in some other platform that uses SplitTextCarrier (which has not one but two maps within).

I'd rather not try to spend our time debating this over github, though, as opentracing/opentracing.io#73 might make one or both of our respective points of view moot (probably mine, actually, as I'm basically arguing for the removal of the "Split" part of "Split*Carrier").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Both Text and Binary Carrier need a defined serialisation specification.
I've raised this before in the need for an Internal Specification, that implementors need to adhere to.

I'm not in favour of the two maps approach, it just seems clumsy and presumptuous that the underlying rpc/http framework can put maps inside of maps.

The solution of state and baggage prefixes used on keys all within the one map makes more sense to me. It seems to be a solution that will work across languages and across rpc/http frameworks best.

I've removed the comment on required carriers.

*
* Attempting to inject to a carrier that has been registered/configured to this Tracer will result in a
* IllegalStateException.
*/
<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)
*
* Attempting to join from a carrier that has been registered/configured to this Tracer will result in a
* IllegalStateException.
*
* If the span serialized state is invalid (corrupt, wrong version, etc) inside the carrier this will result in a
* IllegalArgumentException.
*/
<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);

/** 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);

/** Specify a timestamp of when the Span was started, represented in microseconds since epoch. */
SpanBuilder withStartTimestamp(long microseconds);

/** 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(..)


/** Returns the Span, with a started timestamp (represented in microseconds) as specified. */
Span start(long microseconds);
}
}