Handle null peerName in GrpcClientObservationConvention - #7381
Conversation
a2db9f0 to
32a3e7c
Compare
shakuzen
left a comment
shakuzen
left a comment
There was a problem hiding this comment.
The change looks good for fixing the issue. I think we should also update the nullability of peerName and the corresponding methods to be accurate, which may have helped catch this. Let's also add a test that would have caught this. Perhaps something like:
@Test
void nullPeerNameShouldBeHandledGracefully() {
DefaultGrpcClientObservationConvention convention = new DefaultGrpcClientObservationConvention();
GrpcClientObservationContext context = new GrpcClientObservationContext((carrier, key, value) -> {});
context.setMethodType(MethodDescriptor.MethodType.UNARY);
// Context is intentionally left without a peerName set
KeyValues keyValues = convention.getLowCardinalityKeyValues(context);
assertThat(keyValues).contains(KeyValue.of("net.peer.name", "UNKNOWN"));
}
c693425 to
5894e1d
Compare
jonatan-ivanov
changed the title
I fixed this in 77fffaa. |
|
If we want to fix this in |
When NameResolver.getServiceAuthority() returns empty string, the ObservationGrpcClientInterceptor silently catches URISyntaxException, leaving peerName null. This caused NPE when DefaultGrpcClientObservationConvention.getLowCardinalityKeyValues() passed null to ImmutableKeyValue.requireNonNull(). Fix: Add null check for peerName with "UNKNOWN" fallback value, consistent with how other fields (statusCode, peerPort, method, service) are already handled. Closes micrometer-metricsgh-7380 Co-authored-by: Jonatan Ivanov <[email protected]> Signed-off-by: zheng2.wang <[email protected]>
77fffaa to
a2a7b72
Compare
|
I rebased this on top of |
|
@GGGGGHT Thank you for the PR, this is now merged into |
3 participants
When NameResolver.getServiceAuthority() returns empty string, the ObservationGrpcClientInterceptor silently catches the URISyntaxException, leaving peerName as null. This caused NPE when
DefaultGrpcClientObservationConvention.getLowCardinalityKeyValues() passed null to ImmutableKeyValue.requireNonNull().
Fix: Add null check for peerName with "UNKNOWN" fallback value, consistent with how other fields (statusCode, peerPort, method, service) are already handled.
Issue: micrometer-metrics/micrometer#7380