Skip to content

Commit

Permalink
Fix exemplars (#4678)
Browse files Browse the repository at this point in the history
  • Loading branch information
@trask
trask authored Nov 20, 2021
1 parent 5b88f1b commit 260c603
Show file tree
Hide file tree
Showing 7 changed files with 132 additions and 38 deletions.
16 changes: 15 additions & 1 deletion ...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> builder
attributesExtractors;
private final List<? extends ContextCustomizer<? super REQUEST>> contextCustomizers;
private final List<? extends RequestListener> requestListeners;
private final List<? extends RequestListener> requestMetricListeners;
private final ErrorCauseExtractor errorCauseExtractor;
@Nullable private final StartTimeExtractor<REQUEST> startTimeExtractor;
@Nullable private final EndTimeExtractor<REQUEST, RESPONSE> endTimeExtractor;
Expand All @@ -117,6 +118,7 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> builder
this.attributesExtractors = new ArrayList<>(builder.attributesExtractors);
this.contextCustomizers = new ArrayList<>(builder.contextCustomizers);
this.requestListeners = new ArrayList<>(builder.requestListeners);
this.requestMetricListeners = new ArrayList<>(builder.requestMetricListeners);
this.errorCauseExtractor = builder.errorCauseExtractor;
this.startTimeExtractor = builder.startTimeExtractor;
this.endTimeExtractor = builder.endTimeExtractor;
Expand Down Expand Up @@ -192,6 +194,15 @@ public Context start(Context parentContext, REQUEST request) {
Span span = spanBuilder.startSpan();
context = context.with(span);

// request metric listeners need to run after the span has been added to the context in order
// for them to generate exemplars
if (!requestMetricListeners.isEmpty()) {
long startNanos = getNanos(startTime);
for (RequestListener requestListener : requestMetricListeners) {
context = requestListener.start(context, attributes, startNanos);
}
}

return spanSuppressionStrategy.storeInContext(context, spanKind, span);
}

Expand Down Expand Up @@ -221,11 +232,14 @@ public void end(
endTime = endTimeExtractor.extract(request, response, error);
}

if (!requestListeners.isEmpty()) {
if (!requestListeners.isEmpty() || !requestMetricListeners.isEmpty()) {
long endNanos = getNanos(endTime);
for (RequestListener requestListener : requestListeners) {
requestListener.end(context, attributes, endNanos);
}
for (RequestListener requestListener : requestMetricListeners) {
requestListener.end(context, attributes, endNanos);
}
}

StatusCode statusCode = spanStatusExtractor.extract(request, response, error);
Expand Down
3 changes: 2 additions & 1 deletion .../src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public final class InstrumenterBuilder<REQUEST, RESPONSE> {
new ArrayList<>();
final List<ContextCustomizer<? super REQUEST>> contextCustomizers = new ArrayList<>();
final List<RequestListener> requestListeners = new ArrayList<>();
final List<RequestListener> requestMetricListeners = new ArrayList<>();

SpanKindExtractor<? super REQUEST> spanKindExtractor = SpanKindExtractor.alwaysInternal();
SpanStatusExtractor<? super REQUEST, ? super RESPONSE> spanStatusExtractor =
Expand Down Expand Up @@ -124,7 +125,7 @@ public InstrumenterBuilder<REQUEST, RESPONSE> addContextCustomizer(
/** Adds a {@link RequestMetrics} whose metrics will be recorded for request start and stop. */
@UnstableApi
public InstrumenterBuilder<REQUEST, RESPONSE> addRequestMetrics(RequestMetrics factory) {
requestListeners.add(factory.create(meter));
requestMetricListeners.add(factory.create(meter));
return this;
}

Expand Down
3 changes: 2 additions & 1 deletion ...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetrics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ public void end(Context context, Attributes endAttributes, long endNanos) {
}
duration.record(
(endNanos - state.startTimeNanos()) / NANOS_PER_MS,
applyClientDurationView(state.startAttributes(), endAttributes));
applyClientDurationView(state.startAttributes(), endAttributes),
context);
}

@AutoValue
Expand Down
5 changes: 3 additions & 2 deletions ...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ private HttpServerMetrics(Meter meter) {

@Override
public Context start(Context context, Attributes startAttributes, long startNanos) {
activeRequests.add(1, applyActiveRequestsView(startAttributes));
activeRequests.add(1, applyActiveRequestsView(startAttributes), context);

return context.with(
HTTP_SERVER_REQUEST_METRICS_STATE,
Expand All @@ -89,7 +89,8 @@ public void end(Context context, Attributes endAttributes, long endNanos) {
activeRequests.add(-1, applyActiveRequestsView(state.startAttributes()));
duration.record(
(endNanos - state.startTimeNanos()) / NANOS_PER_MS,
applyServerDurationView(state.startAttributes(), endAttributes));
applyServerDurationView(state.startAttributes(), endAttributes),
context);
}

@AutoValue
Expand Down
34 changes: 34 additions & 0 deletions ...api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import static org.assertj.core.api.Assertions.entry;
import static org.mockito.Mockito.when;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanContext;
Expand Down Expand Up @@ -39,6 +40,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -410,6 +412,38 @@ void client_parent() {
.hasParentSpanId("090a0b0c0d0e0f00")));
}

@Test
void requestMetrics() {
AtomicReference<Context> startContext = new AtomicReference<>();
AtomicReference<Context> endContext = new AtomicReference<>();

RequestListener requestListener =
new RequestListener() {
@Override
public Context start(Context context, Attributes startAttributes, long startNanos) {
startContext.set(context);
return context;
}

@Override
public void end(Context context, Attributes endAttributes, long endNanos) {
endContext.set(context);
}
};

Instrumenter<Map<String, String>, Map<String, String>> instrumenter =
Instrumenter.<Map<String, String>, Map<String, String>>builder(
otelTesting.getOpenTelemetry(), "test", unused -> "span")
.addRequestMetrics(meter -> requestListener)
.newServerInstrumenter(new MapGetter());

Context context = instrumenter.start(Context.root(), REQUEST);
instrumenter.end(context, REQUEST, RESPONSE, null);

assertThat(Span.fromContext(startContext.get()).getSpanContext().isValid()).isTrue();
assertThat(Span.fromContext(endContext.get()).getSpanContext().isValid()).isTrue();
}

@Test
void shouldStartSpanWithGivenStartTime() {
// given
Expand Down
41 changes: 30 additions & 11 deletions ...st/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpClientMetricsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
import static org.awaitility.Awaitility.await;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.api.trace.TraceFlags;
import io.opentelemetry.api.trace.TraceState;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.RequestListener;
import io.opentelemetry.sdk.metrics.SdkMeterProvider;
Expand Down Expand Up @@ -47,7 +51,17 @@ void collectsMetrics() {
.put("http.status_code", 200)
.build();

Context context1 = listener.start(Context.current(), requestAttributes, nanos(100));
Context parent =
Context.root()
.with(
Span.wrap(
SpanContext.create(
"ff01020304050600ff0a0b0c0d0e0f00",
"090a0b0c0d0e0f00",
TraceFlags.getSampled(),
TraceState.getDefault())));

Context context1 = listener.start(parent, requestAttributes, nanos(100));

// TODO(anuraaga): Remove await from this file after 1.8.0 hopefully fixes
// https://github.com/open-telemetry/opentelemetry-java/issues/3725
Expand All @@ -58,7 +72,7 @@ void collectsMetrics() {
assertThat(metrics).isEmpty();
});

Context context2 = listener.start(Context.current(), requestAttributes, nanos(150));
Context context2 = listener.start(Context.root(), requestAttributes, nanos(150));

await()
.untilAsserted(
Expand All @@ -82,15 +96,20 @@ void collectsMetrics() {
.hasDoubleHistogram()
.points()
.satisfiesExactly(
point ->
assertThat(point)
.hasSum(150 /* millis */)
.attributes()
.containsOnly(
attributeEntry("http.url", "https://localhost:1234/"),
attributeEntry("http.method", "GET"),
attributeEntry("http.flavor", "2.0"),
attributeEntry("http.status_code", 200))));
point -> {
assertThat(point)
.hasSum(150 /* millis */)
.attributes()
.containsOnly(
attributeEntry("http.url", "https://localhost:1234/"),
attributeEntry("http.method", "GET"),
attributeEntry("http.flavor", "2.0"),
attributeEntry("http.status_code", 200));
assertThat(point).exemplars().hasSize(1);
assertThat(point.getExemplars().get(0))
.hasTraceId("ff01020304050600ff0a0b0c0d0e0f00")
.hasSpanId("090a0b0c0d0e0f00");
}));
});

listener.end(context2, responseAttributes, nanos(300));
Expand Down
68 changes: 46 additions & 22 deletions ...st/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetricsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
import static org.awaitility.Awaitility.await;

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.api.trace.TraceFlags;
import io.opentelemetry.api.trace.TraceState;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.instrumenter.RequestListener;
import io.opentelemetry.sdk.metrics.SdkMeterProvider;
Expand All @@ -23,7 +27,7 @@ class HttpServerMetricsTest {

@Test
void collectsMetrics() {
InMemoryMetricReader metricReader = new InMemoryMetricReader();
InMemoryMetricReader metricReader = InMemoryMetricReader.create();
SdkMeterProvider meterProvider =
SdkMeterProvider.builder().registerMetricReader(metricReader).build();

Expand All @@ -46,7 +50,17 @@ void collectsMetrics() {
.put("http.status_code", 200)
.build();

Context context1 = listener.start(Context.current(), requestAttributes, nanos(100));
Context parent =
Context.root()
.with(
Span.wrap(
SpanContext.create(
"ff01020304050600ff0a0b0c0d0e0f00",
"090a0b0c0d0e0f00",
TraceFlags.getSampled(),
TraceState.getDefault())));

Context context1 = listener.start(parent, requestAttributes, nanos(100));

// TODO(anuraaga): Remove await from this file after 1.8.0 hopefully fixes
// https://github.com/open-telemetry/opentelemetry-java/issues/3725
Expand All @@ -66,17 +80,22 @@ void collectsMetrics() {
.hasLongSum()
.points()
.satisfiesExactly(
point ->
assertThat(point)
.hasValue(1)
.attributes()
.containsOnly(
attributeEntry("http.host", "host"),
attributeEntry("http.method", "GET"),
attributeEntry("http.scheme", "https"))));
point -> {
assertThat(point)
.hasValue(1)
.attributes()
.containsOnly(
attributeEntry("http.host", "host"),
attributeEntry("http.method", "GET"),
attributeEntry("http.scheme", "https"));
assertThat(point).exemplars().hasSize(1);
assertThat(point.getExemplars().get(0))
.hasTraceId("ff01020304050600ff0a0b0c0d0e0f00")
.hasSpanId("090a0b0c0d0e0f00");
}));
});

Context context2 = listener.start(Context.current(), requestAttributes, nanos(150));
Context context2 = listener.start(Context.root(), requestAttributes, nanos(150));

await()
.untilAsserted(
Expand Down Expand Up @@ -116,17 +135,22 @@ void collectsMetrics() {
.hasDoubleHistogram()
.points()
.satisfiesExactly(
point ->
assertThat(point)
.hasSum(150 /* millis */)
.attributes()
.containsOnly(
attributeEntry("http.scheme", "https"),
attributeEntry("http.host", "host"),
attributeEntry("http.target", "/"),
attributeEntry("http.method", "GET"),
attributeEntry("http.status_code", 200),
attributeEntry("http.flavor", "2.0"))));
point -> {
assertThat(point)
.hasSum(150 /* millis */)
.attributes()
.containsOnly(
attributeEntry("http.scheme", "https"),
attributeEntry("http.host", "host"),
attributeEntry("http.target", "/"),
attributeEntry("http.method", "GET"),
attributeEntry("http.status_code", 200),
attributeEntry("http.flavor", "2.0"));
assertThat(point).exemplars().hasSize(1);
assertThat(point.getExemplars().get(0))
.hasTraceId("ff01020304050600ff0a0b0c0d0e0f00")
.hasSpanId("090a0b0c0d0e0f00");
}));
});

listener.end(context2, responseAttributes, nanos(300));
Expand Down

0 comments on commit 260c603

Please sign in to comment.