From b43d2830e48be6d6e428698955e37723ae697586 Mon Sep 17 00:00:00 2001 From: Vindhya Ningegowda Date: Wed, 12 Jun 2024 16:57:33 -0700 Subject: [PATCH] opentelemetry: Add explicit histogram buckets for per-call metrics (#11281) Add explicit histogram buckets for per-call metrics as specified in gRFC A66 https://github.com/grpc/proposal/blob/master/A66-otel-stats.md#units. --- .../grpc/opentelemetry/GrpcOpenTelemetry.java | 9 ++ .../internal/OpenTelemetryConstants.java | 15 ++ .../OpenTelemetryMetricsModuleTest.java | 133 +++++++++++++----- 3 files changed, 121 insertions(+), 36 deletions(-) diff --git a/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java b/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java index 68989d80e24..03183ef4920 100644 --- a/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java +++ b/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java @@ -18,6 +18,8 @@ import static com.google.common.base.Preconditions.checkNotNull; import static io.grpc.internal.GrpcUtil.IMPLEMENTATION_VERSION; +import static io.grpc.opentelemetry.internal.OpenTelemetryConstants.LATENCY_BUCKETS; +import static io.grpc.opentelemetry.internal.OpenTelemetryConstants.SIZE_BUCKETS; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Stopwatch; @@ -172,6 +174,7 @@ static OpenTelemetryMetricsResource createMetricInstruments(Meter meter, .setUnit("s") .setDescription( "Time taken by gRPC to complete an RPC from application's perspective") + .setExplicitBucketBoundariesAdvice(LATENCY_BUCKETS) .build()); } @@ -189,6 +192,7 @@ static OpenTelemetryMetricsResource createMetricInstruments(Meter meter, "grpc.client.attempt.duration") .setUnit("s") .setDescription("Time taken to complete a client call attempt") + .setExplicitBucketBoundariesAdvice(LATENCY_BUCKETS) .build()); } @@ -200,6 +204,7 @@ static OpenTelemetryMetricsResource createMetricInstruments(Meter meter, .setUnit("By") .setDescription("Compressed message bytes sent per client call attempt") .ofLongs() + .setExplicitBucketBoundariesAdvice(SIZE_BUCKETS) .build()); } @@ -211,6 +216,7 @@ static OpenTelemetryMetricsResource createMetricInstruments(Meter meter, .setUnit("By") .setDescription("Compressed message bytes received per call attempt") .ofLongs() + .setExplicitBucketBoundariesAdvice(SIZE_BUCKETS) .build()); } @@ -228,6 +234,7 @@ static OpenTelemetryMetricsResource createMetricInstruments(Meter meter, .setUnit("s") .setDescription( "Time taken to complete a call from server transport's perspective") + .setExplicitBucketBoundariesAdvice(LATENCY_BUCKETS) .build()); } @@ -239,6 +246,7 @@ static OpenTelemetryMetricsResource createMetricInstruments(Meter meter, .setUnit("By") .setDescription("Compressed message bytes sent per server call") .ofLongs() + .setExplicitBucketBoundariesAdvice(SIZE_BUCKETS) .build()); } @@ -250,6 +258,7 @@ static OpenTelemetryMetricsResource createMetricInstruments(Meter meter, .setUnit("By") .setDescription("Compressed message bytes received per server call") .ofLongs() + .setExplicitBucketBoundariesAdvice(SIZE_BUCKETS) .build()); } diff --git a/opentelemetry/src/main/java/io/grpc/opentelemetry/internal/OpenTelemetryConstants.java b/opentelemetry/src/main/java/io/grpc/opentelemetry/internal/OpenTelemetryConstants.java index decb9c8d893..081e376b8c5 100644 --- a/opentelemetry/src/main/java/io/grpc/opentelemetry/internal/OpenTelemetryConstants.java +++ b/opentelemetry/src/main/java/io/grpc/opentelemetry/internal/OpenTelemetryConstants.java @@ -16,7 +16,9 @@ package io.grpc.opentelemetry.internal; +import com.google.common.collect.ImmutableList; import io.opentelemetry.api.common.AttributeKey; +import java.util.List; public final class OpenTelemetryConstants { @@ -31,6 +33,19 @@ public final class OpenTelemetryConstants { public static final AttributeKey LOCALITY_KEY = AttributeKey.stringKey("grpc.lb.locality"); + public static final List LATENCY_BUCKETS = + ImmutableList.of( + 0d, 0.00001d, 0.00005d, 0.0001d, 0.0003d, 0.0006d, 0.0008d, 0.001d, 0.002d, + 0.003d, 0.004d, 0.005d, 0.006d, 0.008d, 0.01d, 0.013d, 0.016d, 0.02d, + 0.025d, 0.03d, 0.04d, 0.05d, 0.065d, 0.08d, 0.1d, 0.13d, 0.16d, + 0.2d, 0.25d, 0.3d, 0.4d, 0.5d, 0.65d, 0.8d, 1d, 2d, + 5d, 10d, 20d, 50d, 100d); + + public static final List SIZE_BUCKETS = + ImmutableList.of( + 0L, 1024L, 2048L, 4096L, 16384L, 65536L, 262144L, 1048576L, 4194304L, 16777216L, + 67108864L, 268435456L, 1073741824L, 4294967296L); + private OpenTelemetryConstants() { } } diff --git a/opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java b/opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java index 17a2b26b169..6128323154d 100644 --- a/opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java +++ b/opentelemetry/src/test/java/io/grpc/opentelemetry/OpenTelemetryMetricsModuleTest.java @@ -97,6 +97,15 @@ public class OpenTelemetryMetricsModuleTest { = "grpc.server.call.sent_total_compressed_message_size"; private static final String SERVER_CALL_RECV_TOTAL_COMPRESSED_MESSAGE_SIZE = "grpc.server.call.rcvd_total_compressed_message_size"; + private static final double[] latencyBuckets = + { 0d, 0.00001d, 0.00005d, 0.0001d, 0.0003d, 0.0006d, 0.0008d, 0.001d, 0.002d, + 0.003d, 0.004d, 0.005d, 0.006d, 0.008d, 0.01d, 0.013d, 0.016d, 0.02d, + 0.025d, 0.03d, 0.04d, 0.05d, 0.065d, 0.08d, 0.1d, 0.13d, 0.16d, + 0.2d, 0.25d, 0.3d, 0.4d, 0.5d, 0.65d, 0.8d, 1d, 2d, + 5d, 10d, 20d, 50d, 100d }; + private static final double[] sizeBuckets = + { 0L, 1024L, 2048L, 4096L, 16384L, 65536L, 262144L, 1048576L, 4194304L, 16777216L, + 67108864L, 268435456L, 1073741824L, 4294967296L }; private static final class StringInputStream extends InputStream { final String string; @@ -301,7 +310,11 @@ public void clientBasicMetrics() { point .hasCount(1) .hasSum(0.03 + 0.1 + 0.016 + 0.024) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(latencyBuckets) + .hasBucketCounts(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -316,7 +329,10 @@ public void clientBasicMetrics() { point .hasCount(1) .hasSum(1028L + 99) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets) + .hasBucketCounts(0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -333,7 +349,9 @@ public void clientBasicMetrics() { point .hasCount(1) .hasSum(154) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketCounts(0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -347,7 +365,11 @@ public void clientBasicMetrics() { point .hasCount(1) .hasSum(0.03 + 0.1 + 0.016 + 0.024) - .hasAttributes(clientAttributes)))); + .hasAttributes(clientAttributes) + .hasBucketBoundaries(latencyBuckets) + .hasBucketCounts(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0)))); } // This test is only unit-testing the metrics recording logic. The retry behavior is faked. @@ -428,7 +450,8 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(0.03 + 0.1 + 0.024) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(latencyBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -443,7 +466,8 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(1028L) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -460,7 +484,8 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(0) - .hasAttributes(clientAttributes)))); + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets)))); // faking retry @@ -510,12 +535,14 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(0.1) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(latencyBuckets), point -> point .hasCount(1) .hasSum(0.154) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(latencyBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -532,12 +559,14 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(0) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(sizeBuckets), point -> point .hasCount(1) .hasSum(0) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -552,12 +581,14 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(1028L) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(sizeBuckets), point -> point .hasCount(1) .hasSum(1028L) - .hasAttributes(clientAttributes)))); + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets)))); // fake transparent retry fakeClock.forwardTime(10, TimeUnit.MILLISECONDS); @@ -597,12 +628,14 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(0.1) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(latencyBuckets), point -> point .hasCount(2) .hasSum(0.154 + 0.032) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(latencyBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -619,12 +652,14 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(0) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(sizeBuckets), point -> point .hasCount(2) .hasSum(0 + 0) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -639,12 +674,14 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(1028L) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(sizeBuckets), point -> point .hasCount(2) .hasSum(1028L + 0) - .hasAttributes(clientAttributes)))); + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets)))); // fake another transparent retry fakeClock.forwardTime(10, MILLISECONDS); @@ -697,17 +734,20 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(1028L) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(sizeBuckets), point -> point .hasCount(2) .hasSum(1028L + 0) - .hasAttributes(clientAttributes), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets), point -> point .hasCount(1) .hasSum(1028L) - .hasAttributes(clientAttributes2))), + .hasAttributes(clientAttributes2) + .hasBucketBoundaries(sizeBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -722,7 +762,8 @@ public void recordAttemptMetrics() { .hasCount(1) .hasSum(0.03 + 0.1 + 0.024 + 1 + 0.1 + 0.01 + 0.032 + 0.01 + 0.024) - .hasAttributes(clientAttributes2))), + .hasAttributes(clientAttributes2) + .hasBucketBoundaries(latencyBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -736,17 +777,20 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(0.100) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(latencyBuckets), point -> point .hasCount(2) .hasSum(0.154 + 0.032) - .hasAttributes(clientAttributes), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(latencyBuckets), point -> point .hasCount(1) .hasSum(0.024) - .hasAttributes(clientAttributes2))), + .hasAttributes(clientAttributes2) + .hasBucketBoundaries(latencyBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -763,17 +807,20 @@ public void recordAttemptMetrics() { point .hasCount(1) .hasSum(0) - .hasAttributes(clientAttributes1), + .hasAttributes(clientAttributes1) + .hasBucketBoundaries(sizeBuckets), point -> point .hasCount(2) .hasSum(0 + 0) - .hasAttributes(clientAttributes), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets), point -> point .hasCount(1) .hasSum(33D) - .hasAttributes(clientAttributes2)))); + .hasAttributes(clientAttributes2) + .hasBucketBoundaries(sizeBuckets)))); } @Test @@ -831,7 +878,8 @@ public void clientStreamNeverCreatedStillRecordMetrics() { point .hasCount(1) .hasSum(0) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -845,7 +893,8 @@ public void clientStreamNeverCreatedStillRecordMetrics() { point .hasCount(1) .hasSum(3D) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(latencyBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -859,7 +908,8 @@ public void clientStreamNeverCreatedStillRecordMetrics() { point .hasCount(1) .hasSum(0) - .hasAttributes(clientAttributes))), + .hasAttributes(clientAttributes) + .hasBucketBoundaries(latencyBuckets))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -876,7 +926,8 @@ public void clientStreamNeverCreatedStillRecordMetrics() { point .hasCount(1) .hasSum(0) - .hasAttributes(clientAttributes)))); + .hasAttributes(clientAttributes) + .hasBucketBoundaries(sizeBuckets)))); } @@ -1077,7 +1128,10 @@ public void serverBasicMetrics() { point .hasCount(1) .hasSum(1028L + 99) - .hasAttributes(serverAttributes))), + .hasAttributes(serverAttributes) + .hasBucketBoundaries(sizeBuckets) + .hasBucketCounts(0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -1105,7 +1159,11 @@ public void serverBasicMetrics() { point .hasCount(1) .hasSum(0.1 + 0.016 + 0.024) - .hasAttributes(serverAttributes))), + .hasAttributes(serverAttributes) + .hasBucketBoundaries(latencyBuckets) + .hasBucketCounts(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0))), metric -> assertThat(metric) .hasInstrumentationScope(InstrumentationScopeInfo.create( @@ -1122,7 +1180,10 @@ public void serverBasicMetrics() { point .hasCount(1) .hasSum(34L + 154) - .hasAttributes(serverAttributes)))); + .hasAttributes(serverAttributes) + .hasBucketBoundaries(sizeBuckets) + .hasBucketCounts(0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0)))); }