Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
Co-authored-by: Ikhun Um <ih.pert@gmail.com>
  • Loading branch information
marcingrzejszczak and ikhoon committed Jul 20, 2023
1 parent 50bb96d commit dd39c37
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -89,15 +89,15 @@ 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();
}

/**
* Returns the {@link SerializationFormat#uriText()} if it's not {@link SerializationFormat#NONE}.
*/
@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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ public final class MicrometerObservationClient extends SimpleDecoratingHttpClien
*/
public static Function<? super HttpClient, MicrometerObservationClient> newDecorator(
ObservationRegistry observationRegistry) {
requireNonNull(observationRegistry, "observationRegistry");
return delegate -> new MicrometerObservationClient(delegate, observationRegistry, null);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -92,15 +92,15 @@ 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();
}

/**
* Returns the {@link SerializationFormat#uriText()} if it's not {@link SerializationFormat#NONE}.
*/
@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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,15 @@
* }</pre>
*/
@UnstableApi
public final class MicrometerObservationService extends SimpleDecoratingHttpService {
public final class ObservationService extends SimpleDecoratingHttpService {

/**
* Creates a new micrometer observation integrated {@link HttpService} decorator using the
* specified {@link ObservationRegistry} instance.
*/
public static Function<? super HttpService, MicrometerObservationService>
newDecorator(ObservationRegistry observationRegistry) {
requireNonNull(observationRegistry, "observationRegistry");
return service -> new MicrometerObservationService(service, observationRegistry, null);
}

Expand Down

0 comments on commit dd39c37

Please sign in to comment.