Backport changes that were learned in V2 impl (#113)

Author: devinsba

brave-instrumentation/aws-java-sdk-core/README.md

@@ -1,24 +1,27 @@
 # AWS SDK Instrumentation
 
-This module contains instrumentation for [AWS](https://github.com/aws/aws-sdk-java) clients that extend
-`AmazonWebServiceClient`.
+This module contains instrumentation for [AWS](https://github.com/aws/aws-sdk-java) clients that
+extend `AmazonWebServiceClient`.
 
-The `AwsClientTracing` class finalizes your `AwsClientBuilder` instances by adding a `RequestHandler2` instance to your client
-and wrapping the `ExecutorService` in the case of an async client.
+The `AwsClientTracing` class finalizes your `AwsClientBuilder` instances by adding a
+`RequestHandler2` instance to your client and wrapping the `ExecutorService` in the case of an async
+client.
 
 ## Span Model
 
-Traces AWS Java SDK calls. Adds on the standard zipkin/brave http tags, as well as tags that align with the XRay data model.
+Traces AWS Java SDK calls. Adds on the standard zipkin/brave http tags, as well as tags that align
+with the XRay data model.
 
 This implementation creates 2 types of spans to allow for better error visibility.
 
-The outer span, "Application Span", wraps the whole SDK operation. This span uses the AWS service as it's name and will NOT
-have a remoteService configuration, making it a local span. If the entire operation results in an error then this span will
-have an error tag with the cause.
+The outer span, "Application Span", wraps the whole SDK operation. This span uses `aws-sdk` as it's
+name and will NOT have a remoteService configuration, making it a local span. If the entire
+operation results in an error then this span will have an error tag with the cause.
 
-The inner span, "Client Span", is created for each outgoing HTTP request. This span will be of type CLIENT. The remoteService
-will be the name of the AWS service, and the span name will be the name of the operation being done. If the request results in
-an error then the span will be tagged with the error. The AWS request ID is added when available.
+The inner span, "Client Span", is created for each outgoing HTTP request. This span will be of type
+CLIENT. The remoteService will be the name of the AWS service, and the span name will be the name of
+the operation being done. If the request results in an error then the span will be tagged with the
+error. The AWS request ID is added when available.
 
 ## Wiring it up
 

brave-instrumentation/aws-java-sdk-core/src/main/java/brave/instrumentation/aws/TracingRequestHandler.java

@@ -37,9 +37,9 @@
  *
  * This implementation creates 2 types of spans to allow for better error visibility.
  *
- * The outer span, "Application Span", wraps the whole SDK operation. This span uses the AWS service
- * as it's name and will NOT have a remoteService configuration, making it a local span. If the
- * entire operation results in an error then this span will have an error tag with the cause.
+ * The outer span, "Application Span", wraps the whole SDK operation. This span uses aws-sdk as it's
+ * name and will NOT have a remoteService configuration, making it a local span. If the entire
+ * operation results in an error then this span will have an error tag with the cause.
  *
  * The inner span, "Client Span", is created for each outgoing HTTP request. This span will be of
  * type CLIENT. The remoteService will be the name of the AWS service, and the span name will be the
@@ -49,8 +49,8 @@
 final class TracingRequestHandler extends RequestHandler2 {
   static final HandlerContextKey<Span> APPLICATION_SPAN =
       new HandlerContextKey<>("APPLICATION_SPAN");
-  static final HandlerContextKey<TraceContext> DEFERRED_ROOT_SPAN =
-      new HandlerContextKey<>("DEFERRED_ROOT_SPAN");
+  static final HandlerContextKey<TraceContext> DEFERRED_ROOT_CONTEXT =
+      new HandlerContextKey<>("DEFERRED_ROOT_CONTEXT");
   static final HandlerContextKey<Span> CLIENT_SPAN =
       new HandlerContextKey<>(Span.class.getCanonicalName());
 
@@ -79,31 +79,31 @@ final class TracingRequestHandler extends RequestHandler2 {
     Span applicationSpan = tracer.nextSpan();
     // new root span, but we don't yet know if we should sample it
     if (applicationSpan.context().parentIdAsLong() == 0) {
-      request.addHandlerContext(
-          DEFERRED_ROOT_SPAN,
-          applicationSpan.context().toBuilder().sampled(null).build()
-      );
+      request.addHandlerContext(DEFERRED_ROOT_CONTEXT, applicationSpan.context());
     } else {
       request.addHandlerContext(APPLICATION_SPAN, applicationSpan.start());
     }
     return request;
   }
 
   @Override public void beforeAttempt(HandlerBeforeAttemptContext context) {
-    TraceContext deferredRootSpan = context.getRequest().getHandlerContext(DEFERRED_ROOT_SPAN);
+    TraceContext deferredRootContext = context.getRequest().getHandlerContext(DEFERRED_ROOT_CONTEXT);
     Span applicationSpan;
-    if (deferredRootSpan != null) {
+    if (deferredRootContext != null) {
       Boolean sampled = httpTracing.clientSampler().trySample(ADAPTER, context.getRequest());
       if (sampled == null) {
-        sampled = httpTracing.tracing().sampler().isSampled(deferredRootSpan.traceId());
+        sampled = httpTracing.tracing().sampler().isSampled(deferredRootContext.traceId());
       }
-      applicationSpan = tracer.toSpan(deferredRootSpan.toBuilder().sampled(sampled).build());
+      applicationSpan = tracer.toSpan(deferredRootContext.toBuilder().sampled(sampled).build());
       context.getRequest().addHandlerContext(APPLICATION_SPAN, applicationSpan.start());
     } else {
       applicationSpan = context.getRequest().getHandlerContext(APPLICATION_SPAN);
     }
-    applicationSpan.name(context.getRequest().getServiceName());
-    Span clientSpan = nextClientSpan(context.getRequest(), applicationSpan);
+    String operation = getAwsOperationFromRequest(context.getRequest());
+    applicationSpan.name("aws-sdk")
+        .tag("aws.service_name", context.getRequest().getServiceName())
+        .tag("aws.operation", operation);
+    Span clientSpan = nextClientSpan(context.getRequest(), applicationSpan, operation);
     context.getRequest().addHandlerContext(CLIENT_SPAN, clientSpan);
   }
 
@@ -129,15 +129,11 @@ final class TracingRequestHandler extends RequestHandler2 {
     applicationSpan.finish();
   }
 
-  private Span nextClientSpan(Request<?> request, Span applicationSpan) {
+  private Span nextClientSpan(Request<?> request, Span applicationSpan, String operation) {
     Span span = tracer.newChild(applicationSpan.context());
     handler.handleSend(injector, request, span);
-    String operation = getAwsOperationFromRequest(request);
-    span.name(operation);
-    span.remoteServiceName(request.getServiceName());
-    span.tag("aws.service_name", request.getServiceName());
-    span.tag("aws.operation", operation);
-    return span;
+    return span.name(operation)
+        .remoteServiceName(request.getServiceName());
   }
 
   private String getAwsOperationFromRequest(Request<?> request) {

brave-instrumentation/aws-java-sdk-core/src/test/java/brave/instrumentation/aws/AwsClientTracingTest.java

@@ -88,11 +88,10 @@ public void testSpanCreatedAndTagsApplied() throws InterruptedException {
     Span httpSpan = spans.poll(100, TimeUnit.MILLISECONDS);
     assertThat(httpSpan.remoteServiceName()).isEqualToIgnoringCase("amazondynamodbv2");
     assertThat(httpSpan.name()).isEqualToIgnoringCase("deleteitem");
-    assertThat(httpSpan.tags().get("aws.operation")).isEqualToIgnoringCase("deleteitem");
     assertThat(httpSpan.tags().get("aws.request_id")).isEqualToIgnoringCase("abcd");
 
     Span sdkSpan = spans.poll(100, TimeUnit.MILLISECONDS);
-    assertThat(sdkSpan.name()).isEqualToIgnoringCase("amazondynamodbv2");
+    assertThat(sdkSpan.name()).isEqualToIgnoringCase("aws-sdk");
   }
 
   private MockResponse createDeleteItemResponse() {