From bbdfe45fb8f3905e93c6ccfe8deaec50112707ff Mon Sep 17 00:00:00 2001
From: Adrian Cole <adriancole@users.noreply.github.com>
Date: Sat, 10 Aug 2019 12:38:29 +1200
Subject: [PATCH] Adds hack instructions for those in a pinch (#73)

It is important for edge cases to stay edge cases. If someone is
invoking a builder, it is safe to assume they can manage the lifecycle
of what they are building. In the odd case they can't, it is better to
provide instructions vs absorb tech debt here.

This shows how to hack around lifecycle. It also fixes a subtle ordering
bug.
---
 .../java/zipkin2/finagle/ZipkinTracer.java    | 54 +++++++++++++++----
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/core/src/main/java/zipkin2/finagle/ZipkinTracer.java b/core/src/main/java/zipkin2/finagle/ZipkinTracer.java
index b6168b2..8fe9258 100644
--- a/core/src/main/java/zipkin2/finagle/ZipkinTracer.java
+++ b/core/src/main/java/zipkin2/finagle/ZipkinTracer.java
@@ -29,6 +29,7 @@
 import java.io.IOException;
 import scala.Option;
 import scala.Some;
+import scala.runtime.AbstractFunction1;
 import scala.runtime.BoxedUnit;
 import zipkin2.Span;
 import zipkin2.internal.Nullable;
@@ -90,8 +91,28 @@ public class ZipkinTracer extends SamplingTracer implements Closable {
    * tracer = ZipkinTracer.newBuilder(spanReporter).stats(zipkinStats).build();
    * }</pre>
    *
-   * <p><em>Note</em>: You must close the supplied sender externally, after this instance is
-   * closed.
+   * <h3>On closing resources</h3>
+   * The resulting tracer will attempt to close an underlying reporter if it implements {@link
+   * Closeable}. It is best to use normal tools like pre-destroy hooks to close resources in your
+   * application. If you somehow cannot control your resources, yet can invoke this, consider
+   * wrapping the input as a closeable to coordinate an ordered shutdown.
+   *
+   * <p>Ex.
+   * <pre>{@code
+   * class ReporterThatClosesSender implements Reporter<Span>, Closeable {
+   *   final Sender sender;
+   *   final AsyncReporter<Span> reporter;
+   *
+   *   @Override public void close() throws IOException {
+   *     reporter.close();
+   *     sender.close();
+   *   }
+   *
+   *   @Override public void report(Span span) {
+   *     reporter.report(span);
+   *   }
+   * }
+   * }</pre>
    */
   public static Builder newBuilder(Reporter<Span> spanReporter) {
     return new Builder(spanReporter);
@@ -120,14 +141,26 @@ private ZipkinTracer(Reporter<Span> reporter, RawZipkinTracer underlying, Config
     return close(Time.Bottom());
   }
 
+  /**
+   * There are two queues here. The recorder has in-flight data about operations not yet complete.
+   * The reporter (usually) has a queue of spans for operations completed, not yet sent to Zipkin.
+   *
+   * <p>The close process tries to avoid dropping data on the floor by first flushing any in-flight
+   * operations in the recorder (ideally none), then any spans waiting for the next send interval to
+   * Zipkin.
+   */
   @Override public Future<BoxedUnit> close(Time deadline) {
-    if (reporter instanceof Closeable) {
-      try {
-        ((Closeable) reporter).close();
-      } catch (IOException | RuntimeException ignored) {
+    Future<BoxedUnit> result = underlying.recorder.close(deadline);
+    if (!(reporter instanceof Closeable)) return result;
+    return result.onSuccess(new AbstractFunction1<BoxedUnit, BoxedUnit>() {
+      @Override public BoxedUnit apply(BoxedUnit v1) {
+        try {
+          ((Closeable) reporter).close();
+        } catch (IOException | RuntimeException ignored) {
+        }
+        return BoxedUnit.UNIT;
       }
-    }
-    return underlying.recorder.close(deadline);
+    });
   }
 
   @Override public Future<BoxedUnit> close(Duration after) {
@@ -136,6 +169,7 @@ private ZipkinTracer(Reporter<Span> reporter, RawZipkinTracer underlying, Config
 
   protected interface Config {
     @Nullable String localServiceName();
+
     /** How much data to collect. Default sample rate 0.001 (0.1%). Max is 1, min 0. */
     float initialSampleRate();
   }
@@ -183,8 +217,8 @@ public static final class Builder {
     }
 
     /**
-     * Lower-case label of the remote node in the service graph, such as "favstar". Avoid names
-     * with variables or unique identifiers embedded.
+     * Lower-case label of the remote node in the service graph, such as "favstar". Avoid names with
+     * variables or unique identifiers embedded.
      *
      * <p>When unset, the service name is derived from {@link Annotation.ServiceName} which is
      * often incorrectly set to the remote service name.