From fcc67bb6c3e65f737e994a5480d93edfcb57107e Mon Sep 17 00:00:00 2001 From: Adrian Cole <64215+codefromthecrypt@users.noreply.github.com> Date: Fri, 12 Apr 2024 10:33:58 -1000 Subject: [PATCH] Deprecates AsyncReporter/SpanHandler queuedMaxBytes (#264) Detailed rationale in RATIONALE.md Closes #204 Closes https://github.com/openzipkin/brave/issues/1426 Signed-off-by: Adrian Cole --- RATIONALE.md | 24 +++++++++++++++++++ README.md | 13 +++++----- activemq-client/pom.xml | 2 +- amqp-client/pom.xml | 2 +- benchmarks/pom.xml | 2 +- bom/pom.xml | 2 +- brave/pom.xml | 2 +- .../brave/AsyncZipkinSpanHandler.java | 8 ++++--- core/pom.xml | 2 +- .../java/zipkin2/reporter/AsyncReporter.java | 7 +++++- .../reporter/internal/AsyncReporter.java | 15 ++++++------ .../reporter/internal/ByteBoundedQueue.java | 3 +++ .../internal/ByteBoundedQueueTest.java | 2 +- .../internal/CountBoundedQueueTest.java | 2 +- kafka/pom.xml | 2 +- libthrift/pom.xml | 2 +- metrics-micrometer/pom.xml | 2 +- okhttp3/pom.xml | 2 +- pom.xml | 2 +- spring-beans/pom.xml | 2 +- urlconnection/pom.xml | 2 +- 21 files changed, 67 insertions(+), 33 deletions(-) diff --git a/RATIONALE.md b/RATIONALE.md index 7e7bff54..c2a8bcc0 100644 --- 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 index 64c21252..d5d7e7ea 100644 --- 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 index d1b820b1..1a351411 100644 --- a/activemq-client/pom.xml +++ b/activemq-client/pom.xml @@ -9,7 +9,7 @@ zipkin-reporter-parent io.zipkin.reporter2 - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT 4.0.0 diff --git a/amqp-client/pom.xml b/amqp-client/pom.xml index 9136e7b6..7af75881 100644 --- a/amqp-client/pom.xml +++ b/amqp-client/pom.xml @@ -11,7 +11,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT zipkin-sender-amqp-client diff --git a/benchmarks/pom.xml b/benchmarks/pom.xml index 926e3c25..a0f8fd45 100644 --- a/benchmarks/pom.xml +++ b/benchmarks/pom.xml @@ -13,7 +13,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT benchmarks diff --git a/bom/pom.xml b/bom/pom.xml index a1713cd0..fef6fe96 100644 --- a/bom/pom.xml +++ b/bom/pom.xml @@ -20,7 +20,7 @@ io.zipkin.reporter2 zipkin-reporter-bom Zipkin Reporter BOM - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT pom Bill Of Materials POM for all Zipkin reporter artifacts diff --git a/brave/pom.xml b/brave/pom.xml index b4ef3203..99c8ec76 100644 --- a/brave/pom.xml +++ b/brave/pom.xml @@ -9,7 +9,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT 4.0.0 diff --git a/brave/src/main/java/zipkin2/reporter/brave/AsyncZipkinSpanHandler.java b/brave/src/main/java/zipkin2/reporter/brave/AsyncZipkinSpanHandler.java index b82e97db..b917840a 100644 --- 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 index 5d21e1c3..3a3999e7 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -11,7 +11,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT zipkin-reporter diff --git a/core/src/main/java/zipkin2/reporter/AsyncReporter.java b/core/src/main/java/zipkin2/reporter/AsyncReporter.java index f620eb1a..8107e9bb 100644 --- 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 index 4ebbc17d..11c7f0a9 100644 --- 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 index 9e875750..7f036c3f 100644 --- 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. * *

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 extends BoundedQueue { final BytesEncoder 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 index 8cefef06..133d25b7 100644 --- 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 index 36e15d2d..0cc96f50 100644 --- 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 index 3c693976..d92b5429 100644 --- a/kafka/pom.xml +++ b/kafka/pom.xml @@ -11,7 +11,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT zipkin-sender-kafka diff --git a/libthrift/pom.xml b/libthrift/pom.xml index eb9c9969..5dfe1823 100644 --- a/libthrift/pom.xml +++ b/libthrift/pom.xml @@ -11,7 +11,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT zipkin-sender-libthrift diff --git a/metrics-micrometer/pom.xml b/metrics-micrometer/pom.xml index 34b0d9cf..8b3aecd0 100644 --- a/metrics-micrometer/pom.xml +++ b/metrics-micrometer/pom.xml @@ -11,7 +11,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT zipkin-reporter-metrics-micrometer diff --git a/okhttp3/pom.xml b/okhttp3/pom.xml index 1d531e21..d80009f3 100644 --- a/okhttp3/pom.xml +++ b/okhttp3/pom.xml @@ -11,7 +11,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT zipkin-sender-okhttp3 diff --git a/pom.xml b/pom.xml index 9d050a5d..6a35414c 100755 --- a/pom.xml +++ b/pom.xml @@ -10,7 +10,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT pom diff --git a/spring-beans/pom.xml b/spring-beans/pom.xml index df14ee2c..5b2e4982 100644 --- a/spring-beans/pom.xml +++ b/spring-beans/pom.xml @@ -9,7 +9,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT 4.0.0 diff --git a/urlconnection/pom.xml b/urlconnection/pom.xml index 8bb5f322..d7916237 100644 --- a/urlconnection/pom.xml +++ b/urlconnection/pom.xml @@ -11,7 +11,7 @@ io.zipkin.reporter2 zipkin-reporter-parent - 3.3.1-SNAPSHOT + 3.4.0-SNAPSHOT zipkin-sender-urlconnection