From 880b94cb0b2c191745a95289471564c12866c32e Mon Sep 17 00:00:00 2001 From: Andy Bradshaw Date: Mon, 2 Oct 2023 10:50:50 -0400 Subject: [PATCH] PR feedback --- .../dialogue/core/DialogueChannel.java | 6 +- .../core/RequestSizeMetricsChannel.java | 26 ++++--- .../core/RequestSizeMetricsChannelTest.java | 74 ++++++++++++++++++- 3 files changed, 94 insertions(+), 12 deletions(-) diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java index fe06676d2..e39d2a8dd 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java @@ -92,6 +92,7 @@ public Builder clientConfiguration(ClientConfiguration value) { /** * Please use {@link #factory(DialogueChannelFactory)}. + * * @deprecated prefer {@link #factory(DialogueChannelFactory)} */ @Deprecated @@ -185,6 +186,7 @@ public DialogueChannel build() { channel = new RangeAcceptsIdentityEncodingChannel(channel); channel = ContentEncodingChannel.of(channel, endpoint); channel = TracedChannel.create(cf, channel, endpoint); + channel = RequestSizeMetricsChannel.create(cf, channel, endpoint); if (ChannelToEndpointChannel.isConstant(endpoint)) { // Avoid producing metrics for non-constant endpoints which may produce // high cardinality. @@ -207,7 +209,9 @@ public DialogueChannel build() { return new DialogueChannel(cf, channelFactory, stickyChannelSupplier); } - /** Does *not* do any clever live-reloading. */ + /** + * Does *not* do any clever live-reloading. + */ @CheckReturnValue public Channel buildNonLiveReloading() { return build(); diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/RequestSizeMetricsChannel.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/RequestSizeMetricsChannel.java index b7c1a0c58..c4115f582 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/RequestSizeMetricsChannel.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/RequestSizeMetricsChannel.java @@ -17,6 +17,7 @@ package com.palantir.dialogue.core; import com.codahale.metrics.Histogram; +import com.google.common.base.Suppliers; import com.google.common.io.CountingOutputStream; import com.google.common.util.concurrent.ListenableFuture; import com.palantir.conjure.java.client.config.ClientConfiguration; @@ -31,12 +32,15 @@ import java.io.IOException; import java.io.OutputStream; import java.util.Optional; +import java.util.function.Supplier; final class RequestSizeMetricsChannel implements EndpointChannel { private static final SafeLogger log = SafeLoggerFactory.get(RequestSizeMetricsChannel.class); + // MIN_REPORTED_REQUEST_SIZE filters recording small requests to reduce metric cardinality + private static final long MIN_REPORTED_REQUEST_SIZE = 1 << 20; private final EndpointChannel delegate; - private final Histogram retryableRequestSize; - private final Histogram nonretryableRequestSize; + private final Supplier retryableRequestSize; + private final Supplier nonretryableRequestSize; static EndpointChannel create(Config cf, EndpointChannel channel, Endpoint endpoint) { ClientConfiguration clientConf = cf.clientConf(); @@ -47,20 +51,20 @@ static EndpointChannel create(Config cf, EndpointChannel channel, Endpoint endpo EndpointChannel delegate, String channelName, Endpoint endpoint, TaggedMetricRegistry registry) { this.delegate = delegate; DialogueClientMetrics dialogueClientMetrics = DialogueClientMetrics.of(registry); - this.retryableRequestSize = dialogueClientMetrics + this.retryableRequestSize = Suppliers.memoize(() -> dialogueClientMetrics .requestsSize() .channelName(channelName) .serviceName(endpoint.serviceName()) .endpoint(endpoint.endpointName()) .retryable("true") - .build(); - this.nonretryableRequestSize = dialogueClientMetrics + .build()); + this.nonretryableRequestSize = Suppliers.memoize(() -> dialogueClientMetrics .requestsSize() .channelName(channelName) .serviceName(endpoint.serviceName()) .endpoint(endpoint.endpointName()) .retryable("false") - .build(); + .build()); } @Override @@ -75,7 +79,7 @@ private Request wrap(Request request) { // No need to record empty bodies return request; } - Histogram requestSizeHistogram = + Supplier requestSizeHistogram = body.get().repeatable() ? this.retryableRequestSize : this.nonretryableRequestSize; return Request.builder() @@ -86,9 +90,9 @@ private Request wrap(Request request) { private static class RequestSizeRecordingRequestBody implements RequestBody { private final RequestBody delegate; - private final Histogram size; + private final Supplier size; - RequestSizeRecordingRequestBody(RequestBody requestBody, Histogram size) { + RequestSizeRecordingRequestBody(RequestBody requestBody, Supplier size) { this.delegate = requestBody; this.size = size; } @@ -97,7 +101,9 @@ private static class RequestSizeRecordingRequestBody implements RequestBody { public void writeTo(OutputStream output) throws IOException { CountingOutputStream countingOut = new CountingOutputStream(output); delegate.writeTo(countingOut); - size.update(countingOut.getCount()); + if (countingOut.getCount() > MIN_REPORTED_REQUEST_SIZE) { + size.get().update(countingOut.getCount()); + } } @Override diff --git a/dialogue-core/src/test/java/com/palantir/dialogue/core/RequestSizeMetricsChannelTest.java b/dialogue-core/src/test/java/com/palantir/dialogue/core/RequestSizeMetricsChannelTest.java index ff03e20d9..71e0fb4c8 100644 --- a/dialogue-core/src/test/java/com/palantir/dialogue/core/RequestSizeMetricsChannelTest.java +++ b/dialogue-core/src/test/java/com/palantir/dialogue/core/RequestSizeMetricsChannelTest.java @@ -30,11 +30,13 @@ import com.palantir.dialogue.TestEndpoint; import com.palantir.dialogue.TestResponse; import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry; +import com.palantir.tritium.metrics.registry.MetricName; import com.palantir.tritium.metrics.registry.TaggedMetricRegistry; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.OutputStream; import java.nio.charset.StandardCharsets; +import java.util.Objects; import java.util.OptionalLong; import java.util.concurrent.ExecutionException; import org.junit.jupiter.api.Test; @@ -44,6 +46,10 @@ @ExtendWith(MockitoExtension.class) public class RequestSizeMetricsChannelTest { + private static final String JAVA_VERSION = System.getProperty("java.version", "unknown"); + private static final String LIBRARY_VERSION = + Objects.requireNonNullElse(DialogueClientMetrics.class.getPackage().getImplementationVersion(), "unknown"); + @Mock DialogueChannelFactory factory; @@ -52,7 +58,8 @@ public void records_request_size_metrics() throws ExecutionException, Interrupte TaggedMetricRegistry registry = DefaultTaggedMetricRegistry.getDefault(); ByteArrayOutputStream baos = new ByteArrayOutputStream(); - byte[] expected = "test request body".getBytes(StandardCharsets.UTF_8); + int recordableRequestSize = 2 << 20; + byte[] expected = "a".repeat(recordableRequestSize).getBytes(StandardCharsets.UTF_8); Request request = Request.builder() .body(new RequestBody() { @Override @@ -111,6 +118,71 @@ public void close() {} assertThat(snapshot.get99thPercentile()).isEqualTo(expected.length); } + @Test + public void small_request_not_recorded() throws ExecutionException, InterruptedException { + TaggedMetricRegistry registry = DefaultTaggedMetricRegistry.getDefault(); + + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + byte[] expected = "test request body".getBytes(StandardCharsets.UTF_8); + Request request = Request.builder() + .body(new RequestBody() { + @Override + public void writeTo(OutputStream output) throws IOException { + output.write(expected); + } + + @Override + public String contentType() { + return "text/plain"; + } + + @Override + public boolean repeatable() { + return true; + } + + @Override + public OptionalLong contentLength() { + return OptionalLong.of(expected.length); + } + + @Override + public void close() {} + }) + .build(); + + EndpointChannel channel = RequestSizeMetricsChannel.create( + config(ClientConfiguration.builder() + .from(TestConfigurations.create("https://foo:10001")) + .taggedMetricRegistry(registry) + .build()), + r -> { + try { + RequestBody body = r.body().get(); + body.writeTo(baos); + body.close(); + } catch (IOException e) { + throw new RuntimeException(e); + } + return Futures.immediateFuture(new TestResponse().code(200)); + }, + TestEndpoint.GET); + ListenableFuture response = channel.execute(request); + + assertThat(response.get().code()).isEqualTo(200); + MetricName metricName = MetricName.builder() + .safeName("dialogue.client.requests.size") + .putSafeTags("channel-name", "channelName") + .putSafeTags("service-name", "service") + .putSafeTags("endpoint", "endpoint") + .putSafeTags("retryable", "true") + .putSafeTags("libraryName", "dialogue") + .putSafeTags("libraryVersion", LIBRARY_VERSION) + .putSafeTags("javaVersion", JAVA_VERSION) + .build(); + assertThat(registry.remove(metricName)).isEmpty(); + } + private ImmutableConfig config(ClientConfiguration rawConfig) { return ImmutableConfig.builder() .channelName("channelName")