Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@andybradshaw
andybradshaw committed Oct 2, 2023
1 parent 638a1a2 commit 880b94c
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 12 deletions.
6 changes: 5 additions & 1 deletion dialogue-core/src/main/java/com/palantir/dialogue/core/DialogueChannel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ public Builder clientConfiguration(ClientConfiguration value) {

/**
* Please use {@link #factory(DialogueChannelFactory)}.
*
* @deprecated prefer {@link #factory(DialogueChannelFactory)}
*/
@Deprecated
Expand Down Expand Up @@ -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.
Expand All @@ -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();
Expand Down
26 changes: 16 additions & 10 deletions dialogue-core/src/main/java/com/palantir/dialogue/core/RequestSizeMetricsChannel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<Histogram> retryableRequestSize;
private final Supplier<Histogram> nonretryableRequestSize;

static EndpointChannel create(Config cf, EndpointChannel channel, Endpoint endpoint) {
ClientConfiguration clientConf = cf.clientConf();
Expand All @@ -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
Expand All @@ -75,7 +79,7 @@ private Request wrap(Request request) {
// No need to record empty bodies
return request;
}
Histogram requestSizeHistogram =
Supplier<Histogram> requestSizeHistogram =
body.get().repeatable() ? this.retryableRequestSize : this.nonretryableRequestSize;

return Request.builder()
Expand All @@ -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<Histogram> size;

RequestSizeRecordingRequestBody(RequestBody requestBody, Histogram size) {
RequestSizeRecordingRequestBody(RequestBody requestBody, Supplier<Histogram> size) {
this.delegate = requestBody;
this.size = size;
}
Expand All @@ -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
Expand Down
74 changes: 73 additions & 1 deletion dialogue-core/src/test/java/com/palantir/dialogue/core/RequestSizeMetricsChannelTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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
Expand Down Expand Up @@ -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> 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")
Expand Down

0 comments on commit 880b94c

Please sign in to comment.