Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split SpanContext into interface / impl #1935

Merged
merged 5 commits into from
Dec 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.api.trace;

import com.google.auto.value.AutoValue;
import com.google.auto.value.extension.memoized.Memoized;
import javax.annotation.concurrent.Immutable;

@Immutable
@AutoValue
abstract class ImmutableSpanContext implements SpanContext {
Copy link
Member

Choose a reason for hiding this comment

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

Since the interface is also annotated @Immutable and according to the spec any SpanContext must be Immutable, this is maybe not the best name to distinguish it from possible other implementations which would also be Immutable. Maybe DefaultSpanContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I had the same impression - we have Immutable* as a pattern in many AutoValue implementations of interfaces currently. Renaming them all to Default seems fine to me, or even *Impl so the implementation orders next to the interface in the filetree.

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion. I'd keep this name for this PR, and we can always rename later, since this is an internal detail.

bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved

private static final SpanContext INVALID =
create(
TraceId.getInvalid(),
SpanId.getInvalid(),
TraceFlags.getDefault(),
TraceState.getDefault(),
/* remote= */ false);

static SpanContext getInvalid() {
return INVALID;
}

static SpanContext create(
String traceIdHex, String spanIdHex, byte traceFlags, TraceState traceState, boolean remote) {
return new AutoValue_ImmutableSpanContext(
traceIdHex, spanIdHex, traceFlags, traceState, remote);
}

@Override
@Memoized
public byte[] getTraceIdBytes() {
return SpanContext.super.getTraceIdBytes();
}

@Override
@Memoized
public byte[] getSpanIdBytes() {
return SpanContext.super.getSpanIdBytes();
}

@Override
@Memoized
public boolean isValid() {
return SpanContext.super.isValid();
}
}
76 changes: 29 additions & 47 deletions api/src/main/java/io/opentelemetry/api/trace/SpanContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

package io.opentelemetry.api.trace;

import com.google.auto.value.AutoValue;
import com.google.auto.value.extension.memoized.Memoized;
import javax.annotation.concurrent.Immutable;

/**
Expand All @@ -15,25 +13,24 @@
* trace_id} and {@link SpanId span_id}) associated with the {@link Span} and a set of options
* (currently only whether the context is sampled or not), as well as the {@link TraceState
* traceState} and the {@link boolean remote} flag.
*
* <p>Implementations of this interface *must* be immutable and have well-defined value-based
* equals/hashCode implementations. If an implementation does not strictly conform to these
* requirements, behavior of the OpenTelemetry APIs and default SDK cannot be guaranteed. It is
* strongly suggested that you use the implementation that is provided here via {@link
* #create(String, String, byte, TraceState)} or {@link #createFromRemoteParent(String, String,
* byte, TraceState)}.
*/
@Immutable
Copy link
Member

Choose a reason for hiding this comment

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

Does this still make sense on an interface?

Copy link
Member

Choose a reason for hiding this comment

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

I think it does. It places an requirement on all implementers and offers guarantees to users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously, it's a soft requirement, since it can't be enforced, but I still think it's a good thing to document.

@AutoValue
public abstract class SpanContext {

private static final SpanContext INVALID =
create(
TraceId.getInvalid(),
SpanId.getInvalid(),
TraceFlags.getDefault(),
TraceState.getDefault());
public interface SpanContext {

/**
* Returns the invalid {@code SpanContext} that can be used for no-op operations.
*
* @return the invalid {@code SpanContext}.
*/
public static SpanContext getInvalid() {
return INVALID;
static SpanContext getInvalid() {
return ImmutableSpanContext.getInvalid();
}

/**
Expand All @@ -45,15 +42,10 @@ public static SpanContext getInvalid() {
* @param traceState the trace state for the span context.
* @return a new {@code SpanContext} with the given identifiers and options.
*/
public static SpanContext create(
static SpanContext create(
String traceIdHex, String spanIdHex, byte traceFlags, TraceState traceState) {
return create(traceIdHex, spanIdHex, traceFlags, traceState, /* remote=*/ false);
}

private static SpanContext create(
String traceIdHex, String spanIdHex, byte traceFlags, TraceState traceState, boolean remote) {
return new AutoValue_SpanContext(
traceIdHex, spanIdHex, traceFlags, traceState, /* remote$=*/ remote);
return ImmutableSpanContext.create(
traceIdHex, spanIdHex, traceFlags, traceState, /* remote=*/ false);
}

/**
Expand All @@ -66,60 +58,51 @@ private static SpanContext create(
* @param traceState the trace state for the span context.
* @return a new {@code SpanContext} with the given identifiers and options.
*/
public static SpanContext createFromRemoteParent(
static SpanContext createFromRemoteParent(
String traceIdHex, String spanIdHex, byte traceFlags, TraceState traceState) {
return create(traceIdHex, spanIdHex, traceFlags, traceState, /* remote=*/ true);
return ImmutableSpanContext.create(
traceIdHex, spanIdHex, traceFlags, traceState, /* remote=*/ true);
}

abstract String getTraceIdHex();

abstract String getSpanIdHex();

/**
* Returns the trace identifier associated with this {@code SpanContext}.
*
* @return the trace identifier associated with this {@code SpanContext}.
*/
public String getTraceIdAsHexString() {
return getTraceIdHex();
}
String getTraceIdAsHexString();

/**
* Returns the byte[] representation of the trace identifier associated with this {@link
* SpanContext}.
*/
@Memoized
public byte[] getTraceIdBytes() {
return TraceId.bytesFromHex(getTraceIdHex(), 0);
default byte[] getTraceIdBytes() {
return TraceId.bytesFromHex(getTraceIdAsHexString(), 0);
}

/**
* Returns the span identifier associated with this {@code SpanContext}.
*
* @return the span identifier associated with this {@code SpanContext}.
*/
public String getSpanIdAsHexString() {
return getSpanIdHex();
}
String getSpanIdAsHexString();

/**
* Returns the byte[] representation of the span identifier associated with this {@link
* SpanContext}.
*/
@Memoized
public byte[] getSpanIdBytes() {
return SpanId.bytesFromHex(getSpanIdHex(), 0);
default byte[] getSpanIdBytes() {
return SpanId.bytesFromHex(getSpanIdAsHexString(), 0);
}
Comment on lines +93 to 95
Copy link
Member

Choose a reason for hiding this comment

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

Why removing the ability to memoized this call? It may get expensive. I would suggest to not default this and any method that was memoized before.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still memoized by the autovalue implementation. See ImmutableSpanContext above.


/** Whether the span in this context is sampled. */
public boolean isSampled() {
default boolean isSampled() {
return (getTraceFlags() & 1) == 1;
}

/** The byte-representation of {@link TraceFlags}. */
public abstract byte getTraceFlags();
byte getTraceFlags();

public void copyTraceFlagsHexTo(char[] dest, int destOffset) {
default void copyTraceFlagsHexTo(char[] dest, int destOffset) {
BigendianEncoding.byteToBase16String(getTraceFlags(), dest, destOffset);
}

Expand All @@ -128,22 +111,21 @@ public void copyTraceFlagsHexTo(char[] dest, int destOffset) {
*
* @return the {@code TraceState} associated with this {@code SpanContext}.
*/
public abstract TraceState getTraceState();
TraceState getTraceState();

/**
* Returns {@code true} if this {@code SpanContext} is valid.
*
* @return {@code true} if this {@code SpanContext} is valid.
*/
@Memoized
public boolean isValid() {
return TraceId.isValid(getTraceIdHex()) && SpanId.isValid(getSpanIdHex());
default boolean isValid() {
return TraceId.isValid(getTraceIdAsHexString()) && SpanId.isValid(getSpanIdAsHexString());
}
Comment on lines +121 to 123
Copy link
Member

Choose a reason for hiding this comment

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

this will become more expensive after this change, we should calculate this only once since it is used couple of times per instance created.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, see above...this is still memoized by the implementation.


/**
* Returns {@code true} if the {@code SpanContext} was propagated from a remote parent.
*
* @return {@code true} if the {@code SpanContext} was propagated from a remote parent.
*/
public abstract boolean isRemote();
boolean isRemote();
}