Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@dmarkwat
dmarkwat committed Mar 8, 2023
1 parent c5218cc commit 326f9d2
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 82 deletions.
112 changes: 53 additions & 59 deletions ...elemetry/opencensusshim/ProxyingSpan.java → ...emetry/opencensusshim/DelegatingSpan.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

/**
* Delegates <i>all</i> {@link Span} methods to some underlying Span via {@link
* ProxyingSpan#getProxied()}.
* DelegatingSpan#getDelegate()}.
*
* <p>If not all calls are proxied, some, such as and in particular {@link
* Span#storeInContext(Context)} and {@link Span#makeCurrent()}, will use {@code this} instead of
Expand All @@ -30,141 +30,135 @@
* from the shim.
*
* <p>This addresses the inconsistency where not all methods are appropriately delegated by exposing
* a single method, {@link ProxyingSpan#getProxied()}, to simplify and better ensure delegation and
* meeting expectations.
* a single method, {@link DelegatingSpan#getDelegate()}, to simplify and better ensure delegation
* and meeting expectations.
*/
// todo make this unnecessary
@SuppressWarnings("UngroupedOverloads")
interface ProxyingSpan extends Span {
Span getProxied();

// implementations
interface DelegatingSpan extends Span {
Span getDelegate();

@Override
default <T> Span setAttribute(AttributeKey<T> key, T value) {
return getProxied().setAttribute(key, value);
default Span updateName(String name) {
return getDelegate().updateName(name);
}

@Override
default Span addEvent(String name, Attributes attributes) {
return getProxied().addEvent(name, attributes);
default SpanContext getSpanContext() {
return getDelegate().getSpanContext();
}

@Override
default Span addEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) {
return getProxied().addEvent(name, attributes, timestamp, unit);
default boolean isRecording() {
return getDelegate().isRecording();
}

@Override
default Span setStatus(StatusCode statusCode, String description) {
return getProxied().setStatus(statusCode, description);
default <T> Span setAttribute(AttributeKey<T> key, T value) {
return getDelegate().setAttribute(key, value);
}

@Override
default Span recordException(Throwable exception, Attributes additionalAttributes) {
return getProxied().recordException(exception, additionalAttributes);
default Span setAttribute(String key, String value) {
return getDelegate().setAttribute(key, value);
}

@Override
default Span updateName(String name) {
return getProxied().updateName(name);
default Span setAttribute(String key, long value) {
return getDelegate().setAttribute(key, value);
}

@Override
default void end() {
getProxied().end();
default Span setAttribute(String key, double value) {
return getDelegate().setAttribute(key, value);
}

@Override
default void end(long timestamp, TimeUnit unit) {
getProxied().end(timestamp, unit);
default Span setAttribute(String key, boolean value) {
return getDelegate().setAttribute(key, value);
}

@Override
default SpanContext getSpanContext() {
return getProxied().getSpanContext();
default Span setAttribute(AttributeKey<Long> key, int value) {
return getDelegate().setAttribute(key, value);
}

@Override
default boolean isRecording() {
return getProxied().isRecording();
default Span setAllAttributes(Attributes attributes) {
return getDelegate().setAllAttributes(attributes);
}

// default overrides

@Override
default Span setAttribute(String key, String value) {
return getProxied().setAttribute(key, value);
default Span addEvent(String name, Attributes attributes) {
return getDelegate().addEvent(name, attributes);
}

@Override
default Span setAttribute(String key, long value) {
return getProxied().setAttribute(key, value);
default Span addEvent(String name, Attributes attributes, long timestamp, TimeUnit unit) {
return getDelegate().addEvent(name, attributes, timestamp, unit);
}

@Override
default Span setAttribute(String key, double value) {
return getProxied().setAttribute(key, value);
default Span addEvent(String name) {
return getDelegate().addEvent(name);
}

@Override
default Span setAttribute(String key, boolean value) {
return getProxied().setAttribute(key, value);
default Span addEvent(String name, long timestamp, TimeUnit unit) {
return getDelegate().addEvent(name, timestamp, unit);
}

@Override
default Span setAttribute(AttributeKey<Long> key, int value) {
return getProxied().setAttribute(key, value);
default Span addEvent(String name, Instant timestamp) {
return getDelegate().addEvent(name, timestamp);
}

@Override
default Span setAllAttributes(Attributes attributes) {
return getProxied().setAllAttributes(attributes);
default Span addEvent(String name, Attributes attributes, Instant timestamp) {
return getDelegate().addEvent(name, attributes, timestamp);
}

@Override
default Span addEvent(String name) {
return getProxied().addEvent(name);
default Span setStatus(StatusCode statusCode, String description) {
return getDelegate().setStatus(statusCode, description);
}

@Override
default Span addEvent(String name, long timestamp, TimeUnit unit) {
return getProxied().addEvent(name, timestamp, unit);
default Span setStatus(StatusCode statusCode) {
return getDelegate().setStatus(statusCode);
}

@Override
default Span addEvent(String name, Instant timestamp) {
return getProxied().addEvent(name, timestamp);
default Span recordException(Throwable exception, Attributes additionalAttributes) {
return getDelegate().recordException(exception, additionalAttributes);
}

@Override
default Span addEvent(String name, Attributes attributes, Instant timestamp) {
return getProxied().addEvent(name, attributes, timestamp);
default Span recordException(Throwable exception) {
return getDelegate().recordException(exception);
}

@Override
default Span setStatus(StatusCode statusCode) {
return getProxied().setStatus(statusCode);
default void end(Instant timestamp) {
getDelegate().end(timestamp);
}

@Override
default Span recordException(Throwable exception) {
return getProxied().recordException(exception);
default void end() {
getDelegate().end();
}

@Override
default void end(Instant timestamp) {
getProxied().end(timestamp);
default void end(long timestamp, TimeUnit unit) {
getDelegate().end(timestamp, unit);
}

@Override
default Context storeInContext(Context context) {
return getProxied().storeInContext(context);
return getDelegate().storeInContext(context);
}

@MustBeClosed
@Override
default Scope makeCurrent() {
return getProxied().makeCurrent();
return getDelegate().makeCurrent();
}
}
27 changes: 14 additions & 13 deletions opencensus-shim/src/main/java/io/opentelemetry/opencensusshim/OpenTelemetrySpanImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@
import java.util.Map;
import java.util.logging.Logger;

class OpenTelemetrySpanImpl extends Span implements io.opentelemetry.api.trace.Span, ProxyingSpan {
class OpenTelemetrySpanImpl extends Span
implements io.opentelemetry.api.trace.Span, DelegatingSpan {
private static final Logger LOGGER = Logger.getLogger(OpenTelemetrySpanImpl.class.getName());
private static final EnumSet<Span.Options> RECORD_EVENTS_SPAN_OPTIONS =
EnumSet.of(Span.Options.RECORD_EVENTS);
Expand All @@ -63,7 +64,7 @@ class OpenTelemetrySpanImpl extends Span implements io.opentelemetry.api.trace.S
// otel

@Override
public io.opentelemetry.api.trace.Span getProxied() {
public io.opentelemetry.api.trace.Span getDelegate() {
return otelSpan;
}

Expand All @@ -74,10 +75,10 @@ public void putAttribute(String key, AttributeValue value) {
Preconditions.checkNotNull(key, "key");
Preconditions.checkNotNull(value, "value");
value.match(
arg -> ProxyingSpan.super.setAttribute(key, arg),
arg -> ProxyingSpan.super.setAttribute(key, arg),
arg -> ProxyingSpan.super.setAttribute(key, arg),
arg -> ProxyingSpan.super.setAttribute(key, arg),
arg -> DelegatingSpan.super.setAttribute(key, arg),
arg -> DelegatingSpan.super.setAttribute(key, arg),
arg -> DelegatingSpan.super.setAttribute(key, arg),
arg -> DelegatingSpan.super.setAttribute(key, arg),
arg -> null);
}

Expand All @@ -91,14 +92,14 @@ public void putAttributes(Map<String, AttributeValue> attributes) {
public void addAnnotation(String description, Map<String, AttributeValue> attributes) {
AttributesBuilder attributesBuilder = Attributes.builder();
mapAttributes(attributes, attributesBuilder);
ProxyingSpan.super.addEvent(description, attributesBuilder.build());
DelegatingSpan.super.addEvent(description, attributesBuilder.build());
}

@Override
public void addAnnotation(Annotation annotation) {
AttributesBuilder attributesBuilder = Attributes.builder();
mapAttributes(annotation.getAttributes(), attributesBuilder);
ProxyingSpan.super.addEvent(annotation.getDescription(), attributesBuilder.build());
DelegatingSpan.super.addEvent(annotation.getDescription(), attributesBuilder.build());
}

@Override
Expand All @@ -108,7 +109,7 @@ public void addLink(Link link) {

@Override
public void addMessageEvent(MessageEvent messageEvent) {
ProxyingSpan.super.addEvent(
DelegatingSpan.super.addEvent(
String.valueOf(messageEvent.getMessageId()),
Attributes.of(
AttributeKey.stringKey(MESSAGE_EVENT_ATTRIBUTE_KEY_TYPE),
Expand All @@ -122,22 +123,22 @@ public void addMessageEvent(MessageEvent messageEvent) {
@Override
public void setStatus(Status status) {
Preconditions.checkNotNull(status, "status");
ProxyingSpan.super.setStatus(status.isOk() ? StatusCode.OK : StatusCode.ERROR);
DelegatingSpan.super.setStatus(status.isOk() ? StatusCode.OK : StatusCode.ERROR);
}

@Override
public io.opentelemetry.api.trace.Span setStatus(StatusCode canonicalCode, String description) {
return ProxyingSpan.super.setStatus(canonicalCode, description);
return DelegatingSpan.super.setStatus(canonicalCode, description);
}

@Override
public void end(EndSpanOptions options) {
ProxyingSpan.super.end();
DelegatingSpan.super.end();
}

@Override
public SpanContext getSpanContext() {
return ProxyingSpan.super.getSpanContext();
return DelegatingSpan.super.getSpanContext();
}

@Override
Expand Down
19 changes: 9 additions & 10 deletions ...etry/opencensusshim/ProxyingSpanTest.java → ...ry/opencensusshim/DelegatingSpanTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
* and as instrumented via the javaagent. Details <a
* href="https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/6876">here</a>.
*/
class ProxyingSpanTest {
class DelegatingSpanTest {

/*
Verifies all methods on the otel Span interface are under test.
Expand All @@ -41,7 +41,7 @@ flexibility for special cases (e.g. getSpanContext() and isRecording())
@Test
void verifyAllMethodsAreUnderTest() {
List<Method> methods =
proxyMethodsProvider()
delegateMethodsProvider()
.map(
pm -> {
try {
Expand All @@ -62,13 +62,13 @@ void verifyAllMethodsAreUnderTest() {
}

@ParameterizedTest
@MethodSource("proxyMethodsProvider")
@MethodSource("delegateMethodsProvider")
void testit(String name, Class<?>[] params, VerificationMode timesCalled)
throws NoSuchMethodException, InvocationTargetException, IllegalAccessException {
Method method = getInterfaceMethod(Span.class, name, params);
Span mockedSpan = Mockito.spy(Span.current());
OpenTelemetrySpanImpl span = new OpenTelemetrySpanImpl(mockedSpan);
assertProxied(span, mockedSpan, method, timesCalled);
assertDelegated(span, mockedSpan, method, timesCalled);
}

static List<Method> allInterfaceMethods(Class<?> clazz) {
Expand All @@ -77,8 +77,7 @@ static List<Method> allInterfaceMethods(Class<?> clazz) {
.collect(Collectors.toList());
}

static Stream<Arguments> proxyMethodsProvider() {
// todo fill in remaining methods
static Stream<Arguments> delegateMethodsProvider() {
return Stream.of(
Arguments.of("end", new Class<?>[] {}, times(1)),
Arguments.of(
Expand Down Expand Up @@ -145,7 +144,7 @@ static Stream<Arguments> proxyMethodsProvider() {
// called never -- it's shared between OC and Otel Span types and is always true, so returns
// `true`
Arguments.of("isRecording", new Class<?>[] {}, times(0)),
// called twice: once in constructor, then once during proxy
// called twice: once in constructor, then once during delegation
Arguments.of("getSpanContext", new Class<?>[] {}, times(2)));
}

Expand Down Expand Up @@ -175,14 +174,14 @@ static Object valueLookup(Class<?> clazz) {
}
}

static <T> Method getInterfaceMethod(Class<T> proxy, String name, Class<?>[] params)
static <T> Method getInterfaceMethod(Class<T> clazz, String name, Class<?>[] params)
throws NoSuchMethodException {
Method method = proxy.getMethod(name, params);
Method method = clazz.getMethod(name, params);
assertThat(method).isNotNull();
return method;
}

static <T> void assertProxied(T proxy, T delegate, Method method, VerificationMode mode)
static <T> void assertDelegated(T proxy, T delegate, Method method, VerificationMode mode)
throws InvocationTargetException, IllegalAccessException {
// Get parameters for method
Class<?>[] parameterTypes = method.getParameterTypes();
Expand Down

0 comments on commit 326f9d2

Please sign in to comment.