Consistently include status keyvalue even if unknown

Without this, the keyvalues present on the observation would depend on the status being available or not. This creates issues for the Prometheus registry. Resolves gh-5609
micrometer-metrics · Nov 29, 2024 · 53a7c52 · 53a7c52
1 parent a90bdd9
commit 53a7c52
Show file tree

Hide file tree

Showing 5 changed files with 57 additions and 51 deletions.
diff --git a/...ava/io/micrometer/core/instrument/binder/grpc/DefaultGrpcClientObservationConvention.java b/...ava/io/micrometer/core/instrument/binder/grpc/DefaultGrpcClientObservationConvention.java
@@ -15,13 +15,9 @@
  */
 package io.micrometer.core.instrument.binder.grpc;
 
-import io.micrometer.common.KeyValue;
 import io.micrometer.common.KeyValues;
 import io.micrometer.core.instrument.binder.grpc.GrpcObservationDocumentation.LowCardinalityKeyNames;
 
-import java.util.ArrayList;
-import java.util.List;
-
 /**
  * Default convention for gRPC client. This class defines how to extract values from
  * {@link GrpcClientObservationContext}.
@@ -43,14 +39,11 @@ public String getContextualName(GrpcClientObservationContext context) {
 
     @Override
     public KeyValues getLowCardinalityKeyValues(GrpcClientObservationContext context) {
-        List<KeyValue> keyValues = new ArrayList<>();
-        keyValues.add(LowCardinalityKeyNames.METHOD.withValue(context.getMethodName()));
-        keyValues.add(LowCardinalityKeyNames.SERVICE.withValue(context.getServiceName()));
-        keyValues.add(LowCardinalityKeyNames.METHOD_TYPE.withValue(context.getMethodType().name()));
-        if (context.getStatusCode() != null) {
-            keyValues.add(LowCardinalityKeyNames.STATUS_CODE.withValue(context.getStatusCode().name()));
-        }
-        return KeyValues.of(keyValues);
+        String statusCode = context.getStatusCode() != null ? context.getStatusCode().name() : UNKNOWN;
+        return KeyValues.of(LowCardinalityKeyNames.STATUS_CODE.withValue(statusCode),
+                LowCardinalityKeyNames.METHOD.withValue(context.getMethodName()),
+                LowCardinalityKeyNames.SERVICE.withValue(context.getServiceName()),
+                LowCardinalityKeyNames.METHOD_TYPE.withValue(context.getMethodType().name()));
     }
 
 }
diff --git a/...ava/io/micrometer/core/instrument/binder/grpc/DefaultGrpcServerObservationConvention.java b/...ava/io/micrometer/core/instrument/binder/grpc/DefaultGrpcServerObservationConvention.java
@@ -15,13 +15,9 @@
  */
 package io.micrometer.core.instrument.binder.grpc;
 
-import io.micrometer.common.KeyValue;
 import io.micrometer.common.KeyValues;
 import io.micrometer.core.instrument.binder.grpc.GrpcObservationDocumentation.LowCardinalityKeyNames;
 
-import java.util.ArrayList;
-import java.util.List;
-
 /**
  * Default convention for gRPC server. This class defines how to extract values from
  * {@link GrpcServerObservationContext}.
@@ -43,14 +39,11 @@ public String getContextualName(GrpcServerObservationContext context) {
 
     @Override
     public KeyValues getLowCardinalityKeyValues(GrpcServerObservationContext context) {
-        List<KeyValue> keyValues = new ArrayList<>();
-        keyValues.add(LowCardinalityKeyNames.METHOD.withValue(context.getMethodName()));
-        keyValues.add(LowCardinalityKeyNames.SERVICE.withValue(context.getServiceName()));
-        keyValues.add(LowCardinalityKeyNames.METHOD_TYPE.withValue(context.getMethodType().name()));
-        if (context.getStatusCode() != null) {
-            keyValues.add(LowCardinalityKeyNames.STATUS_CODE.withValue(context.getStatusCode().name()));
-        }
-        return KeyValues.of(keyValues);
+        String statusCode = context.getStatusCode() != null ? context.getStatusCode().name() : UNKNOWN;
+        return KeyValues.of(LowCardinalityKeyNames.STATUS_CODE.withValue(statusCode),
+                LowCardinalityKeyNames.METHOD.withValue(context.getMethodName()),
+                LowCardinalityKeyNames.SERVICE.withValue(context.getServiceName()),
+                LowCardinalityKeyNames.METHOD_TYPE.withValue(context.getMethodType().name()));
     }
 
 }
diff --git a/.../main/java/io/micrometer/core/instrument/binder/grpc/GrpcClientObservationConvention.java b/.../main/java/io/micrometer/core/instrument/binder/grpc/GrpcClientObservationConvention.java
@@ -26,6 +26,8 @@
  */
 public interface GrpcClientObservationConvention extends ObservationConvention<GrpcClientObservationContext> {
 
+    String UNKNOWN = "UNKNOWN";
+
     @Override
     default boolean supportsContext(Context context) {
         return context instanceof GrpcClientObservationContext;

diff --git a/.../main/java/io/micrometer/core/instrument/binder/grpc/GrpcServerObservationConvention.java b/.../main/java/io/micrometer/core/instrument/binder/grpc/GrpcServerObservationConvention.java
@@ -26,6 +26,8 @@
  */
 public interface GrpcServerObservationConvention extends ObservationConvention<GrpcServerObservationContext> {
 
+    String UNKNOWN = "UNKNOWN";
+
     @Override
     default boolean supportsContext(Context context) {
         return context instanceof GrpcServerObservationContext;

diff --git a/...ter-core/src/test/java/io/micrometer/core/instrument/binder/grpc/GrpcObservationTest.java b/...ter-core/src/test/java/io/micrometer/core/instrument/binder/grpc/GrpcObservationTest.java
@@ -44,6 +44,7 @@
 import io.micrometer.observation.Observation.Event;
 import io.micrometer.observation.ObservationHandler;
 import io.micrometer.observation.ObservationTextPublisher;
+import io.micrometer.observation.tck.ObservationContextAssert;
 import io.micrometer.observation.tck.TestObservationRegistry;
 import io.micrometer.observation.tck.TestObservationRegistryAssert;
 import org.junit.jupiter.api.AfterEach;
@@ -154,9 +155,13 @@ void unaryRpc() {
             assertThat(clientHandler.getEvents()).containsExactly(GrpcClientEvents.MESSAGE_SENT,
                     GrpcClientEvents.MESSAGE_RECEIVED);
             // tag::assertion[]
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
+            TestObservationRegistryAssert.assertThat(observationRegistry).hasAnObservation(observationContextAssert -> {
+                observationContextAssert.hasNameEqualTo("grpc.client");
+                assertCommonKeyValueNames(observationContextAssert);
+            }).hasAnObservation(observationContextAssert -> {
+                observationContextAssert.hasNameEqualTo("grpc.server");
+                assertCommonKeyValueNames(observationContextAssert);
+            });
             // end::assertion[]
         }
 
@@ -188,9 +193,7 @@ public void onFailure(Throwable t) {
 
             await().until(() -> futures.stream().allMatch(Future::isDone));
             assertThat(responses).hasSize(count).containsExactlyInAnyOrderElementsOf(messages);
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
+            assertClientAndServerObservations();
         }
 
         @Test
@@ -230,9 +233,7 @@ void clientStreamingRpc() {
             verifyServerContext("grpc.testing.SimpleService", "ClientStreamingRpc",
                     "grpc.testing.SimpleService/ClientStreamingRpc", MethodType.CLIENT_STREAMING);
             assertThat(serverHandler.getContext().getStatusCode()).isEqualTo(Code.OK);
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
+            assertClientAndServerObservations();
         }
 
         @Test
@@ -264,9 +265,7 @@ void serverStreamingRpc() {
             assertThat(clientHandler.getContext().getStatusCode()).isEqualTo(Code.OK);
             assertThat(clientHandler.getEvents()).containsExactly(GrpcClientEvents.MESSAGE_SENT,
                     GrpcClientEvents.MESSAGE_RECEIVED, GrpcClientEvents.MESSAGE_RECEIVED);
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
+            assertClientAndServerObservations();
         }
 
         @Test
@@ -316,9 +315,7 @@ void bidiStreamingRpc() {
 
             assertThat(serverHandler.getContext().getStatusCode()).isEqualTo(Code.OK);
             assertThat(clientHandler.getContext().getStatusCode()).isEqualTo(Code.OK);
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
-                .hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
+            assertClientAndServerObservations();
         }
 
         private StreamObserver<SimpleResponse> createResponseObserver(List<String> messages, AtomicBoolean completed) {
@@ -343,6 +340,16 @@ public void onCompleted() {
 
     }
 
+    private void assertClientAndServerObservations() {
+        TestObservationRegistryAssert.assertThat(observationRegistry).hasAnObservation(observationContextAssert -> {
+            observationContextAssert.hasNameEqualTo("grpc.client");
+            assertCommonKeyValueNames(observationContextAssert);
+        }).hasAnObservation(observationContextAssert -> {
+            observationContextAssert.hasNameEqualTo("grpc.server");
+            assertCommonKeyValueNames(observationContextAssert);
+        });
+    }
+
     @Nested
     class WithExceptionService {
 
@@ -373,9 +380,7 @@ void unaryRpcFailure() {
             assertThat(clientHandler.getContext().getStatusCode()).isEqualTo(Code.UNKNOWN);
             assertThat(serverHandler.getEvents()).containsExactly(GrpcServerEvents.MESSAGE_RECEIVED);
             assertThat(clientHandler.getEvents()).containsExactly(GrpcClientEvents.MESSAGE_SENT);
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(
-                        observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server").hasError());
+            assertServerErrorObservation();
         }
 
         @Test
@@ -397,9 +402,7 @@ void clientStreamingRpcFailure() {
             assertThat(serverHandler.getContext().getStatusCode()).isNull();
             assertThat(clientHandler.getEvents()).isEmpty();
             assertThat(serverHandler.getEvents()).isEmpty();
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(
-                        observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server").hasError());
+            assertServerErrorObservation();
         }
 
         @Test
@@ -423,9 +426,7 @@ void serverStreamingRpcFailure() {
             assertThat(serverHandler.getContext().getStatusCode()).isNull();
             assertThat(clientHandler.getEvents()).containsExactly(GrpcClientEvents.MESSAGE_SENT);
             assertThat(serverHandler.getEvents()).containsExactly(GrpcServerEvents.MESSAGE_RECEIVED);
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(
-                        observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server").hasError());
+            assertServerErrorObservation();
         }
 
         @Test
@@ -448,9 +449,14 @@ void bidiStreamingRpcFailure() {
             assertThat(serverHandler.getContext().getStatusCode()).isNull();
             assertThat(clientHandler.getEvents()).isEmpty();
             assertThat(serverHandler.getEvents()).isEmpty();
-            TestObservationRegistryAssert.assertThat(observationRegistry)
-                .hasAnObservation(
-                        observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server").hasError());
+            assertServerErrorObservation();
+        }
+
+        private void assertServerErrorObservation() {
+            TestObservationRegistryAssert.assertThat(observationRegistry).hasAnObservation(observationContextAssert -> {
+                observationContextAssert.hasNameEqualTo("grpc.server").hasError();
+                assertCommonKeyValueNames(observationContextAssert);
+            });
         }
 
         private StreamObserver<SimpleResponse> createResponseObserver(AtomicBoolean errored) {
@@ -498,6 +504,16 @@ void verifyClientContext(String serviceName, String methodName, String contextua
         });
     }
 
+    void assertCommonKeyValueNames(ObservationContextAssert<?> observationContextAssert) {
+        observationContextAssert
+            .hasLowCardinalityKeyValueWithKey(GrpcObservationDocumentation.LowCardinalityKeyNames.METHOD.asString())
+            .hasLowCardinalityKeyValueWithKey(
+                    GrpcObservationDocumentation.LowCardinalityKeyNames.METHOD_TYPE.asString())
+            .hasLowCardinalityKeyValueWithKey(GrpcObservationDocumentation.LowCardinalityKeyNames.SERVICE.asString())
+            .hasLowCardinalityKeyValueWithKey(
+                    GrpcObservationDocumentation.LowCardinalityKeyNames.STATUS_CODE.asString());
+    }
+
     // GRPC service extending SimpleService and provides echo implementation.
     static class EchoService extends SimpleServiceImplBase {