Address comments by @anuraaga

brave/build.gradle

@@ -1,6 +1,5 @@
 dependencies {
     testCompile project(':thrift')
 
-    compile project(':zipkin')
-    compile 'io.zipkin.brave:brave-instrumentation-http'
+    compile 'io.zipkin.brave:brave'
 }

brave/src/main/java/com/linecorp/armeria/client/brave/BraveClient.java

@@ -16,53 +16,150 @@
 
 package com.linecorp.armeria.client.brave;
 
+import static com.google.common.base.Strings.isNullOrEmpty;
 import static com.linecorp.armeria.common.brave.RequestContextCurrentTraceContext.ensureScopeUsesRequestContext;
 
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.SocketAddress;
 import java.util.function.Function;
 
+import javax.annotation.Nullable;
+
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import com.linecorp.armeria.client.Client;
-import com.linecorp.armeria.client.tracing.HttpTracingClient;
+import com.linecorp.armeria.client.ClientRequestContext;
+import com.linecorp.armeria.client.SimpleDecoratingClient;
 import com.linecorp.armeria.common.HttpRequest;
 import com.linecorp.armeria.common.HttpResponse;
-import com.linecorp.armeria.common.brave.RequestContextCurrentTraceContext;
+import com.linecorp.armeria.common.RequestHeadersBuilder;
+import com.linecorp.armeria.common.logging.RequestLog;
+import com.linecorp.armeria.common.logging.RequestLogAvailability;
+import com.linecorp.armeria.internal.brave.AsciiStringKeyFactory;
+import com.linecorp.armeria.internal.brave.SpanContextUtil;
+import com.linecorp.armeria.internal.brave.SpanTags;
+import com.linecorp.armeria.internal.brave.TraceContextUtil;
 
+import brave.Span;
+import brave.Span.Kind;
+import brave.Tracer;
+import brave.Tracer.SpanInScope;
 import brave.Tracing;
-import brave.http.HttpTracing;
+import brave.propagation.TraceContext;
 
 /**
  * Decorates a {@link Client} to trace outbound {@link HttpRequest}s using
  * <a href="https://github.com/openzipkin/brave">Brave</a>.
- *
- * <p>This decorator puts trace data into HTTP headers. The specifications of header names and its values
- * correspond to <a href="http://zipkin.io/">Zipkin</a>.
  */
-public final class BraveClient extends HttpTracingClient {
+public final class BraveClient extends SimpleDecoratingClient<HttpRequest, HttpResponse> {
 
     private static final Logger logger = LoggerFactory.getLogger(BraveClient.class);
 
+    // TODO(minwoox) Add the variant which takes HttpTracing.
+
     /**
      * Creates a new tracing {@link Client} decorator using the specified {@link Tracing} instance.
      */
+    public static Function<Client<HttpRequest, HttpResponse>, BraveClient> newDecorator(Tracing tracing) {
+        return newDecorator(tracing, null);
+    }
+
+    /**
+     * Creates a new tracing {@link Client} decorator using the specified {@link Tracing} instance
+     * and remote service name.
+     */
     public static Function<Client<HttpRequest, HttpResponse>, BraveClient> newDecorator(
-            HttpTracing httpTracing) {
+            Tracing tracing, @Nullable String remoteServiceName) {
         try {
-            ensureScopeUsesRequestContext(httpTracing.tracing());
+            ensureScopeUsesRequestContext(tracing);
         } catch (IllegalStateException e) {
             logger.warn("{} - it is appropriate to ignore this warning if this client is not being used " +
                         "inside an Armeria server (e.g., this is a normal spring-mvc tomcat server).",
                         e.getMessage());
         }
-        return delegate -> new BraveClient(delegate, httpTracing);
+        return delegate -> new BraveClient(delegate, tracing, remoteServiceName);
     }
 
+    private final Tracer tracer;
+    private final TraceContext.Injector<RequestHeadersBuilder> injector;
+    @Nullable
+    private final String remoteServiceName;
+
     /**
      * Creates a new instance.
      */
-    private BraveClient(Client<HttpRequest, HttpResponse> delegate, HttpTracing httpTracing) {
-        super(delegate, httpTracing.tracing(), httpTracing.serverName(),
-              RequestContextCurrentTraceContext::copy);
+    private BraveClient(Client<HttpRequest, HttpResponse> delegate, Tracing tracing,
+                        @Nullable String remoteServiceName) {
+        super(delegate);
+        tracer = tracing.tracer();
+        injector = tracing.propagationFactory().create(AsciiStringKeyFactory.INSTANCE)
+                          .injector(RequestHeadersBuilder::set);
+        this.remoteServiceName = remoteServiceName;
+    }
+
+    @Override
+    public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Exception {
+        final Span span = tracer.nextSpan();
+
+        // Inject the headers.
+        final RequestHeadersBuilder newHeaders = req.headers().toBuilder();
+        injector.inject(span.context(), newHeaders);
+        req = HttpRequest.of(req, newHeaders.build());
+        ctx.updateRequest(req);
+
+        // For no-op spans, we only need to inject into headers and don't set any other attributes.
+        if (span.isNoop()) {
+            return delegate().execute(ctx, req);
+        }
+
+        final String method = ctx.method().name();
+        span.kind(Kind.CLIENT).name(method);
+        ctx.log().addListener(log -> SpanContextUtil.startSpan(span, log),
+                              RequestLogAvailability.REQUEST_START);
+
+        // Ensure the trace context propagates to children
+        ctx.onChild(TraceContextUtil::copy);
+
+        ctx.log().addListener(log -> {
+            SpanTags.logWireSend(span, log.requestFirstBytesTransferredTimeNanos(), log);
+
+            // If the client timed-out the request, we will have never received any response data at all.
+            if (log.isAvailable(RequestLogAvailability.RESPONSE_FIRST_BYTES_TRANSFERRED)) {
+                SpanTags.logWireReceive(span, log.responseFirstBytesTransferredTimeNanos(), log);
+            }
+
+            finishSpan(span, log);
+        }, RequestLogAvailability.COMPLETE);
+
+        try (SpanInScope ignored = tracer.withSpanInScope(span)) {
+            return delegate().execute(ctx, req);
+        }
+    }
+
+    private void finishSpan(Span span, RequestLog log) {
+        setRemoteEndpoint(span, log);
+        SpanContextUtil.closeSpan(span, log);
+    }
+
+    private void setRemoteEndpoint(Span span, RequestLog log) {
+        final SocketAddress remoteAddress = log.context().remoteAddress();
+        final InetAddress address;
+        final int port;
+        if (remoteAddress instanceof InetSocketAddress) {
+            final InetSocketAddress socketAddress = (InetSocketAddress) remoteAddress;
+            address = socketAddress.getAddress();
+            port = socketAddress.getPort();
+        } else {
+            address = null;
+            port = 0;
+        }
+        if (!isNullOrEmpty(remoteServiceName)) {
+            span.remoteServiceName(remoteServiceName);
+        }
+        if (address != null) {
+            span.remoteIpAndPort(address.getHostAddress(), port);
+        }
     }
 }

brave/src/main/java/com/linecorp/armeria/common/brave/RequestContextCurrentTraceContext.java

@@ -16,6 +16,7 @@
 
 package com.linecorp.armeria.common.brave;
 
+import static com.linecorp.armeria.internal.brave.TraceContextUtil.getTraceContextAttribute;
 import static java.util.Objects.requireNonNull;
 
 import java.util.Collections;
@@ -37,7 +38,6 @@
 import brave.propagation.CurrentTraceContext;
 import brave.propagation.TraceContext;
 import io.netty.util.Attribute;
-import io.netty.util.AttributeKey;
 
 /**
  * {@linkplain Tracing.Builder#currentTraceContext(CurrentTraceContext) Tracing
@@ -49,19 +49,9 @@
  */
 public final class RequestContextCurrentTraceContext extends CurrentTraceContext {
 
-    /**
-     * Use this singleton when building a {@link Tracing} instance for use with
-     * {@link BraveService} or {@link BraveClient}.
-     *
-     * <p>If you need to customize the context, use {@link #builder()} instead.
-     *
-     * @see Tracing.Builder#currentTraceContext(CurrentTraceContext)
-     */
-    public static final CurrentTraceContext DEFAULT = new RequestContextCurrentTraceContext(new Builder());
+    private static final CurrentTraceContext DEFAULT = new RequestContextCurrentTraceContext(new Builder());
 
     private static final Logger logger = LoggerFactory.getLogger(RequestContextCurrentTraceContext.class);
-    private static final AttributeKey<TraceContext> TRACE_CONTEXT_KEY =
-            AttributeKey.valueOf(RequestContextCurrentTraceContext.class, "TRACE_CONTEXT");
 
     // Thread-local for storing TraceContext when invoking callbacks off the request thread.
     private static final ThreadLocal<TraceContext> THREAD_LOCAL_CONTEXT = new ThreadLocal<>();
@@ -97,6 +87,18 @@ public CurrentTraceContext build() {
         }
     }
 
+    /**
+     * Returns the default {@link CurrentTraceContext}. Use this when building a {@link Tracing} instance
+     * for use with {@link BraveService} or {@link BraveClient}.
+     *
+     * <p>If you need to customize the context, use {@link #builder()} instead.
+     *
+     * @see Tracing.Builder#currentTraceContext(CurrentTraceContext)
+     */
+    public static CurrentTraceContext ofDefault() {
+        return DEFAULT;
+    }
+
     /**
      * Use this when you need customizations such as log integration via
      * {@linkplain Builder#addScopeDecorator(ScopeDecorator)}.
@@ -127,23 +129,10 @@ public static void ensureScopeUsesRequestContext(Tracing tracing) {
                     "Tracing.currentTraceContext is not a " + RequestContextCurrentTraceContext.class
                             .getSimpleName() + " scope. " +
                     "Please call Tracing.Builder.currentTraceContext(" + RequestContextCurrentTraceContext.class
-                            .getSimpleName() + ".DEFAULT).");
+                            .getSimpleName() + ".ofDefault()).");
         }
     }
 
-    /**
-     * Use this to ensure the trace context propagates to children.
-     *
-     * <p>Ex.
-     * <pre>{@code
-     *  // Ensure the trace context propagates to children
-     * ctx.onChild(RequestContextCurrentTraceContext::copy);
-     * }</pre>
-     */
-    public static void copy(RequestContext src, RequestContext dst) {
-        dst.attr(TRACE_CONTEXT_KEY).set(src.attr(TRACE_CONTEXT_KEY).get());
-    }
-
     private RequestContextCurrentTraceContext(Builder builder) {
         super(builder);
     }
@@ -156,14 +145,14 @@ public TraceContext get() {
             return null;
         }
         if (ctx.eventLoop().inEventLoop()) {
-            return ctx.attr(TRACE_CONTEXT_KEY).get();
+            return getTraceContextAttribute(ctx).get();
         } else {
             final TraceContext threadLocalContext = THREAD_LOCAL_CONTEXT.get();
             if (threadLocalContext != null) {
                 return threadLocalContext;
             }
             // First span on a non-request thread will use the request's TraceContext as a parent.
-            return ctx.attr(TRACE_CONTEXT_KEY).get();
+            return getTraceContextAttribute(ctx).get();
         }
     }
 
@@ -190,7 +179,7 @@ public Scope newScope(@Nullable TraceContext currentSpan) {
     }
 
     private Scope createScopeForRequestThread(RequestContext ctx, @Nullable TraceContext currentSpan) {
-        final Attribute<TraceContext> traceContextAttribute = ctx.attr(TRACE_CONTEXT_KEY);
+        final Attribute<TraceContext> traceContextAttribute = getTraceContextAttribute(ctx);
 
         final TraceContext previous = traceContextAttribute.getAndSet(currentSpan);
 
@@ -252,7 +241,7 @@ private static Attribute<TraceContext> getTraceContextAttributeOrWarnOnce() {
         if (ctx == null) {
             return null;
         }
-        return ctx.attr(TRACE_CONTEXT_KEY);
+        return getTraceContextAttribute(ctx);
     }
 
     private enum LogRequestContextWarningOnce implements Supplier<RequestContext> {

zipkin/src/main/java/com/linecorp/armeria/internal/tracing/AsciiStringKeyFactory.java → brave/src/main/java/com/linecorp/armeria/internal/brave/AsciiStringKeyFactory.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 LINE Corporation
+ * Copyright 2019 LINE Corporation
  *
  * LINE Corporation licenses this file to you under the Apache License,
  * version 2.0 (the "License"); you may not use this file except in compliance
@@ -14,7 +14,7 @@
  * under the License.
  */
 
-package com.linecorp.armeria.internal.tracing;
+package com.linecorp.armeria.internal.brave;
 
 import com.linecorp.armeria.common.HttpHeaderNames;
 import com.linecorp.armeria.common.HttpHeaders;

zipkin/src/main/java/com/linecorp/armeria/internal/tracing/SpanContextUtil.java → brave/src/main/java/com/linecorp/armeria/internal/brave/SpanContextUtil.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 LINE Corporation
+ * Copyright 2019 LINE Corporation
  *
  * LINE Corporation licenses this file to you under the Apache License,
  * version 2.0 (the "License"); you may not use this file except in compliance
@@ -14,7 +14,7 @@
  * under the License.
  */
 
-package com.linecorp.armeria.internal.tracing;
+package com.linecorp.armeria.internal.brave;
 
 import java.util.concurrent.TimeUnit;
 

zipkin/src/main/java/com/linecorp/armeria/internal/tracing/SpanTags.java → brave/src/main/java/com/linecorp/armeria/internal/brave/SpanTags.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 LINE Corporation
+ * Copyright 2019 LINE Corporation
  *
  * LINE Corporation licenses this file to you under the Apache License,
  * version 2.0 (the "License"); you may not use this file except in compliance
@@ -14,7 +14,7 @@
  * under the License.
  */
 
-package com.linecorp.armeria.internal.tracing;
+package com.linecorp.armeria.internal.brave;
 
 import java.net.SocketAddress;
 

brave/src/main/java/com/linecorp/armeria/internal/brave/TraceContextUtil.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright 2019 LINE Corporation
+ *
+ * LINE Corporation licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.linecorp.armeria.internal.brave;
+
+import com.linecorp.armeria.common.RequestContext;
+
+import brave.propagation.TraceContext;
+import io.netty.util.Attribute;
+import io.netty.util.AttributeKey;
+
+public final class TraceContextUtil {
+
+    private static final AttributeKey<TraceContext> TRACE_CONTEXT_KEY =
+            AttributeKey.valueOf(TraceContextUtil.class, "TRACE_CONTEXT");
+
+    /**
+     * Use this to ensure the trace context propagates to children.
+     *
+     * <p>Ex.
+     * <pre>{@code
+     *  // Ensure the trace context propagates to children
+     * ctx.onChild(RequestContextCurrentTraceContext::copy);
+     * }</pre>
+     */
+    public static void copy(RequestContext src, RequestContext dst) {
+        dst.attr(TRACE_CONTEXT_KEY).set(src.attr(TRACE_CONTEXT_KEY).get());
+    }
+
+    public static Attribute<TraceContext> getTraceContextAttribute(RequestContext ctx) {
+        return ctx.attr(TRACE_CONTEXT_KEY);
+    }
+
+    private TraceContextUtil() {}
+}