Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use http.route in HttpServerMetrics #5266

Merged
merged 2 commits into from
Jan 31, 2022
Merged

Use http.route in HttpServerMetrics #5266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1aa48f3
Use http.route in HttpServerMetrics
Jan 28, 2022
1d918d7
remove http.target fallback
Jan 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion ...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ private boolean isBetterRoute(String name) {
return name.length() > routeLength;
}

// TODO: use that in HttpServerMetrics
/**
* Returns the {@code http.route} attribute value that's stored in the passed {@code context}, or
* null if it was not set before.
Expand Down
21 changes: 20 additions & 1 deletion ...c/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
import io.opentelemetry.instrumentation.api.annotations.UnstableApi;
import io.opentelemetry.instrumentation.api.instrumenter.RequestListener;
import io.opentelemetry.instrumentation.api.instrumenter.RequestMetrics;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import java.util.concurrent.TimeUnit;
import java.util.function.Function;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -49,8 +51,14 @@ public static RequestMetrics get() {

private final LongUpDownCounter activeRequests;
private final DoubleHistogram duration;
private final Function<Context, String> httpRouteHolderGetter;

private HttpServerMetrics(Meter meter) {
this(meter, HttpRouteHolder::getRoute);
}

// visible for tests
HttpServerMetrics(Meter meter, Function<Context, String> httpRouteHolderGetter) {
activeRequests =
meter
.upDownCounterBuilder("http.server.active_requests")
Expand All @@ -64,6 +72,8 @@ private HttpServerMetrics(Meter meter) {
.setUnit("ms")
.setDescription("The duration of the inbound HTTP request")
.build();

this.httpRouteHolderGetter = httpRouteHolderGetter;
}

@Override
Expand All @@ -86,10 +96,19 @@ 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(), addHttpRoute(context, endAttributes)),
context);
}

// TODO: the http.route should be extracted by the AttributesExtractor, HttpServerMetrics should
// not access the context to get it
private Attributes addHttpRoute(Context context, Attributes endAttributes) {
String route = httpRouteHolderGetter.apply(context);
return route == null
? endAttributes
: endAttributes.toBuilder().put(SemanticAttributes.HTTP_ROUTE, route).build();
}

@AutoValue
abstract static class State {

Expand Down
39 changes: 2 additions & 37 deletions ...ain/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.instrumentation.api.config.Config;
import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
import java.util.HashSet;
import java.util.Set;
Expand All @@ -19,19 +18,9 @@
@SuppressWarnings("rawtypes")
final class TemporaryMetricsView {

// TODO (trask) remove this once http.route is captured consistently
//
// this is not enabled by default because it falls back to http.target (which can be high
// cardinality) when http.route is not available
private static final boolean USE_HTTP_TARGET_FALLBACK =
Config.get()
.getBoolean("otel.instrumentation.metrics.experimental.use-http-target-fallback", false);

private static final Set<AttributeKey> durationAlwaysInclude = buildDurationAlwaysInclude();
private static final Set<AttributeKey> durationClientView = buildDurationClientView();
private static final Set<AttributeKey> durationServerView = buildDurationServerView();
private static final Set<AttributeKey> durationServerFallbackView =
buildDurationServerFallbackView();
private static final Set<AttributeKey> activeRequestsView = buildActiveRequestsView();

private static Set<AttributeKey> buildDurationAlwaysInclude() {
Expand Down Expand Up @@ -65,19 +54,6 @@ private static Set<AttributeKey> buildDurationServerView() {
return view;
}

private static Set<AttributeKey> buildDurationServerFallbackView() {
// We pull identifying attributes according to:
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives
// With the following caveat:
// - we always rely on http.route + http.host in this repository.
// - we prefer http.route (which is scrubbed) over http.target (which is not scrubbed).
Set<AttributeKey> view = new HashSet<>(durationAlwaysInclude);
view.add(SemanticAttributes.HTTP_SCHEME);
view.add(SemanticAttributes.HTTP_HOST);
view.add(SemanticAttributes.HTTP_TARGET);
return view;
}

private static Set<AttributeKey> buildActiveRequestsView() {
// the list of included metrics is from
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attributes
Expand All @@ -97,21 +73,10 @@ static Attributes applyClientDurationView(Attributes startAttributes, Attributes
return filtered.build();
}

private static <T> boolean containsAttribute(
AttributeKey<T> key, Attributes startAttributes, Attributes endAttributes) {
return startAttributes.get(key) != null || endAttributes.get(key) != null;
}

static Attributes applyServerDurationView(Attributes startAttributes, Attributes endAttributes) {
Set<AttributeKey> fullSet = durationServerView;
// Use http.target when http.route is not available.
if (USE_HTTP_TARGET_FALLBACK
&& !containsAttribute(SemanticAttributes.HTTP_ROUTE, startAttributes, endAttributes)) {
fullSet = durationServerFallbackView;
}
AttributesBuilder filtered = Attributes.builder();
applyView(filtered, startAttributes, fullSet);
applyView(filtered, endAttributes, fullSet);
applyView(filtered, startAttributes, durationServerView);
applyView(filtered, endAttributes, durationServerView);
return filtered.build();
}

Expand Down
43 changes: 43 additions & 0 deletions ...st/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpServerMetricsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,49 @@ void collectsMetrics() {
.satisfiesExactly(point -> assertThat(point).hasSum(300 /* millis */)));
}

@Test
void collectsHttpRouteFromContext() {
// given
InMemoryMetricReader metricReader = InMemoryMetricReader.create();
SdkMeterProvider meterProvider =
SdkMeterProvider.builder()
.registerMetricReader(metricReader)
.setMinimumCollectionInterval(Duration.ZERO)
.build();

RequestListener listener = new HttpServerMetrics(meterProvider.get("test"), c -> "/test/{id}");

Attributes requestAttributes =
Attributes.builder().put("http.host", "host").put("http.scheme", "https").build();

Attributes responseAttributes = Attributes.empty();

Context parentContext = Context.root();

// when
Context context = listener.start(parentContext, requestAttributes, nanos(100));
listener.end(context, responseAttributes, nanos(200));

// then
assertThat(metricReader.collectAllMetrics())
.anySatisfy(
metric ->
assertThat(metric)
.hasName("http.server.duration")
.hasUnit("ms")
.hasDoubleHistogram()
.points()
.satisfiesExactly(
point ->
assertThat(point)
.hasSum(100 /* millis */)
.attributes()
.containsOnly(
attributeEntry("http.scheme", "https"),
attributeEntry("http.host", "host"),
attributeEntry("http.route", "/test/{id}"))));
}

private static long nanos(int millis) {
return TimeUnit.MILLISECONDS.toNanos(millis);
}
Expand Down
31 changes: 0 additions & 31 deletions ...java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,37 +109,6 @@ public void shouldApplyServerDurationView_withRoute() {
attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
}

@Test
public void shouldApplyServerDurationView_withTarget() {
Attributes startAttributes =
Attributes.builder()
.put(SemanticAttributes.HTTP_METHOD, "GET")
.put(SemanticAttributes.HTTP_URL, "https://somehost/high/cardinality/12345")
.put(SemanticAttributes.HTTP_SCHEME, "https")
.put(SemanticAttributes.HTTP_HOST, "somehost")
.put(SemanticAttributes.HTTP_SERVER_NAME, "somehost")
.put(
SemanticAttributes.HTTP_TARGET, "/somehost/high/cardinality/12345;jsessionId=12145")
.put(SemanticAttributes.NET_HOST_NAME, "somehost")
.put(SemanticAttributes.NET_HOST_PORT, 443)
.build();

Attributes endAttributes =
Attributes.builder()
.put(SemanticAttributes.HTTP_STATUS_CODE, 500)
.put(SemanticAttributes.NET_PEER_NAME, "somehost2")
.put(SemanticAttributes.NET_PEER_IP, "127.0.0.1")
.put(SemanticAttributes.NET_PEER_PORT, 443)
.build();

OpenTelemetryAssertions.assertThat(applyServerDurationView(startAttributes, endAttributes))
.containsOnly(
attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"),
attributeEntry(SemanticAttributes.HTTP_HOST.getKey(), "somehost"),
attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
}

@Test
public void shouldApplyActiveRequestsView() {
Attributes attributes =
Expand Down