Make twilio instrumentation consistent with others (#1835)

This commit is contained in:
Trask Stalnaker 2020-12-06 22:42:19 -08:00 committed by GitHub
parent b2f2c96904
commit 1ca562ca9c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 57 additions and 91 deletions

View File

@ -5,6 +5,7 @@
package io.opentelemetry.javaagent.instrumentation.twilio;
import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext;
import static io.opentelemetry.javaagent.instrumentation.twilio.TwilioTracer.tracer;
import static io.opentelemetry.javaagent.tooling.ClassLoaderMatcher.hasClassesNamed;
import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.AgentElementMatchers.extendsClass;
@ -21,9 +22,8 @@ import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.twilio.Twilio;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap;
import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
import java.util.Map;
import net.bytebuddy.asm.Advice;
@ -78,20 +78,15 @@ public class TwilioAsyncInstrumentation implements TypeInstrumentation {
public static void methodEnter(
@Advice.This Object that,
@Advice.Origin("#m") String methodName,
@Advice.Local("otelSpan") Span span,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
// Ensure that we only create a span for the top-level Twilio client method; except in the
// case of async operations where we want visibility into how long the task was delayed from
// starting. Our call depth checker does not span threads, so the async case is handled
// automatically for us.
if (CallDepthThreadLocalMap.incrementCallDepth(Twilio.class) > 0) {
Context parentContext = currentContext();
if (!tracer().shouldStartSpan(parentContext)) {
return;
}
// Don't automatically close the span with the scope if we're executing an async method
span = tracer().startSpan(that, methodName);
scope = tracer().startScope(span);
context = tracer().startSpan(parentContext, that, methodName);
scope = context.makeCurrent();
}
/** Method exit instrumentation. */
@ -99,24 +94,22 @@ public class TwilioAsyncInstrumentation implements TypeInstrumentation {
public static void methodExit(
@Advice.Thrown Throwable throwable,
@Advice.Return ListenableFuture<?> response,
@Advice.Local("otelSpan") Span span,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
if (scope == null) {
return;
}
CallDepthThreadLocalMap.reset(Twilio.class);
// span finished in SpanFinishingCallback
scope.close();
if (throwable != null) {
// There was an synchronous error,
// which means we shouldn't wait for a callback to close the span.
tracer().endExceptionally(span, throwable);
tracer().endExceptionally(context, throwable);
} else {
// We're calling an async operation, we still need to finish the span when it's
// complete and report the results; set an appropriate callback
Futures.addCallback(
response, new SpanFinishingCallback<>(span), Twilio.getExecutorService());
response, new SpanFinishingCallback<>(context), Twilio.getExecutorService());
}
}
}
@ -128,22 +121,20 @@ public class TwilioAsyncInstrumentation implements TypeInstrumentation {
public static class SpanFinishingCallback<T> implements FutureCallback<T> {
/** Span that we should finish and annotate when the future is complete. */
private final Span span;
private final Context context;
public SpanFinishingCallback(Span span) {
this.span = span;
public SpanFinishingCallback(Context context) {
this.context = context;
}
@Override
public void onSuccess(Object result) {
tracer().end(span, result);
span.end();
tracer().end(context, result);
}
@Override
public void onFailure(Throwable t) {
tracer().endExceptionally(span, t);
span.end();
tracer().endExceptionally(context, t);
}
}
}

View File

@ -5,6 +5,7 @@
package io.opentelemetry.javaagent.instrumentation.twilio;
import static io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge.currentContext;
import static io.opentelemetry.javaagent.instrumentation.twilio.TwilioTracer.tracer;
import static io.opentelemetry.javaagent.tooling.ClassLoaderMatcher.hasClassesNamed;
import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.AgentElementMatchers.extendsClass;
@ -15,10 +16,8 @@ import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.not;
import com.twilio.Twilio;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap;
import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
import java.util.Map;
import net.bytebuddy.asm.Advice;
@ -74,19 +73,15 @@ public class TwilioSyncInstrumentation implements TypeInstrumentation {
public static void methodEnter(
@Advice.This Object that,
@Advice.Origin("#m") String methodName,
@Advice.Local("otelSpan") Span span,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
// Ensure that we only create a span for the top-level Twilio client method; except in the
// case of async operations where we want visibility into how long the task was delayed from
// starting. Our call depth checker does not span threads, so the async case is handled
// automatically for us.
if (CallDepthThreadLocalMap.incrementCallDepth(Twilio.class) > 0) {
Context parentContext = currentContext();
if (!tracer().shouldStartSpan(parentContext)) {
return;
}
span = tracer().startSpan(that, methodName);
scope = tracer().startScope(span);
context = tracer().startSpan(parentContext, that, methodName);
scope = context.makeCurrent();
}
/** Method exit instrumentation. */
@ -94,18 +89,17 @@ public class TwilioSyncInstrumentation implements TypeInstrumentation {
public static void methodExit(
@Advice.Thrown Throwable throwable,
@Advice.Return Object response,
@Advice.Local("otelSpan") Span span,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
if (scope == null) {
return;
}
CallDepthThreadLocalMap.reset(Twilio.class);
scope.close();
if (throwable != null) {
tracer().endExceptionally(span, throwable);
tracer().endExceptionally(context, throwable);
} else {
tracer().end(span, response);
tracer().end(context, response);
}
}
}

View File

@ -5,12 +5,13 @@
package io.opentelemetry.javaagent.instrumentation.twilio;
import static io.opentelemetry.api.trace.Span.Kind.CLIENT;
import com.google.common.util.concurrent.ListenableFuture;
import com.twilio.rest.api.v2010.account.Call;
import com.twilio.rest.api.v2010.account.Message;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.instrumentation.api.tracer.BaseTracer;
import java.lang.reflect.Method;
import java.util.concurrent.TimeUnit;
@ -27,15 +28,18 @@ public class TwilioTracer extends BaseTracer {
return TRACER;
}
public Span startSpan(Object serviceExecutor, String methodName) {
return tracer.spanBuilder(spanNameOnServiceExecution(serviceExecutor, methodName)).startSpan();
public boolean shouldStartSpan(Context parentContext) {
return parentContext.get(CONTEXT_CLIENT_SPAN_KEY) == null;
}
@Override
public Scope startScope(Span span) {
Context context = Context.current().with(span);
context = context.with(CONTEXT_CLIENT_SPAN_KEY, span);
return context.makeCurrent();
public Context startSpan(Context parentContext, Object serviceExecutor, String methodName) {
Span span =
tracer
.spanBuilder(spanNameOnServiceExecution(serviceExecutor, methodName))
.setSpanKind(CLIENT)
.setParent(parentContext)
.startSpan();
return parentContext.with(span).with(CONTEXT_CLIENT_SPAN_KEY, span);
}
/** Decorate trace based on service execution metadata. */
@ -44,7 +48,7 @@ public class TwilioTracer extends BaseTracer {
}
/** Annotate the span with the results of the operation. */
public void end(Span span, Object result) {
public void end(Context context, Object result) {
// Unwrap ListenableFuture (if present)
if (result instanceof ListenableFuture) {
@ -55,6 +59,8 @@ public class TwilioTracer extends BaseTracer {
}
}
Span span = Span.fromContext(context);
// Nothing to do here, so return
if (result == null) {
span.end();
@ -94,6 +100,10 @@ public class TwilioTracer extends BaseTracer {
super.end(span);
}
public void endExceptionally(Context context, Throwable throwable) {
super.endExceptionally(Span.fromContext(context), throwable);
}
/**
* Helper method for calling a getter using reflection. This will be slow, so only use when
* required.

View File

@ -5,7 +5,7 @@
package test
import static io.opentelemetry.api.trace.Span.Kind.INTERNAL
import static io.opentelemetry.api.trace.Span.Kind.CLIENT
import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace
import com.fasterxml.jackson.databind.ObjectMapper
@ -142,7 +142,7 @@ class TwilioClientTest extends AgentTestRunner {
}
span(1) {
name "MessageCreator.create"
kind INTERNAL
kind CLIENT
errored false
attributes {
"twilio.type" "com.twilio.rest.api.v2010.account.Message"
@ -186,7 +186,7 @@ class TwilioClientTest extends AgentTestRunner {
}
span(1) {
name "CallCreator.create"
kind INTERNAL
kind CLIENT
errored false
attributes {
"twilio.type" "com.twilio.rest.api.v2010.account.Call"
@ -252,7 +252,7 @@ class TwilioClientTest extends AgentTestRunner {
}
span(1) {
name "MessageCreator.create"
kind INTERNAL
kind CLIENT
childOf(span(0))
errored false
attributes {
@ -334,7 +334,7 @@ class TwilioClientTest extends AgentTestRunner {
}
span(1) {
name "MessageCreator.create"
kind INTERNAL
kind CLIENT
childOf(span(0))
errored false
attributes {
@ -413,7 +413,7 @@ class TwilioClientTest extends AgentTestRunner {
message.body == "Hello, World!"
assertTraces(1) {
trace(0, 3) {
trace(0, 2) {
span(0) {
name "test"
errored false
@ -423,7 +423,7 @@ class TwilioClientTest extends AgentTestRunner {
}
span(1) {
name "MessageCreator.createAsync"
kind INTERNAL
kind CLIENT
childOf(span(0))
errored false
attributes {
@ -433,18 +433,6 @@ class TwilioClientTest extends AgentTestRunner {
"twilio.status" "sent"
}
}
span(2) {
name "MessageCreator.create"
kind INTERNAL
childOf(span(1))
errored false
attributes {
"twilio.type" "com.twilio.rest.api.v2010.account.Message"
"twilio.account" "AC14984e09e497506cf0d5eb59b1f6ace7"
"twilio.sid" "MMXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
"twilio.status" "sent"
}
}
}
}
@ -484,7 +472,7 @@ class TwilioClientTest extends AgentTestRunner {
}
span(1) {
name "MessageCreator.create"
kind INTERNAL
kind CLIENT
errored true
errorEvent(ApiException, "Testing Failure")
}
@ -512,7 +500,7 @@ class TwilioClientTest extends AgentTestRunner {
trace(0, 1) {
span(0) {
name "MessageCreator.create"
kind INTERNAL
kind CLIENT
hasNoParent()
errored false
attributes {
@ -556,7 +544,7 @@ class TwilioClientTest extends AgentTestRunner {
message.body == "Hello, World!"
assertTraces(1) {
trace(0, 3) {
trace(0, 2) {
span(0) {
name "test"
errored false
@ -566,18 +554,7 @@ class TwilioClientTest extends AgentTestRunner {
}
span(1) {
name "MessageCreator.createAsync"
kind INTERNAL
errored false
attributes {
"twilio.type" "com.twilio.rest.api.v2010.account.Message"
"twilio.account" "AC14984e09e497506cf0d5eb59b1f6ace7"
"twilio.sid" "MMXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
"twilio.status" "sent"
}
}
span(2) {
name "MessageCreator.create"
kind INTERNAL
kind CLIENT
errored false
attributes {
"twilio.type" "com.twilio.rest.api.v2010.account.Message"
@ -627,7 +604,7 @@ class TwilioClientTest extends AgentTestRunner {
expect:
assertTraces(1) {
trace(0, 3) {
trace(0, 2) {
span(0) {
name "test"
errored true
@ -636,13 +613,7 @@ class TwilioClientTest extends AgentTestRunner {
}
span(1) {
name "MessageCreator.createAsync"
kind INTERNAL
errored true
errorEvent(ApiException, "Testing Failure")
}
span(2) {
name "MessageCreator.create"
kind INTERNAL
kind CLIENT
errored true
errorEvent(ApiException, "Testing Failure")
}