Deprecates AsyncReporter/SpanHandler queuedMaxBytes (#264)

Detailed rationale in RATIONALE.md Closes #204 Closes openzipkin/brave#1426 Signed-off-by: Adrian Cole <adrian@tetrate.io>
openzipkin · Apr 12, 2024 · fcc67bb · fcc67bb
1 parent e54db40
commit fcc67bb
Show file tree

Hide file tree

Showing 21 changed files with 67 additions and 33 deletions.
diff --git a/RATIONALE.md b/RATIONALE.md
@@ -1,5 +1,29 @@
 # zipkin-reporter rationale
 
+## AsyncReporter
+
+### Why is `queuedMaxBytes` (`ByteBoundedQueue`) deprecated?
+
+When introduced, `AsyncReporter` had three ways to trigger a queue flush:
+* `queuedMaxSpans` - when the number of spans in the queue exceeds a threshold
+* `queuedMaxBytes` - when the size of the spans in the queue exceeds a threshold
+* `messageTimeout` - when a span has been in the queue longer than a threshold
+
+`queuedMaxBytes` was deprecated because requires time in the critical path, to
+calculate the size of a span to make sure it doesn't breach the threshold.
+This is problematic in tools that check for blocking, like Virtual Threads.
+https://github.com/openzipkin/brave/issues/1426
+
+Also, the `queuedMaxBytes` is not representative of the total heap used by the
+queue. Moreover, any discussion of heap is confusing per this issue:
+https://github.com/openzipkin/zipkin-reporter-java/issues/204
+
+Meanwhile, for a lot of sites, the other two gates would hit prior to 1pct
+memory anyway.
+
+By deprecating this (in v3.4), we eliminate confusion on this topic and allow
+version 4 a chance to clean up some complicated code.
+
 ## HttpEndpointSupplier
 
 `HttpEndpointSupplier` was generalizes endpoint resolution for built-in and third-party senders.

diff --git a/README.md b/README.md
@@ -64,12 +64,13 @@ backlog as a function of bytes.
 
 Here are the most important properties to understand when tuning.
 
-Property | Description
---- | ---
-`queuedMaxBytes` |  Maximum backlog of span bytes reported vs sent. Corresponds to `ReporterMetrics.updateQueuedBytes`. Default 1% of heap
-`messageMaxBytes` | Maximum bytes sendable per message including overhead. Default `500,000` bytes (`500KB`). Defined by `BytesMessageSender.messageMaxBytes`
-`messageTimeout` |  Maximum time to wait for messageMaxBytes to accumulate before sending. Default 1 second
-`closeTimeout` |  Maximum time to block for in-flight spans to send on close. Default 1 second
+| Property          | Description                                                                                                                               |
+|-------------------|-------------------------------------------------------------------------------------------------------------------------------------------|
+| `queuedMaxSpans`  | Maximum backlog of spans reported vs sent. Default 10000                                                                                  |
+| `queuedMaxBytes`  | Maximum backlog of span bytes reported vs sent. Corresponds to `ReporterMetrics.updateQueuedBytes`. Disabled by default                   |
+| `messageMaxBytes` | Maximum bytes sendable per message including overhead. Default `500,000` bytes (`500KB`). Defined by `BytesMessageSender.messageMaxBytes` |
+| `messageTimeout`  | Maximum time to wait for messageMaxBytes to accumulate before sending. Default 1 second                                                   |
+| `closeTimeout`    | Maximum time to block for in-flight spans to send on close. Default 1 second                                                              |
 
 #### Dealing with span backlog
 When `messageTimeout` is non-zero, a single thread is responsible for

diff --git a/activemq-client/pom.xml b/activemq-client/pom.xml
@@ -9,7 +9,7 @@
   <parent>
     <artifactId>zipkin-reporter-parent</artifactId>
     <groupId>io.zipkin.reporter2</groupId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
   <modelVersion>4.0.0</modelVersion>
 

diff --git a/amqp-client/pom.xml b/amqp-client/pom.xml
@@ -11,7 +11,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>zipkin-sender-amqp-client</artifactId>

diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml
@@ -13,7 +13,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>benchmarks</artifactId>

diff --git a/bom/pom.xml b/bom/pom.xml
@@ -20,7 +20,7 @@
   <groupId>io.zipkin.reporter2</groupId>
   <artifactId>zipkin-reporter-bom</artifactId>
   <name>Zipkin Reporter BOM</name>
-  <version>3.3.1-SNAPSHOT</version>
+  <version>3.4.0-SNAPSHOT</version>
   <packaging>pom</packaging>
   <description>Bill Of Materials POM for all Zipkin reporter artifacts</description>
 

diff --git a/brave/pom.xml b/brave/pom.xml
@@ -9,7 +9,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
   <modelVersion>4.0.0</modelVersion>
 

diff --git a/brave/src/main/java/zipkin2/reporter/brave/AsyncZipkinSpanHandler.java b/brave/src/main/java/zipkin2/reporter/brave/AsyncZipkinSpanHandler.java
@@ -142,11 +142,13 @@ public Builder queuedMaxSpans(int queuedMaxSpans) {
     }
 
     /**
-     * @see AsyncReporter.Builder#queuedMaxBytes(int)
-     * @since 2.14
+     * Maximum backlog of span bytes reported vs sent. Disabled by default
+     *
+     * @deprecated This will be removed in version 4.0. Use {@link #queuedMaxSpans(int)} instead.
      */
+    @Deprecated
     public Builder queuedMaxBytes(int queuedMaxBytes) {
-      delegate.queuedMaxBytes(queuedMaxBytes);
+      this.delegate.queuedMaxBytes(queuedMaxBytes);
       return this;
     }
 

diff --git a/core/pom.xml b/core/pom.xml
@@ -11,7 +11,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>zipkin-reporter</artifactId>

diff --git a/core/src/main/java/zipkin2/reporter/AsyncReporter.java b/core/src/main/java/zipkin2/reporter/AsyncReporter.java
@@ -146,7 +146,12 @@ public Builder queuedMaxSpans(int queuedMaxSpans) {
       return this;
     }
 
-    /** Maximum backlog of span bytes reported vs sent. Default 1% of heap */
+    /**
+     * Maximum backlog of span bytes reported vs sent. Disabled by default
+     *
+     * @deprecated This will be removed in version 4.0. Use {@link #queuedMaxSpans(int)} instead.
+     */
+    @Deprecated
     public Builder queuedMaxBytes(int queuedMaxBytes) {
       this.delegate.queuedMaxBytes(queuedMaxBytes);
       return this;

diff --git a/core/src/main/java/zipkin2/reporter/internal/AsyncReporter.java b/core/src/main/java/zipkin2/reporter/internal/AsyncReporter.java
@@ -74,7 +74,7 @@ public static final class Builder {
     long messageTimeoutNanos = TimeUnit.SECONDS.toNanos(1);
     long closeTimeoutNanos = TimeUnit.SECONDS.toNanos(1);
     int queuedMaxSpans = 10000;
-    int queuedMaxBytes = onePercentOfMemory();
+    int queuedMaxBytes = 0; // disabled by default
 
     Builder(BoundedAsyncReporter<?> asyncReporter) {
       this.sender = asyncReporter.sender;
@@ -87,12 +87,6 @@ public static final class Builder {
       this.queuedMaxBytes = asyncReporter.queuedMaxBytes;
     }
 
-    static int onePercentOfMemory() {
-      long result = (long) (Runtime.getRuntime().totalMemory() * 0.01);
-      // don't overflow in the rare case 1% of memory is larger than 2 GiB!
-      return (int) Math.max(Math.min(Integer.MAX_VALUE, result), Integer.MIN_VALUE);
-    }
-
     Builder(BytesMessageSender sender) {
       if (sender == null) throw new NullPointerException("sender == null");
       this.sender = sender;
@@ -159,7 +153,12 @@ public Builder queuedMaxSpans(int queuedMaxSpans) {
       return this;
     }
 
-    /** Maximum backlog of span bytes reported vs sent. Default 1% of heap */
+    /**
+     * Maximum backlog of span bytes reported vs sent. Disabled by default
+     *
+     * @deprecated This will be removed in version 4.0. Use {@link #queuedMaxSpans(int)} instead.
+     */
+    @Deprecated
     public Builder queuedMaxBytes(int queuedMaxBytes) {
       this.queuedMaxBytes = queuedMaxBytes;
       return this;

diff --git a/core/src/main/java/zipkin2/reporter/internal/ByteBoundedQueue.java b/core/src/main/java/zipkin2/reporter/internal/ByteBoundedQueue.java
@@ -15,7 +15,10 @@
  * Multi-producer, multi-consumer queue that is bounded by both count and size.
  *
  * <p>This is similar to {@link java.util.concurrent.ArrayBlockingQueue} in implementation.
+ *
+ * @deprecated This will be removed in version 4.0. Use {@link CountBoundedQueue} instead.
  */
+@Deprecated
 final class ByteBoundedQueue<S> extends BoundedQueue<S> {
   final BytesEncoder<S> encoder;
   final BytesMessageSender sender;

diff --git a/core/src/test/java/zipkin2/reporter/internal/ByteBoundedQueueTest.java b/core/src/test/java/zipkin2/reporter/internal/ByteBoundedQueueTest.java
@@ -53,7 +53,7 @@ class ByteBoundedQueueTest {
       queue.drainTo(consumer, 1);
     }
 
-    // ensure we have all of the spans
+    // ensure we have all the spans
     assertThat(polled)
       .containsExactly(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14);
   }

diff --git a/core/src/test/java/zipkin2/reporter/internal/CountBoundedQueueTest.java b/core/src/test/java/zipkin2/reporter/internal/CountBoundedQueueTest.java
@@ -57,7 +57,7 @@ class CountBoundedQueueTest {
       queue.drainTo(consumer, 1);
     }
 
-    // ensure we have all of the spans
+    // ensure we have all the spans
     assertThat(polled)
       .containsExactly(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14);
   }

diff --git a/kafka/pom.xml b/kafka/pom.xml
@@ -11,7 +11,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>zipkin-sender-kafka</artifactId>

diff --git a/libthrift/pom.xml b/libthrift/pom.xml
@@ -11,7 +11,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>zipkin-sender-libthrift</artifactId>

diff --git a/metrics-micrometer/pom.xml b/metrics-micrometer/pom.xml
@@ -11,7 +11,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>zipkin-reporter-metrics-micrometer</artifactId>

diff --git a/okhttp3/pom.xml b/okhttp3/pom.xml
@@ -11,7 +11,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>zipkin-sender-okhttp3</artifactId>

diff --git a/pom.xml b/pom.xml
@@ -10,7 +10,7 @@
 
   <groupId>io.zipkin.reporter2</groupId>
   <artifactId>zipkin-reporter-parent</artifactId>
-  <version>3.3.1-SNAPSHOT</version>
+  <version>3.4.0-SNAPSHOT</version>
   <packaging>pom</packaging>
 
   <modules>

diff --git a/spring-beans/pom.xml b/spring-beans/pom.xml
@@ -9,7 +9,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
   <modelVersion>4.0.0</modelVersion>
 

diff --git a/urlconnection/pom.xml b/urlconnection/pom.xml
@@ -11,7 +11,7 @@
   <parent>
     <groupId>io.zipkin.reporter2</groupId>
     <artifactId>zipkin-reporter-parent</artifactId>
-    <version>3.3.1-SNAPSHOT</version>
+    <version>3.4.0-SNAPSHOT</version>
   </parent>
 
   <artifactId>zipkin-sender-urlconnection</artifactId>