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 3, 2023
1 parent 1d4f94c commit bf43b43
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 29 deletions.
6 changes: 5 additions & 1 deletion dialogue-clients/metrics.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,11 @@ Dialogue-specific metrics that are not necessarily applicable to other client im
- `dialogue.client.request.queued.time` tagged `channel-name` (timer): Time spent waiting in the queue before execution.
- `dialogue.client.request.endpoint.queued.time` tagged `channel-name`, `service-name`, `endpoint` (timer): Time spent waiting in the queue before execution on a specific endpoint due to server QoS.
- `dialogue.client.request.sticky.queued.time` tagged `channel-name` (timer): Time spent waiting in the sticky queue before execution attempt.
- `dialogue.client.requests.size` tagged `channel-name`, `service-name`, `endpoint`, `retryable` (histogram): Size of requests
- `dialogue.client.requests.size` (histogram): Size of requests
- `repeatable` values (`true`,`false`)
- `channel-name`
- `service-name`
- `endpoint`
- `dialogue.client.create` tagged `client-name`, `client-type` (meter): Marked every time a new client is created.

### dialogue.concurrencylimiter
Expand Down
21 changes: 14 additions & 7 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 @@ -26,21 +26,23 @@
import com.palantir.dialogue.Request;
import com.palantir.dialogue.RequestBody;
import com.palantir.dialogue.Response;
import com.palantir.dialogue.core.DialogueClientMetrics.RequestsSize_Repeatable;
import com.palantir.logsafe.logger.SafeLogger;
import com.palantir.logsafe.logger.SafeLoggerFactory;
import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;
import java.io.IOException;
import java.io.OutputStream;
import java.util.Optional;
import java.util.OptionalLong;
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 Supplier<Histogram> retryableRequestSize;
private final Supplier<Histogram> nonretryableRequestSize;
private final Supplier<Histogram> repeatableRequestSize;
private final Supplier<Histogram> unrepeatableRequestSize;

static EndpointChannel create(Config cf, EndpointChannel channel, Endpoint endpoint) {
ClientConfiguration clientConf = cf.clientConf();
Expand All @@ -51,19 +53,19 @@ 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 = Suppliers.memoize(() -> dialogueClientMetrics
this.repeatableRequestSize = Suppliers.memoize(() -> dialogueClientMetrics
.requestsSize()
.repeatable(RequestsSize_Repeatable.TRUE)
.channelName(channelName)
.serviceName(endpoint.serviceName())
.endpoint(endpoint.endpointName())
.retryable("true")
.build());
this.nonretryableRequestSize = Suppliers.memoize(() -> dialogueClientMetrics
this.unrepeatableRequestSize = Suppliers.memoize(() -> dialogueClientMetrics
.requestsSize()
.repeatable(RequestsSize_Repeatable.FALSE)
.channelName(channelName)
.serviceName(endpoint.serviceName())
.endpoint(endpoint.endpointName())
.retryable("false")
.build());
}

Expand All @@ -80,7 +82,7 @@ private Request wrap(Request request) {
return request;
}
Supplier<Histogram> requestSizeHistogram =
body.get().repeatable() ? this.retryableRequestSize : this.nonretryableRequestSize;
body.get().repeatable() ? this.repeatableRequestSize : this.unrepeatableRequestSize;

return Request.builder()
.from(request)
Expand Down Expand Up @@ -116,6 +118,11 @@ public boolean repeatable() {
return delegate.repeatable();
}

@Override
public OptionalLong contentLength() {
return delegate.contentLength();
}

@Override
public void close() {
delegate.close();
Expand Down
7 changes: 6 additions & 1 deletion dialogue-core/src/main/metrics/dialogue-core-metrics.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,12 @@ namespaces:
docs: Time spent waiting in the sticky queue before execution attempt.
requests.size:
type: histogram
tags: [channel-name, service-name, endpoint, retryable]
tags:
- name: repeatable
values: [ 'true', 'false' ]
- name: channel-name
- name: service-name
- name: endpoint
docs: Size of requests
# Note: the 'dialogue.client.create' metric is also defined in the apache metrics.
create:
Expand Down
36 changes: 16 additions & 20 deletions 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 @@ -19,6 +19,7 @@
import static org.assertj.core.api.Assertions.assertThat;

import com.codahale.metrics.Snapshot;
import com.google.common.io.ByteStreams;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.palantir.conjure.java.client.config.ClientConfiguration;
Expand All @@ -29,30 +30,30 @@
import com.palantir.dialogue.TestConfigurations;
import com.palantir.dialogue.TestEndpoint;
import com.palantir.dialogue.TestResponse;
import com.palantir.dialogue.core.DialogueClientMetrics.RequestsSize_Repeatable;
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.OptionalLong;
import java.util.concurrent.ExecutionException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
public class RequestSizeMetricsChannelTest {
@Mock
DialogueChannelFactory factory;

private static final DialogueChannelFactory STUB_FACTORY = _ignored -> {
throw new AssertionError("DialogueChannelFactory should not be used");
};

@Test
public void records_request_size_metrics() throws ExecutionException, InterruptedException {
TaggedMetricRegistry registry = DefaultTaggedMetricRegistry.getDefault();
TaggedMetricRegistry registry = new DefaultTaggedMetricRegistry();

ByteArrayOutputStream baos = new ByteArrayOutputStream();
int recordableRequestSize = 2 << 20;
byte[] expected = "a".repeat(recordableRequestSize).getBytes(StandardCharsets.UTF_8);
Request request = Request.builder()
Expand Down Expand Up @@ -85,17 +86,15 @@ public void close() {}
EndpointChannel channel = RequestSizeMetricsChannel.create(
ImmutableConfig.builder()
.channelName("channelName")
.channelFactory(factory)
.channelFactory(STUB_FACTORY)
.rawConfig(ClientConfiguration.builder()
.from(TestConfigurations.create("https://foo:10001"))
.taggedMetricRegistry(registry)
.build())
.build(),
r -> {
try {
RequestBody body = r.body().get();
body.writeTo(baos);
body.close();
try (RequestBody body = r.body().get()) {
body.writeTo(ByteStreams.nullOutputStream());
} catch (IOException e) {
throw new RuntimeException(e);
}
Expand All @@ -107,10 +106,10 @@ public void close() {}
assertThat(response.get().code()).isEqualTo(200);
Snapshot snapshot = DialogueClientMetrics.of(registry)
.requestsSize()
.repeatable(RequestsSize_Repeatable.TRUE)
.channelName("channelName")
.serviceName("service")
.endpoint("endpoint")
.retryable("true")
.build()
.getSnapshot();
assertThat(snapshot.size()).isEqualTo(1);
Expand All @@ -119,9 +118,8 @@ public void close() {}

@Test
public void small_request_not_recorded() throws ExecutionException, InterruptedException {
TaggedMetricRegistry registry = DefaultTaggedMetricRegistry.getDefault();
TaggedMetricRegistry registry = new DefaultTaggedMetricRegistry();

ByteArrayOutputStream baos = new ByteArrayOutputStream();
byte[] expected = "test request body".getBytes(StandardCharsets.UTF_8);
Request request = Request.builder()
.body(new RequestBody() {
Expand Down Expand Up @@ -153,17 +151,15 @@ public void close() {}
EndpointChannel channel = RequestSizeMetricsChannel.create(
ImmutableConfig.builder()
.channelName("smallRequestChannelName")
.channelFactory(factory)
.channelFactory(STUB_FACTORY)
.rawConfig(ClientConfiguration.builder()
.from(TestConfigurations.create("https://foo:10001"))
.taggedMetricRegistry(registry)
.build())
.build(),
r -> {
try {
RequestBody body = r.body().get();
body.writeTo(baos);
body.close();
try (RequestBody body = r.body().get()) {
body.writeTo(ByteStreams.nullOutputStream());
} catch (IOException e) {
throw new RuntimeException(e);
}
Expand All @@ -175,10 +171,10 @@ public void close() {}
assertThat(response.get().code()).isEqualTo(200);
MetricName metricName = DialogueClientMetrics.of(registry)
.requestsSize()
.repeatable(RequestsSize_Repeatable.TRUE)
.channelName("smallRequestChannelName")
.serviceName("service")
.endpoint("endpoint")
.retryable("true")
.buildMetricName();
assertThat(registry.remove(metricName)).isEmpty();
}
Expand Down

0 comments on commit bf43b43

Please sign in to comment.