Decouples AsyncZipkinSpanHandler from ZipkinSpanHandler to avoid leak (…

…#238)

* Decouples AsyncZipkinSpanHandler from ZipkinSpanHandler to avoid leak

ZipkinSpanHandler leaks the `zipkin2.Span` type through its factory
methods. This prevents libraries that scan types, such as Spring, from
avoiding a zipkin dep. The error shows up like below.

The alternative is to not share a base class that uses a type you can't
load. So, this changes AsyncZipkinSpanHandler to not extend
ZipkinSpanHandler. This allows a portable change, except the more
extreme interpretation, and lets spring apps exclude zipkin2.Span (or
rather not include it when using brave).

```
Caused by: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'zipkinSpanHandler' defined in class path resource [brave/example/TracingAutoConfiguration.class]: Initialization of bean failed; nested exception is java.lang.TypeNotPresentException: Type zipkin2.Span not present
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:562) ~[spring-beans-4.3.25.RELEASE.jar:4.3.25.RELEASE]
--snip--
```

Signed-off-by: Adrian Cole <adrian@tetrate.io>

brave/src/main/java/zipkin2/reporter/brave/AsyncZipkinSpanHandler.java

@@ -16,9 +16,12 @@
 import brave.Tag;
 import brave.handler.MutableSpan;
 import brave.handler.SpanHandler;
+import brave.propagation.TraceContext;
+import java.io.Closeable;
 import java.io.Flushable;
 import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
+import zipkin2.reporter.Reporter;
 import zipkin2.reporter.ReporterMetrics;
 import zipkin2.reporter.Sender;
 import zipkin2.reporter.internal.AsyncReporter;
@@ -39,7 +42,7 @@
  * @see brave.Tracing.Builder#addSpanHandler(SpanHandler)
  * @since 2.14
  */
-public final class AsyncZipkinSpanHandler extends ZipkinSpanHandler implements Flushable {
+public final class AsyncZipkinSpanHandler extends SpanHandler implements Closeable, Flushable {
   /** @since 2.14 */
   public static AsyncZipkinSpanHandler create(Sender sender) {
     return newBuilder(sender).build();
@@ -51,6 +54,14 @@ public static Builder newBuilder(Sender sender) {
     return new Builder(sender);
   }
 
+  /**
+   * Allows this instance to be reconfigured, for example {@link ZipkinSpanHandler.Builder#alwaysReportSpans(boolean)}.
+   *
+   * <p><em>Note:</em> Call {@link #close()} if you no longer need this instance, as otherwise it
+   * can leak resources.
+   *
+   * @since 2.15
+   */
   public Builder toBuilder() {
     return new Builder(this);
   }
@@ -146,19 +157,53 @@ public AsyncZipkinSpanHandler build() {
     }
   }
 
+  final Reporter<MutableSpan> spanReporter;
+  final Tag<Throwable> errorTag; // for toBuilder()
+  final boolean alwaysReportSpans;
+
   AsyncZipkinSpanHandler(Builder builder) {
-    super(
-      builder.delegate.build(new JsonV2Encoder(builder.errorTag)),
-      builder.errorTag,
-      builder.alwaysReportSpans
-    );
+    this.spanReporter = builder.delegate.build(new JsonV2Encoder(builder.errorTag));
+    this.errorTag = builder.errorTag;
+    this.alwaysReportSpans = builder.alwaysReportSpans;
   }
 
   @Override public void flush() {
     ((AsyncReporter<MutableSpan>) spanReporter).flush();
   }
 
+  /**
+   * Implementations that throw exceptions on close have bugs. This may result in log warnings,
+   * though.
+   *
+   * @since 2.15
+   */
   @Override public void close() {
     ((AsyncReporter<MutableSpan>) spanReporter).close();
   }
+
+  @Override public boolean end(TraceContext context, MutableSpan span, Cause cause) {
+    if (!alwaysReportSpans && !Boolean.TRUE.equals(context.sampled())) return true;
+    spanReporter.report(span);
+    return true;
+  }
+
+  @Override public String toString() {
+    return spanReporter.toString();
+  }
+
+  /**
+   * Overridden to avoid duplicates when added via {@link brave.Tracing.Builder#addSpanHandler(SpanHandler)}
+   */
+  @Override public boolean equals(Object o) {
+    if (o == this) return true;
+    if (!(o instanceof AsyncZipkinSpanHandler)) return false;
+    return spanReporter.equals(((AsyncZipkinSpanHandler) o).spanReporter);
+  }
+
+  /**
+   * Overridden to avoid duplicates when added via {@link brave.Tracing.Builder#addSpanHandler(SpanHandler)}
+   */
+  @Override public int hashCode() {
+    return spanReporter.hashCode();
+  }
 }

brave/src/test/java/zipkin2/reporter/brave/BasicUsageTest.java

@@ -14,6 +14,7 @@
 package zipkin2.reporter.brave;
 
 import brave.Tracing;
+import brave.handler.SpanHandler;
 import brave.propagation.B3SingleFormat;
 import brave.propagation.TraceContext;
 import brave.propagation.TraceContextOrSamplingFlags;
@@ -26,13 +27,17 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-abstract class BasicUsageTest<H extends ZipkinSpanHandler> {
+abstract class BasicUsageTest<H extends SpanHandler> {
   List<Span> spans = new ArrayList<>();
   H zipkinSpanHandler;
   Tracing tracing;
 
   abstract H zipkinSpanHandler(List<Span> spans);
 
+  abstract void close(H handler);
+
+  abstract SpanHandler alwaysReportSpans(H handler);
+
   @BeforeEach void init() {
     zipkinSpanHandler = zipkinSpanHandler(spans);
     tracing = Tracing.newBuilder()
@@ -49,9 +54,9 @@ abstract class BasicUsageTest<H extends ZipkinSpanHandler> {
 
   @Test void reconfigure() {
     tracing.close();
-    zipkinSpanHandler.close();
+    close(zipkinSpanHandler);
 
-    zipkinSpanHandler = (H) zipkinSpanHandler.toBuilder().alwaysReportSpans(true).build();
+    zipkinSpanHandler = (H) alwaysReportSpans(zipkinSpanHandler);
     tracing = Tracing.newBuilder()
       .localServiceName("Aa")
       .localIp("1.2.3.4")

brave/src/test/java/zipkin2/reporter/brave/BasicUsageTest_Async.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2023 The OpenZipkin Authors
+ * Copyright 2016-2024 The OpenZipkin Authors
  *
  * Licensed 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
@@ -13,6 +13,7 @@
  */
 package zipkin2.reporter.brave;
 
+import brave.handler.SpanHandler;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 import org.junit.jupiter.api.extension.RegisterExtension;
@@ -27,8 +28,16 @@ class BasicUsageTest_Async extends BasicUsageTest<AsyncZipkinSpanHandler> {
 
   @Override AsyncZipkinSpanHandler zipkinSpanHandler(List<Span> spans) {
     return AsyncZipkinSpanHandler.newBuilder(sender)
-        .messageTimeout(0, TimeUnit.MILLISECONDS) // don't spawn a thread
-        .build();
+      .messageTimeout(0, TimeUnit.MILLISECONDS) // don't spawn a thread
+      .build();
+  }
+
+  @Override void close(AsyncZipkinSpanHandler handler) {
+    handler.close();
+  }
+
+  @Override SpanHandler alwaysReportSpans(AsyncZipkinSpanHandler handler) {
+    return handler.toBuilder().alwaysReportSpans(true).build();
   }
 
   @Override void triggerReport() {

brave/src/test/java/zipkin2/reporter/brave/BasicUsageTest_Converting.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2023 The OpenZipkin Authors
+ * Copyright 2016-2024 The OpenZipkin Authors
  *
  * Licensed 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
@@ -13,11 +13,20 @@
  */
 package zipkin2.reporter.brave;
 
+import brave.handler.SpanHandler;
 import java.util.List;
 import zipkin2.Span;
 
 class BasicUsageTest_Converting extends BasicUsageTest<ZipkinSpanHandler> {
   @Override ZipkinSpanHandler zipkinSpanHandler(List<Span> spans) {
     return (ZipkinSpanHandler) ZipkinSpanHandler.create(spans::add);
   }
+
+  @Override void close(ZipkinSpanHandler handler) {
+    handler.close();
+  }
+
+  @Override SpanHandler alwaysReportSpans(ZipkinSpanHandler handler) {
+    return handler.toBuilder().alwaysReportSpans(true).build();
+  }
 }