Skip to content

Commit 53a7c52

Browse files
shakuzenshakuzen
committed
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
1 parent a90bdd9 commit 53a7c52

File tree

5 files changed

+57
-51
lines changed

5 files changed

+57
-51
lines changed

‎micrometer-core/src/main/java/io/micrometer/core/instrument/binder/grpc/DefaultGrpcClientObservationConvention.java

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,9 @@
1515
*/
1616
package io.micrometer.core.instrument.binder.grpc;
1717

18-
import io.micrometer.common.KeyValue;
1918
import io.micrometer.common.KeyValues;
2019
import io.micrometer.core.instrument.binder.grpc.GrpcObservationDocumentation.LowCardinalityKeyNames;
2120

22-
import java.util.ArrayList;
23-
import java.util.List;
24-
2521
/**
2622
* Default convention for gRPC client. This class defines how to extract values from
2723
* {@link GrpcClientObservationContext}.
@@ -43,14 +39,11 @@ public String getContextualName(GrpcClientObservationContext context) {
4339

4440
@Override
4541
public KeyValues getLowCardinalityKeyValues(GrpcClientObservationContext context) {
46-
List<KeyValue> keyValues = new ArrayList<>();
47-
keyValues.add(LowCardinalityKeyNames.METHOD.withValue(context.getMethodName()));
48-
keyValues.add(LowCardinalityKeyNames.SERVICE.withValue(context.getServiceName()));
49-
keyValues.add(LowCardinalityKeyNames.METHOD_TYPE.withValue(context.getMethodType().name()));
50-
if (context.getStatusCode() != null) {
51-
keyValues.add(LowCardinalityKeyNames.STATUS_CODE.withValue(context.getStatusCode().name()));
52-
}
53-
return KeyValues.of(keyValues);
42+
String statusCode = context.getStatusCode() != null ? context.getStatusCode().name() : UNKNOWN;
43+
return KeyValues.of(LowCardinalityKeyNames.STATUS_CODE.withValue(statusCode),
44+
LowCardinalityKeyNames.METHOD.withValue(context.getMethodName()),
45+
LowCardinalityKeyNames.SERVICE.withValue(context.getServiceName()),
46+
LowCardinalityKeyNames.METHOD_TYPE.withValue(context.getMethodType().name()));
5447
}
5548

5649
}

‎micrometer-core/src/main/java/io/micrometer/core/instrument/binder/grpc/DefaultGrpcServerObservationConvention.java

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,9 @@
1515
*/
1616
package io.micrometer.core.instrument.binder.grpc;
1717

18-
import io.micrometer.common.KeyValue;
1918
import io.micrometer.common.KeyValues;
2019
import io.micrometer.core.instrument.binder.grpc.GrpcObservationDocumentation.LowCardinalityKeyNames;
2120

22-
import java.util.ArrayList;
23-
import java.util.List;
24-
2521
/**
2622
* Default convention for gRPC server. This class defines how to extract values from
2723
* {@link GrpcServerObservationContext}.
@@ -43,14 +39,11 @@ public String getContextualName(GrpcServerObservationContext context) {
4339

4440
@Override
4541
public KeyValues getLowCardinalityKeyValues(GrpcServerObservationContext context) {
46-
List<KeyValue> keyValues = new ArrayList<>();
47-
keyValues.add(LowCardinalityKeyNames.METHOD.withValue(context.getMethodName()));
48-
keyValues.add(LowCardinalityKeyNames.SERVICE.withValue(context.getServiceName()));
49-
keyValues.add(LowCardinalityKeyNames.METHOD_TYPE.withValue(context.getMethodType().name()));
50-
if (context.getStatusCode() != null) {
51-
keyValues.add(LowCardinalityKeyNames.STATUS_CODE.withValue(context.getStatusCode().name()));
52-
}
53-
return KeyValues.of(keyValues);
42+
String statusCode = context.getStatusCode() != null ? context.getStatusCode().name() : UNKNOWN;
43+
return KeyValues.of(LowCardinalityKeyNames.STATUS_CODE.withValue(statusCode),
44+
LowCardinalityKeyNames.METHOD.withValue(context.getMethodName()),
45+
LowCardinalityKeyNames.SERVICE.withValue(context.getServiceName()),
46+
LowCardinalityKeyNames.METHOD_TYPE.withValue(context.getMethodType().name()));
5447
}
5548

5649
}

‎micrometer-core/src/main/java/io/micrometer/core/instrument/binder/grpc/GrpcClientObservationConvention.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626
*/
2727
public interface GrpcClientObservationConvention extends ObservationConvention<GrpcClientObservationContext> {
2828

29+
String UNKNOWN = "UNKNOWN";
30+
2931
@Override
3032
default boolean supportsContext(Context context) {
3133
return context instanceof GrpcClientObservationContext;

‎micrometer-core/src/main/java/io/micrometer/core/instrument/binder/grpc/GrpcServerObservationConvention.java

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626
*/
2727
public interface GrpcServerObservationConvention extends ObservationConvention<GrpcServerObservationContext> {
2828

29+
String UNKNOWN = "UNKNOWN";
30+
2931
@Override
3032
default boolean supportsContext(Context context) {
3133
return context instanceof GrpcServerObservationContext;

‎micrometer-core/src/test/java/io/micrometer/core/instrument/binder/grpc/GrpcObservationTest.java

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import io.micrometer.observation.Observation.Event;
4545
import io.micrometer.observation.ObservationHandler;
4646
import io.micrometer.observation.ObservationTextPublisher;
47+
import io.micrometer.observation.tck.ObservationContextAssert;
4748
import io.micrometer.observation.tck.TestObservationRegistry;
4849
import io.micrometer.observation.tck.TestObservationRegistryAssert;
4950
import org.junit.jupiter.api.AfterEach;
@@ -154,9 +155,13 @@ void unaryRpc() {
154155
assertThat(clientHandler.getEvents()).containsExactly(GrpcClientEvents.MESSAGE_SENT,
155156
GrpcClientEvents.MESSAGE_RECEIVED);
156157
// tag::assertion[]
157-
TestObservationRegistryAssert.assertThat(observationRegistry)
158-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
159-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
158+
TestObservationRegistryAssert.assertThat(observationRegistry).hasAnObservation(observationContextAssert -> {
159+
observationContextAssert.hasNameEqualTo("grpc.client");
160+
assertCommonKeyValueNames(observationContextAssert);
161+
}).hasAnObservation(observationContextAssert -> {
162+
observationContextAssert.hasNameEqualTo("grpc.server");
163+
assertCommonKeyValueNames(observationContextAssert);
164+
});
160165
// end::assertion[]
161166
}
162167

@@ -188,9 +193,7 @@ public void onFailure(Throwable t) {
188193

189194
await().until(() -> futures.stream().allMatch(Future::isDone));
190195
assertThat(responses).hasSize(count).containsExactlyInAnyOrderElementsOf(messages);
191-
TestObservationRegistryAssert.assertThat(observationRegistry)
192-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
193-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
196+
assertClientAndServerObservations();
194197
}
195198

196199
@Test
@@ -230,9 +233,7 @@ void clientStreamingRpc() {
230233
verifyServerContext("grpc.testing.SimpleService", "ClientStreamingRpc",
231234
"grpc.testing.SimpleService/ClientStreamingRpc", MethodType.CLIENT_STREAMING);
232235
assertThat(serverHandler.getContext().getStatusCode()).isEqualTo(Code.OK);
233-
TestObservationRegistryAssert.assertThat(observationRegistry)
234-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
235-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
236+
assertClientAndServerObservations();
236237
}
237238

238239
@Test
@@ -264,9 +265,7 @@ void serverStreamingRpc() {
264265
assertThat(clientHandler.getContext().getStatusCode()).isEqualTo(Code.OK);
265266
assertThat(clientHandler.getEvents()).containsExactly(GrpcClientEvents.MESSAGE_SENT,
266267
GrpcClientEvents.MESSAGE_RECEIVED, GrpcClientEvents.MESSAGE_RECEIVED);
267-
TestObservationRegistryAssert.assertThat(observationRegistry)
268-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
269-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
268+
assertClientAndServerObservations();
270269
}
271270

272271
@Test
@@ -316,9 +315,7 @@ void bidiStreamingRpc() {
316315

317316
assertThat(serverHandler.getContext().getStatusCode()).isEqualTo(Code.OK);
318317
assertThat(clientHandler.getContext().getStatusCode()).isEqualTo(Code.OK);
319-
TestObservationRegistryAssert.assertThat(observationRegistry)
320-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.client"))
321-
.hasAnObservation(observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server"));
318+
assertClientAndServerObservations();
322319
}
323320

324321
private StreamObserver<SimpleResponse> createResponseObserver(List<String> messages, AtomicBoolean completed) {
@@ -343,6 +340,16 @@ public void onCompleted() {
343340

344341
}
345342

343+
private void assertClientAndServerObservations() {
344+
TestObservationRegistryAssert.assertThat(observationRegistry).hasAnObservation(observationContextAssert -> {
345+
observationContextAssert.hasNameEqualTo("grpc.client");
346+
assertCommonKeyValueNames(observationContextAssert);
347+
}).hasAnObservation(observationContextAssert -> {
348+
observationContextAssert.hasNameEqualTo("grpc.server");
349+
assertCommonKeyValueNames(observationContextAssert);
350+
});
351+
}
352+
346353
@Nested
347354
class WithExceptionService {
348355

@@ -373,9 +380,7 @@ void unaryRpcFailure() {
373380
assertThat(clientHandler.getContext().getStatusCode()).isEqualTo(Code.UNKNOWN);
374381
assertThat(serverHandler.getEvents()).containsExactly(GrpcServerEvents.MESSAGE_RECEIVED);
375382
assertThat(clientHandler.getEvents()).containsExactly(GrpcClientEvents.MESSAGE_SENT);
376-
TestObservationRegistryAssert.assertThat(observationRegistry)
377-
.hasAnObservation(
378-
observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server").hasError());
383+
assertServerErrorObservation();
379384
}
380385

381386
@Test
@@ -397,9 +402,7 @@ void clientStreamingRpcFailure() {
397402
assertThat(serverHandler.getContext().getStatusCode()).isNull();
398403
assertThat(clientHandler.getEvents()).isEmpty();
399404
assertThat(serverHandler.getEvents()).isEmpty();
400-
TestObservationRegistryAssert.assertThat(observationRegistry)
401-
.hasAnObservation(
402-
observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server").hasError());
405+
assertServerErrorObservation();
403406
}
404407

405408
@Test
@@ -423,9 +426,7 @@ void serverStreamingRpcFailure() {
423426
assertThat(serverHandler.getContext().getStatusCode()).isNull();
424427
assertThat(clientHandler.getEvents()).containsExactly(GrpcClientEvents.MESSAGE_SENT);
425428
assertThat(serverHandler.getEvents()).containsExactly(GrpcServerEvents.MESSAGE_RECEIVED);
426-
TestObservationRegistryAssert.assertThat(observationRegistry)
427-
.hasAnObservation(
428-
observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server").hasError());
429+
assertServerErrorObservation();
429430
}
430431

431432
@Test
@@ -448,9 +449,14 @@ void bidiStreamingRpcFailure() {
448449
assertThat(serverHandler.getContext().getStatusCode()).isNull();
449450
assertThat(clientHandler.getEvents()).isEmpty();
450451
assertThat(serverHandler.getEvents()).isEmpty();
451-
TestObservationRegistryAssert.assertThat(observationRegistry)
452-
.hasAnObservation(
453-
observationContextAssert -> observationContextAssert.hasNameEqualTo("grpc.server").hasError());
452+
assertServerErrorObservation();
453+
}
454+
455+
private void assertServerErrorObservation() {
456+
TestObservationRegistryAssert.assertThat(observationRegistry).hasAnObservation(observationContextAssert -> {
457+
observationContextAssert.hasNameEqualTo("grpc.server").hasError();
458+
assertCommonKeyValueNames(observationContextAssert);
459+
});
454460
}
455461

456462
private StreamObserver<SimpleResponse> createResponseObserver(AtomicBoolean errored) {
@@ -498,6 +504,16 @@ void verifyClientContext(String serviceName, String methodName, String contextua
498504
});
499505
}
500506

507+
void assertCommonKeyValueNames(ObservationContextAssert<?> observationContextAssert) {
508+
observationContextAssert
509+
.hasLowCardinalityKeyValueWithKey(GrpcObservationDocumentation.LowCardinalityKeyNames.METHOD.asString())
510+
.hasLowCardinalityKeyValueWithKey(
511+
GrpcObservationDocumentation.LowCardinalityKeyNames.METHOD_TYPE.asString())
512+
.hasLowCardinalityKeyValueWithKey(GrpcObservationDocumentation.LowCardinalityKeyNames.SERVICE.asString())
513+
.hasLowCardinalityKeyValueWithKey(
514+
GrpcObservationDocumentation.LowCardinalityKeyNames.STATUS_CODE.asString());
515+
}
516+
501517
// GRPC service extending SimpleService and provides echo implementation.
502518
static class EchoService extends SimpleServiceImplBase {
503519

0 commit comments

Comments
 (0)