From dd39c37ffbb4d814d3efa88ebfbaff63cfad8b6d Mon Sep 17 00:00:00 2001 From: Marcin Grzejszczak Date: Fri, 14 Jul 2023 11:04:05 +0200 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Ikhun Um --- .../DefaultHttpClientObservationConvention.java | 8 ++++---- .../client/observation/MicrometerObservationClient.java | 5 +++-- .../observation/DefaultServiceObservationConvention.java | 8 ++++---- .../observation/HttpServerObservationDocumentation.java | 2 +- .../server/observation/MicrometerObservationService.java | 3 ++- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/client/observation/DefaultHttpClientObservationConvention.java b/core/src/main/java/com/linecorp/armeria/client/observation/DefaultHttpClientObservationConvention.java index 4d2b548f8857..d228352112ac 100644 --- a/core/src/main/java/com/linecorp/armeria/client/observation/DefaultHttpClientObservationConvention.java +++ b/core/src/main/java/com/linecorp/armeria/client/observation/DefaultHttpClientObservationConvention.java @@ -42,8 +42,8 @@ public KeyValues getLowCardinalityKeyValues(HttpClientContext context) { final ClientRequestContext ctx = context.requestContext(); KeyValues keyValues = KeyValues.of( LowCardinalityKeys.HTTP_METHOD.withValue(ctx.method().name())); - if (context.getResponse() != null) { - final RequestLog log = ctx.log().ensureComplete(); + final RequestLog log = context.getResponse(); + if (log != null) { keyValues = keyValues.and(LowCardinalityKeys.HTTP_PROTOCOL.withValue(protocol(log))); final String serFmt = serializationFormat(log); if (serFmt != null) { @@ -89,7 +89,7 @@ public KeyValues getHighCardinalityKeyValues(HttpClientContext context) { * Returns the {@link SessionProtocol#uriText()} of the {@link RequestLog}. */ private static String protocol(RequestLog requestLog) { - return requestLog.scheme().sessionProtocol().uriText(); + return requestLog.sessionProtocol().uriText(); } /** @@ -97,7 +97,7 @@ private static String protocol(RequestLog requestLog) { */ @Nullable private static String serializationFormat(RequestLog requestLog) { - final SerializationFormat serFmt = requestLog.scheme().serializationFormat(); + final SerializationFormat serFmt = requestLog.serializationFormat(); return serFmt == SerializationFormat.NONE ? null : serFmt.uriText(); } diff --git a/core/src/main/java/com/linecorp/armeria/client/observation/MicrometerObservationClient.java b/core/src/main/java/com/linecorp/armeria/client/observation/MicrometerObservationClient.java index 3e562b9b832a..bb1ebac0f5d7 100644 --- a/core/src/main/java/com/linecorp/armeria/client/observation/MicrometerObservationClient.java +++ b/core/src/main/java/com/linecorp/armeria/client/observation/MicrometerObservationClient.java @@ -76,6 +76,7 @@ public final class MicrometerObservationClient extends SimpleDecoratingHttpClien */ public static Function newDecorator( ObservationRegistry observationRegistry) { + requireNonNull(observationRegistry, "observationRegistry"); return delegate -> new MicrometerObservationClient(delegate, observationRegistry, null); } @@ -111,7 +112,7 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex final RequestHeadersBuilder newHeaders = req.headers().toBuilder(); final HttpClientContext httpClientContext = new HttpClientContext(ctx, newHeaders, req); final Observation observation = HttpClientObservationDocumentation.OBSERVATION.observation( - this.httpClientObservationConvention, DefaultHttpClientObservationConvention.INSTANCE, + httpClientObservationConvention, DefaultHttpClientObservationConvention.INSTANCE, () -> httpClientContext, observationRegistry).start(); final HttpRequest newReq = req.withHeaders(newHeaders); ctx.updateRequest(newReq); @@ -127,7 +128,7 @@ public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Ex return observation.scopedChecked(() -> unwrap().execute(ctx, newReq)); } - private void enrichObservation(ClientRequestContext ctx, HttpClientContext httpClientContext, + private static void enrichObservation(ClientRequestContext ctx, HttpClientContext httpClientContext, Observation observation) { if (observation.isNoop()) { // For no-op spans, we only need to inject into headers and don't set any other attributes. diff --git a/core/src/main/java/com/linecorp/armeria/server/observation/DefaultServiceObservationConvention.java b/core/src/main/java/com/linecorp/armeria/server/observation/DefaultServiceObservationConvention.java index 81aef142d43a..ca75aa6adf29 100644 --- a/core/src/main/java/com/linecorp/armeria/server/observation/DefaultServiceObservationConvention.java +++ b/core/src/main/java/com/linecorp/armeria/server/observation/DefaultServiceObservationConvention.java @@ -66,8 +66,8 @@ public KeyValues getHighCardinalityKeyValues(HttpServerContext context) { firstNonNull(context.httpRequest().authority(), "UNKNOWN")), HighCardinalityKeys.HTTP_URL.withValue(ctx.uri().toString()) ); - if (context.getResponse() != null) { - final RequestLog log = ctx.log().ensureComplete(); + final RequestLog log = context.getResponse(); + if (log != null) { final InetSocketAddress raddr = ctx.remoteAddress(); keyValues = keyValues.and( @@ -92,7 +92,7 @@ public KeyValues getHighCardinalityKeyValues(HttpServerContext context) { * Returns the {@link SessionProtocol#uriText()} of the {@link RequestLog}. */ private static String protocol(RequestLog requestLog) { - return requestLog.scheme().sessionProtocol().uriText(); + return requestLog.sessionProtocol().uriText(); } /** @@ -100,7 +100,7 @@ private static String protocol(RequestLog requestLog) { */ @Nullable private static String serializationFormat(RequestLog requestLog) { - final SerializationFormat serFmt = requestLog.scheme().serializationFormat(); + final SerializationFormat serFmt = requestLog.serializationFormat(); return serFmt == SerializationFormat.NONE ? null : serFmt.uriText(); } diff --git a/core/src/main/java/com/linecorp/armeria/server/observation/HttpServerObservationDocumentation.java b/core/src/main/java/com/linecorp/armeria/server/observation/HttpServerObservationDocumentation.java index 6d02c11d26d6..223ae7f7f597 100644 --- a/core/src/main/java/com/linecorp/armeria/server/observation/HttpServerObservationDocumentation.java +++ b/core/src/main/java/com/linecorp/armeria/server/observation/HttpServerObservationDocumentation.java @@ -26,7 +26,7 @@ import io.micrometer.observation.ObservationConvention; import io.micrometer.observation.docs.ObservationDocumentation; -enum HttpServerObservationDocumentation implements ObservationDocumentation { +enum HttpServiceObservationDocumentation implements ObservationDocumentation { /** * A span collected by {@link MicrometerObservationService}. diff --git a/core/src/main/java/com/linecorp/armeria/server/observation/MicrometerObservationService.java b/core/src/main/java/com/linecorp/armeria/server/observation/MicrometerObservationService.java index 14777c116e56..8699a7528b37 100644 --- a/core/src/main/java/com/linecorp/armeria/server/observation/MicrometerObservationService.java +++ b/core/src/main/java/com/linecorp/armeria/server/observation/MicrometerObservationService.java @@ -67,7 +67,7 @@ * } */ @UnstableApi -public final class MicrometerObservationService extends SimpleDecoratingHttpService { +public final class ObservationService extends SimpleDecoratingHttpService { /** * Creates a new micrometer observation integrated {@link HttpService} decorator using the @@ -75,6 +75,7 @@ public final class MicrometerObservationService extends SimpleDecoratingHttpServ */ public static Function newDecorator(ObservationRegistry observationRegistry) { + requireNonNull(observationRegistry, "observationRegistry"); return service -> new MicrometerObservationService(service, observationRegistry, null); }