Skip to content

Commit

Permalink
Applied changes following the review
Browse files Browse the repository at this point in the history
  • Loading branch information
@marcingrzejszczak
marcingrzejszczak committed Jul 20, 2023
1 parent dd39c37 commit 3b3ed20
Show file tree
Hide file tree
Showing 14 changed files with 241 additions and 151 deletions.
19 changes: 15 additions & 4 deletions ...client/observation/HttpClientContext.java → ...observation/ClientObservationContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.linecorp.armeria.client.observation;

import com.google.common.base.MoreObjects;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.common.HttpRequest;
import com.linecorp.armeria.common.RequestHeadersBuilder;
Expand All @@ -28,7 +30,7 @@
import io.micrometer.observation.transport.RequestReplySenderContext;

/**
* A {@link Context} which may be used in conjunction with {@link MicrometerObservationClient}
* A {@link Context} which may be used in conjunction with {@link ObservationClient}
* to implement custom {@link ObservationConvention}s or {@link ObservationHandler}s.
* <pre>{@code
* ObservationConvention<HttpClientContext> convention = ...
Expand All @@ -38,13 +40,14 @@
* }</pre>
*/
@UnstableApi
public final class HttpClientContext extends RequestReplySenderContext<RequestHeadersBuilder, RequestLog> {
public final class ClientObservationContext
extends RequestReplySenderContext<RequestHeadersBuilder, RequestLog> {

private final ClientRequestContext clientRequestContext;
private final HttpRequest httpRequest;

HttpClientContext(ClientRequestContext clientRequestContext, RequestHeadersBuilder carrier,
HttpRequest httpRequest) {
ClientObservationContext(ClientRequestContext clientRequestContext, RequestHeadersBuilder carrier,
HttpRequest httpRequest) {
super(RequestHeadersBuilder::add);
this.clientRequestContext = clientRequestContext;
this.httpRequest = httpRequest;
Expand All @@ -64,4 +67,12 @@ public ClientRequestContext requestContext() {
public HttpRequest httpRequest() {
return httpRequest;
}

@Override
public String toString() {
return MoreObjects.toStringHelper(this).omitNullValues()
.add("clientRequestContext", clientRequestContext)
.add("httpRequest", httpRequest)
.toString();
}
}
86 changes: 59 additions & 27 deletions .../java/com/linecorp/armeria/client/observation/DefaultHttpClientObservationConvention.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,69 +20,98 @@

import java.net.InetSocketAddress;

import com.google.common.collect.ImmutableList;

import com.linecorp.armeria.client.ClientRequestContext;
import com.linecorp.armeria.client.observation.HttpClientObservationDocumentation.HighCardinalityKeys;
import com.linecorp.armeria.client.observation.HttpClientObservationDocumentation.LowCardinalityKeys;
import com.linecorp.armeria.common.SerializationFormat;
import com.linecorp.armeria.common.SessionProtocol;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.logging.RequestLog;
import com.linecorp.armeria.common.logging.RequestLogAccess;
import com.linecorp.armeria.common.logging.RequestLogProperty;

import io.micrometer.common.KeyValue;
import io.micrometer.common.KeyValues;
import io.micrometer.observation.Observation.Context;
import io.micrometer.observation.ObservationConvention;

class DefaultHttpClientObservationConvention implements ObservationConvention<HttpClientContext> {
class DefaultHttpClientObservationConvention implements ObservationConvention<ClientObservationContext> {

static final DefaultHttpClientObservationConvention INSTANCE =
new DefaultHttpClientObservationConvention();

@Override
public KeyValues getLowCardinalityKeyValues(HttpClientContext context) {
public KeyValues getLowCardinalityKeyValues(ClientObservationContext context) {
final ClientRequestContext ctx = context.requestContext();
KeyValues keyValues = KeyValues.of(
LowCardinalityKeys.HTTP_METHOD.withValue(ctx.method().name()));
int expectedSize = 1;
final RequestLog log = context.getResponse();
KeyValue protocol = null;
KeyValue serializationFormat = null;
KeyValue statusCode = null;
if (log != null) {
keyValues = keyValues.and(LowCardinalityKeys.HTTP_PROTOCOL.withValue(protocol(log)));
protocol = LowCardinalityKeys.HTTP_PROTOCOL.withValue(protocol(log));
statusCode = LowCardinalityKeys.STATUS_CODE
.withValue(log.responseStatus().codeAsText());
expectedSize = 3;
final String serFmt = serializationFormat(log);
if (serFmt != null) {
keyValues = keyValues.and(LowCardinalityKeys.HTTP_SERIALIZATION_FORMAT.withValue(serFmt));
expectedSize = 4;
serializationFormat = LowCardinalityKeys.HTTP_SERIALIZATION_FORMAT.withValue(serFmt);
}
keyValues = keyValues.and(LowCardinalityKeys.STATUS_CODE
.withValue(log.responseStatus().codeAsText()));
}
return keyValues;
ImmutableList.Builder<KeyValue> builder = ImmutableList.builderWithExpectedSize(expectedSize);
builder.add(LowCardinalityKeys.HTTP_METHOD.withValue(ctx.method().name()));
addIfNotNull(protocol, builder);
addIfNotNull(statusCode, builder);
addIfNotNull(serializationFormat, builder);
return KeyValues.of(builder.build());
}

private void addIfNotNull(@Nullable KeyValue keyValue, ImmutableList.Builder<KeyValue> builder) {
if (keyValue != null) {
builder.add(keyValue);
}
}

@Override
public KeyValues getHighCardinalityKeyValues(HttpClientContext context) {
public KeyValues getHighCardinalityKeyValues(ClientObservationContext context) {
final ClientRequestContext ctx = context.requestContext();
KeyValues keyValues = KeyValues.of(
HighCardinalityKeys.HTTP_PATH.withValue(ctx.path()),
HighCardinalityKeys.HTTP_HOST.withValue(firstNonNull(ctx.authority(), "UNKNOWN")),
HighCardinalityKeys.HTTP_URL.withValue(ctx.uri().toString())
);
int expectedSize = 3;
KeyValue addressRemote = null;
KeyValue addressLocal = null;
KeyValue error = null;
if (context.getResponse() != null) {
final RequestLog log = ctx.log().ensureComplete();
final InetSocketAddress raddr = ctx.remoteAddress();
if (raddr != null) {
keyValues = keyValues.and(HighCardinalityKeys.ADDRESS_REMOTE.withValue(raddr.toString()));
expectedSize = expectedSize + 1;
addressRemote = HighCardinalityKeys.ADDRESS_REMOTE.withValue(raddr.toString());
}
final InetSocketAddress laddr = ctx.localAddress();
if (laddr != null) {
keyValues = keyValues.and(HighCardinalityKeys.ADDRESS_LOCAL.withValue(laddr.toString()));
expectedSize = expectedSize + 1;
addressLocal = HighCardinalityKeys.ADDRESS_LOCAL.withValue(laddr.toString());
}

final Throwable responseCause = log.responseCause();
if (responseCause != null) {
keyValues = keyValues.and(HighCardinalityKeys.ERROR.withValue(responseCause.toString()));
expectedSize = expectedSize + 1;
error = HighCardinalityKeys.ERROR.withValue(responseCause.toString());
} else if (log.responseStatus().isError()) {
keyValues =
keyValues.and(HighCardinalityKeys.ERROR.withValue(log.responseStatus().codeAsText()));
expectedSize = expectedSize + 1;
error = HighCardinalityKeys.ERROR.withValue(log.responseStatus().codeAsText());
}
}
return keyValues;
ImmutableList.Builder<KeyValue> builder = ImmutableList.builderWithExpectedSize(expectedSize);
builder.add(HighCardinalityKeys.HTTP_PATH.withValue(ctx.path()),
HighCardinalityKeys.HTTP_HOST.withValue(firstNonNull(ctx.authority(), "UNKNOWN")),
HighCardinalityKeys.HTTP_URL.withValue(ctx.uri().toString()));
addIfNotNull(addressRemote, builder);
addIfNotNull(addressLocal, builder);
addIfNotNull(error, builder);
return KeyValues.of(builder.build());
}

/**
Expand All @@ -107,15 +136,18 @@ public String getName() {
}

@Override
public String getContextualName(HttpClientContext context) {
final ClientRequestContext clientRequestContext = context.requestContext();
final RequestLog log = clientRequestContext.log().ensureComplete();
final String name = log.name();
return firstNonNull(name, context.getName());
public String getContextualName(ClientObservationContext context) {
final RequestLogAccess logAccess = context.requestContext().log();
if (logAccess.isAvailable(
RequestLogProperty.NAME)) {
return logAccess.partial().fullName();
} else {
return context.getName();
}
}

@Override
public boolean supportsContext(Context context) {
return context instanceof HttpClientContext;
return context instanceof ClientObservationContext;
}
}
2 changes: 1 addition & 1 deletion ...main/java/com/linecorp/armeria/client/observation/HttpClientObservationDocumentation.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
enum HttpClientObservationDocumentation implements ObservationDocumentation {

/**
* A span collected by {@link MicrometerObservationClient}.
* A span collected by {@link ObservationClient}.
*/
OBSERVATION {
@Override
Expand Down
37 changes: 20 additions & 17 deletions ...ervation/MicrometerObservationClient.java → ...client/observation/ObservationClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,40 +68,39 @@
* }</pre>
*/
@UnstableApi
public final class MicrometerObservationClient extends SimpleDecoratingHttpClient {
public final class ObservationClient extends SimpleDecoratingHttpClient {

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

/**
* Creates a new micrometer observation integrated {@link HttpClient} decorator
* using the specified {@link ObservationRegistry} and {@link ObservationConvention}.
*/
public static Function<? super HttpClient, MicrometerObservationClient> newDecorator(
public static Function<? super HttpClient, ObservationClient> newDecorator(
ObservationRegistry observationRegistry,
ObservationConvention<HttpClientContext> httpClientObservationConvention) {
@Nullable ObservationConvention<ClientObservationContext> observationConvention) {
requireNonNull(observationRegistry, "observationRegistry");
requireNonNull(observationConvention, "observationConvention");

return delegate -> new MicrometerObservationClient(delegate, observationRegistry,
observationConvention);
return delegate -> new ObservationClient(delegate, observationRegistry,
observationConvention);
}

private final ObservationRegistry observationRegistry;

@Nullable
private final ObservationConvention<HttpClientContext> httpClientObservationConvention;
private final ObservationConvention<ClientObservationContext> httpClientObservationConvention;

private MicrometerObservationClient(
private ObservationClient(
HttpClient delegate, ObservationRegistry observationRegistry,
@Nullable ObservationConvention<HttpClientContext> httpClientObservationConvention) {
@Nullable ObservationConvention<ClientObservationContext> httpClientObservationConvention) {
super(delegate);
this.observationRegistry = requireNonNull(observationRegistry, "observationRegistry");
this.httpClientObservationConvention = httpClientObservationConvention;
Expand All @@ -110,25 +109,29 @@ private MicrometerObservationClient(
@Override
public HttpResponse execute(ClientRequestContext ctx, HttpRequest req) throws Exception {
final RequestHeadersBuilder newHeaders = req.headers().toBuilder();
final HttpClientContext httpClientContext = new HttpClientContext(ctx, newHeaders, req);
final ClientObservationContext clientObservationContext = new ClientObservationContext(ctx, newHeaders, req);
final Observation observation = HttpClientObservationDocumentation.OBSERVATION.observation(
httpClientObservationConvention, DefaultHttpClientObservationConvention.INSTANCE,
() -> httpClientContext, observationRegistry).start();
() -> clientObservationContext, observationRegistry).start();
final HttpRequest newReq = req.withHeaders(newHeaders);
ctx.updateRequest(newReq);
final RequestContextExtension ctxExtension = ctx.as(RequestContextExtension.class);

if (!observationRegistry.isNoop() && !observation.isNoop() && ctxExtension != null) {
if (observationRegistry.isNoop() || observation.isNoop()) {
return unwrap().execute(ctx, newReq);
}

if (ctxExtension != null) {
// Make the span the current span and run scope decorators when the ctx is pushed.
ctxExtension.hook(observation::openScope);
}

enrichObservation(ctx, httpClientContext, observation);
enrichObservation(ctx, clientObservationContext, observation);

return observation.scopedChecked(() -> unwrap().execute(ctx, newReq));
}

private static void enrichObservation(ClientRequestContext ctx, HttpClientContext httpClientContext,
private static void enrichObservation(ClientRequestContext ctx, ClientObservationContext clientObservationContext,
Observation observation) {
if (observation.isNoop()) {
// For no-op spans, we only need to inject into headers and don't set any other attributes.
Expand All @@ -151,7 +154,7 @@ private static void enrichObservation(ClientRequestContext ctx, HttpClientContex
.thenAccept(requestLog -> {
// TODO: ClientConnectionTimings - there is no way to record events
// with a specific timestamp for an observation
httpClientContext.setResponse(requestLog);
clientObservationContext.setResponse(requestLog);
observation.stop();
});
}
Expand Down
Loading

0 comments on commit 3b3ed20

Please sign in to comment.