End span after closing scope (#12952)

Author: trask

docs/contributing/using-instrumenter-api.md

@@ -84,14 +84,16 @@ Response decoratedMethod(Request request) {
   }
 
   Context context = instrumenter.start(parentContext, request);
+  Response response;
   try (Scope scope = context.makeCurrent()) {
-    Response response = actualMethod(request);
-    instrumenter.end(context, request, response, null);
-    return response;
-  } catch (Throwable error) {
-    instrumenter.end(context, request, null, error);
-    throw error;
+    response = actualMethod(request);
+  } catch (Throwable t) {
+    instrumenter.end(context, request, null, t);
+    throw t;
   }
+  // calling end after the scope is closed is a best practice
+  instrumenter.end(context, request, response, null);
+  return response;
 }
 ```
 

instrumentation/apache-httpclient/apache-httpclient-4.3/library/src/main/java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/TracingProtocolExec.java

@@ -89,16 +89,14 @@ private CloseableHttpResponse execute(
       HttpExecutionAware httpExecutionAware,
       Context context)
       throws IOException, HttpException {
-    CloseableHttpResponse response = null;
-    Throwable error = null;
+    CloseableHttpResponse response;
     try (Scope ignored = context.makeCurrent()) {
       response = exec.execute(route, request, httpContext, httpExecutionAware);
-      return response;
-    } catch (Throwable e) {
-      error = e;
-      throw e;
-    } finally {
-      instrumenter.end(context, instrumenterRequest, response, error);
+    } catch (Throwable t) {
+      instrumenter.end(context, instrumenterRequest, null, t);
+      throw t;
     }
+    instrumenter.end(context, instrumenterRequest, response, null);
+    return response;
   }
 }

instrumentation/elasticsearch/elasticsearch-rest-7.0/library/src/main/java/io/opentelemetry/instrumentation/elasticsearch/rest/v7_0/RestClientWrapper.java

@@ -67,19 +67,18 @@ private static Class<?> createProxyClass() {
                       return method.invoke(target, args);
                     }
 
-                    Response response = null;
-                    Throwable throwable = null;
                     Context context = instrumenter.start(parentContext, otelRequest);
-                    try (Scope scope = context.makeCurrent()) {
+
+                    Response response;
+                    try (Scope ignored = context.makeCurrent()) {
                       response = (Response) method.invoke(target, args);
-                    } catch (Throwable exception) {
-                      throwable = exception;
-                      throw throwable;
-                    } finally {
-                      instrumenter.end(context, otelRequest, response, throwable);
+                    } catch (Throwable t) {
+                      instrumenter.end(context, otelRequest, null, t);
+                      throw t;
                     }
-
+                    instrumenter.end(context, otelRequest, response, null);
                     return response;
+
                   } else if ("performRequestAsync".equals(method.getName())
                       && args.length == 2
                       && args[0] instanceof Request
@@ -94,22 +93,17 @@ private static Class<?> createProxyClass() {
                       return method.invoke(target, args);
                     }
 
-                    Throwable throwable = null;
                     Context context = instrumenter.start(parentContext, otelRequest);
                     args[1] =
                         new RestResponseListener(
                             responseListener, parentContext, instrumenter, context, otelRequest);
-                    try (Scope scope = context.makeCurrent()) {
+                    try (Scope ignored = context.makeCurrent()) {
                       return method.invoke(target, args);
-                    } catch (Throwable exception) {
-                      throwable = exception;
-                      throw throwable;
-                    } finally {
-                      if (throwable != null) {
-                        instrumenter.end(context, otelRequest, null, throwable);
-                      }
-                      // span ended in RestResponseListener
+                    } catch (Throwable t) {
+                      instrumenter.end(context, otelRequest, null, t);
+                      throw t;
                     }
+                    // span ended in RestResponseListener
                   }
 
                   // delegate to wrapped RestClient

instrumentation/grpc-1.6/library/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/TracingClientInterceptor.java

@@ -58,13 +58,11 @@ public <REQUEST, RESPONSE> ClientCall<REQUEST, RESPONSE> interceptCall(
     Context context = instrumenter.start(parentContext, request);
     ClientCall<REQUEST, RESPONSE> result;
     try (Scope ignored = context.makeCurrent()) {
-      try {
-        // call other interceptors
-        result = next.newCall(method, callOptions);
-      } catch (Throwable e) {
-        instrumenter.end(context, request, Status.UNKNOWN, e);
-        throw e;
-      }
+      // call other interceptors
+      result = next.newCall(method, callOptions);
+    } catch (Throwable t) {
+      instrumenter.end(context, request, Status.UNKNOWN, t);
+      throw t;
     }
 
     return new TracingClientCall<>(result, parentContext, context, request);

instrumentation/java-http-client/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/httpclient/HttpHeadersInstrumentation.java

@@ -11,6 +11,7 @@
 import static net.bytebuddy.matcher.ElementMatchers.nameStartsWith;
 import static net.bytebuddy.matcher.ElementMatchers.named;
 
+import io.opentelemetry.context.Context;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import java.net.http.HttpHeaders;
@@ -39,7 +40,7 @@ public static class HeadersAdvice {
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void methodExit(@Advice.Return(readOnly = false) HttpHeaders headers) {
-      headers = setter().inject(headers);
+      headers = setter().inject(headers, Context.current());
     }
   }
 }

instrumentation/java-http-client/library/src/main/java/io/opentelemetry/instrumentation/httpclient/internal/HttpHeadersSetter.java

@@ -25,13 +25,13 @@ public HttpHeadersSetter(ContextPropagators contextPropagators) {
     this.contextPropagators = contextPropagators;
   }
 
-  public HttpHeaders inject(HttpHeaders original) {
+  public HttpHeaders inject(HttpHeaders original, Context context) {
     Map<String, List<String>> headerMap = new HashMap<>(original.map());
 
     contextPropagators
         .getTextMapPropagator()
         .inject(
-            Context.current(),
+            context,
             headerMap,
             (carrier, key, value) -> {
               if (carrier != null) {

instrumentation/java-http-client/library/src/main/java/io/opentelemetry/instrumentation/httpclient/internal/OpenTelemetryHttpClient.java

@@ -95,21 +95,17 @@ public <T> HttpResponse<T> send(
       return client.send(request, responseBodyHandler);
     }
 
-    HttpResponse<T> response = null;
-    Throwable error = null;
     Context context = instrumenter.start(parentContext, request);
+    HttpRequestWrapper requestWrapper =
+        new HttpRequestWrapper(request, headersSetter.inject(request.headers(), context));
+    HttpResponse<T> response;
     try (Scope ignore = context.makeCurrent()) {
-      HttpRequestWrapper requestWrapper =
-          new HttpRequestWrapper(request, headersSetter.inject(request.headers()));
-
       response = client.send(requestWrapper, responseBodyHandler);
-    } catch (Throwable throwable) {
-      error = throwable;
-      throw throwable;
-    } finally {
-      instrumenter.end(context, request, response, error);
+    } catch (Throwable t) {
+      instrumenter.end(context, request, null, t);
+      throw t;
     }
-
+    instrumenter.end(context, request, response, null);
     return response;
   }
 
@@ -136,17 +132,18 @@ private <T> CompletableFuture<HttpResponse<T>> traceAsync(
     }
 
     Context context = instrumenter.start(parentContext, request);
-    try (Scope ignore = context.makeCurrent()) {
-      HttpRequestWrapper requestWrapper =
-          new HttpRequestWrapper(request, headersSetter.inject(request.headers()));
-
-      CompletableFuture<HttpResponse<T>> future = action.apply(requestWrapper);
-      future = future.whenComplete(new ResponseConsumer(instrumenter, context, request));
-      future = CompletableFutureWrapper.wrap(future, parentContext);
-      return future;
-    } catch (Throwable throwable) {
-      instrumenter.end(context, request, null, throwable);
-      throw throwable;
+    HttpRequestWrapper requestWrapper =
+        new HttpRequestWrapper(request, headersSetter.inject(request.headers(), context));
+
+    CompletableFuture<HttpResponse<T>> future;
+    try (Scope ignored = context.makeCurrent()) {
+      future = action.apply(requestWrapper);
+    } catch (Throwable t) {
+      instrumenter.end(context, request, null, t);
+      throw t;
     }
+    future = future.whenComplete(new ResponseConsumer(instrumenter, context, request));
+    future = CompletableFutureWrapper.wrap(future, parentContext);
+    return future;
   }
 }

instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/KafkaTelemetry.java

@@ -252,10 +252,10 @@ <K, V> Future<RecordMetadata> buildAndInjectSpan(
     }
 
     Context context = producerInstrumenter.start(parentContext, request);
+    propagator().inject(context, record.headers(), SETTER);
+
     try (Scope ignored = context.makeCurrent()) {
-      propagator().inject(context, record.headers(), SETTER);
-      callback = new ProducerCallback(callback, parentContext, context, request);
-      return sendFn.apply(record, callback);
+      return sendFn.apply(record, new ProducerCallback(callback, parentContext, context, request));
     }
   }
 

instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/HttpClientRequestTracingHandler.java

@@ -55,10 +55,10 @@ public void writeRequested(ChannelHandlerContext ctx, MessageEvent event) throws
 
     try (Scope ignored = context.makeCurrent()) {
       super.writeRequested(ctx, event);
-      // span is ended normally in HttpClientResponseTracingHandler
     } catch (Throwable throwable) {
       instrumenter().end(context, request, null, throwable);
       throw throwable;
     }
+    // span is ended normally in HttpClientResponseTracingHandler
   }
 }

instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/server/HttpServerRequestTracingHandler.java

@@ -51,10 +51,10 @@ public void messageReceived(ChannelHandlerContext ctx, MessageEvent event) throw
 
     try (Scope ignored = context.makeCurrent()) {
       super.messageReceived(ctx, event);
-      // the span is ended normally in HttpServerResponseTracingHandler
     } catch (Throwable throwable) {
       instrumenter().end(context, request, null, throwable);
       throw throwable;
     }
+    // span is ended normally in HttpServerResponseTracingHandler
   }
 }