Skip to content

Commit

Permalink
Update HTTP metrics 'view' code to match the specification (#4556)
Browse files Browse the repository at this point in the history 
* Update HTTP metrics 'view' code to match the specification, including optional attribute rules.

* Update server metrics test for new logic.

* Fix client metrics test.

* Spotless fix.

* Updates from Java SiG discussion.

* Fixes from review.

* Apply suggestions from code review

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

* Update spotless from code reivew merge.

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
  • Loading branch information
@jsuereth @trask
jsuereth and trask authored Nov 6, 2021
1 parent 2f47e8a commit d3f8ab6
Show file tree
Hide file tree
Showing 6 changed files with 204 additions and 36 deletions.
4 changes: 2 additions & 2 deletions ...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 @@ -5,7 +5,7 @@

package io.opentelemetry.instrumentation.api.instrumenter.http;

import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyDurationView;
import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyClientDurationView;

import com.google.auto.value.AutoValue;
import io.opentelemetry.api.common.Attributes;
Expand Down Expand Up @@ -76,7 +76,7 @@ public void end(Context context, Attributes endAttributes, long endNanos) {
}
duration.record(
(endNanos - state.startTimeNanos()) / NANOS_PER_MS,
applyDurationView(state.startAttributes(), endAttributes));
applyClientDurationView(state.startAttributes(), endAttributes));
}

@AutoValue
Expand Down
4 changes: 2 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 @@ -6,7 +6,7 @@
package io.opentelemetry.instrumentation.api.instrumenter.http;

import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyActiveRequestsView;
import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyDurationView;
import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyServerDurationView;

import com.google.auto.value.AutoValue;
import io.opentelemetry.api.common.Attributes;
Expand Down Expand Up @@ -89,7 +89,7 @@ public void end(Context context, Attributes endAttributes, long endNanos) {
activeRequests.add(-1, applyActiveRequestsView(state.startAttributes()));
duration.record(
(endNanos - state.startTimeNanos()) / NANOS_PER_MS,
applyDurationView(state.startAttributes(), endAttributes));
applyServerDurationView(state.startAttributes(), endAttributes));
}

@AutoValue
Expand Down
97 changes: 82 additions & 15 deletions ...ain/java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,54 @@
@SuppressWarnings("rawtypes")
final class TemporaryMetricsView {

private static final Set<AttributeKey> durationView = buildDurationView();

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> buildDurationView() {
private static Set<AttributeKey> buildDurationAlwaysInclude() {
// the list of included metrics is from
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attributes
Set<AttributeKey> view = new HashSet<>();
view.add(SemanticAttributes.HTTP_METHOD);
view.add(SemanticAttributes.HTTP_STATUS_CODE); // Optional
view.add(SemanticAttributes.HTTP_FLAVOR); // Optional
return view;
}

private static Set<AttributeKey> buildDurationClientView() {
// We pull identifying attributes according to:
// https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/http-metrics.md#attribute-alternatives
Set<AttributeKey> view = new HashSet<>(durationAlwaysInclude);
view.add(SemanticAttributes.HTTP_URL);
return view;
}

private static Set<AttributeKey> buildDurationServerView() {
// 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_ROUTE);
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_STATUS_CODE);
view.add(SemanticAttributes.HTTP_FLAVOR);
view.add(SemanticAttributes.NET_PEER_NAME);
view.add(SemanticAttributes.NET_PEER_PORT);
view.add(SemanticAttributes.NET_PEER_IP);
view.add(SemanticAttributes.HTTP_SERVER_NAME);
view.add(SemanticAttributes.NET_HOST_NAME);
view.add(SemanticAttributes.NET_HOST_PORT);
view.add(SemanticAttributes.HTTP_HOST);
view.add(SemanticAttributes.HTTP_TARGET);
return view;
}

Expand All @@ -52,10 +81,27 @@ private static Set<AttributeKey> buildActiveRequestsView() {
return view;
}

static Attributes applyDurationView(Attributes startAttributes, Attributes endAttributes) {
static Attributes applyClientDurationView(Attributes startAttributes, Attributes endAttributes) {
AttributesBuilder filtered = Attributes.builder();
applyView(filtered, startAttributes, durationView);
applyView(filtered, endAttributes, durationView);
applyView(filtered, startAttributes, durationClientView);
applyView(filtered, endAttributes, durationClientView);
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 (!containsAttribute(SemanticAttributes.HTTP_ROUTE, startAttributes, endAttributes)) {
fullSet = durationServerFallbackView;
}
AttributesBuilder filtered = Attributes.builder();
applyView(filtered, startAttributes, fullSet);
applyView(filtered, endAttributes, fullSet);
return filtered.build();
}

Expand All @@ -72,10 +118,31 @@ private static void applyView(
(BiConsumer<AttributeKey, Object>)
(key, value) -> {
if (view.contains(key)) {
filtered.put(key, value);
// For now, we filter query parameters out of URLs in metrics.
if (SemanticAttributes.HTTP_URL.equals(key)
|| SemanticAttributes.HTTP_TARGET.equals(key)) {
filtered.put(key, removeQueryParamFromUrlOrTarget(value.toString()));
} else {
filtered.put(key, value);
}
}
});
}

// Attempt to handle cleaning URLs like http://myServer;jsessionId=1 or targets like
// /my/path?queryParam=2
private static String removeQueryParamFromUrlOrTarget(String urlOrTarget) {
// Note: Maybe not the most robust, but purely to limit cardinality.
int idx = -1;
for (int i = 0; i < urlOrTarget.length(); ++i) {
char ch = urlOrTarget.charAt(i);
if (ch == '?' || ch == ';') {
idx = i;
break;
}
}
return idx == -1 ? urlOrTarget : urlOrTarget.substring(0, idx);
}

private TemporaryMetricsView() {}
}
10 changes: 4 additions & 6 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 @@ -32,7 +32,9 @@ void collectsMetrics() {
Attributes requestAttributes =
Attributes.builder()
.put("http.method", "GET")
.put("http.url", "https://localhost:1234/")
.put("http.host", "host")
.put("http.target", "/")
.put("http.scheme", "https")
.put("net.host.name", "localhost")
.put("net.host.port", 1234)
Expand Down Expand Up @@ -85,14 +87,10 @@ void collectsMetrics() {
.hasSum(150 /* millis */)
.attributes()
.containsOnly(
attributeEntry("http.host", "host"),
attributeEntry("http.url", "https://localhost:1234/"),
attributeEntry("http.method", "GET"),
attributeEntry("http.scheme", "https"),
attributeEntry("http.flavor", "2.0"),
attributeEntry("http.server_name", "server"),
attributeEntry("http.status_code", 200),
attributeEntry("net.host.name", "localhost"),
attributeEntry("net.host.port", 1234L))));
attributeEntry("http.status_code", 200))));
});

listener.end(context2, responseAttributes, nanos(300));
Expand Down
9 changes: 4 additions & 5 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 @@ -33,6 +33,7 @@ void collectsMetrics() {
Attributes.builder()
.put("http.method", "GET")
.put("http.host", "host")
.put("http.target", "/")
.put("http.scheme", "https")
.put("net.host.name", "localhost")
.put("net.host.port", 1234)
Expand Down Expand Up @@ -120,14 +121,12 @@ void collectsMetrics() {
.hasSum(150 /* millis */)
.attributes()
.containsOnly(
attributeEntry("http.scheme", "https"),
attributeEntry("http.host", "host"),
attributeEntry("http.target", "/"),
attributeEntry("http.method", "GET"),
attributeEntry("http.scheme", "https"),
attributeEntry("http.flavor", "2.0"),
attributeEntry("http.server_name", "server"),
attributeEntry("http.status_code", 200),
attributeEntry("net.host.name", "localhost"),
attributeEntry("net.host.port", 1234L))));
attributeEntry("http.flavor", "2.0"))));
});

listener.end(context2, responseAttributes, nanos(300));
Expand Down
116 changes: 110 additions & 6 deletions ...java/io/opentelemetry/instrumentation/api/instrumenter/http/TemporaryMetricsViewTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
package io.opentelemetry.instrumentation.api.instrumenter.http;

import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyActiveRequestsView;
import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyDurationView;
import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyClientDurationView;
import static io.opentelemetry.instrumentation.api.instrumenter.http.TemporaryMetricsView.applyServerDurationView;
import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.attributeEntry;

import io.opentelemetry.api.common.Attributes;
Expand All @@ -17,24 +18,127 @@
public class TemporaryMetricsViewTest {

@Test
public void shouldApplyDurationView() {
public void shouldApplyClientDurationView_urlIdentity() {
Attributes startAttributes =
Attributes.builder()
.put(SemanticAttributes.HTTP_URL, "https://somehost/high/cardinality/12345")
.put(SemanticAttributes.HTTP_SCHEME, "https")
.put(SemanticAttributes.HTTP_METHOD, "GET")
.put(SemanticAttributes.HTTP_URL, "http://somehost/high/cardinality/12345")
.put(SemanticAttributes.NET_PEER_NAME, "somehost")
.put(SemanticAttributes.HTTP_HOST, "somehost")
.put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345")
.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(applyClientDurationView(startAttributes, endAttributes))
.containsOnly(
attributeEntry(
SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"),
attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
}

@Test
public void shouldApplyClientDurationView_urlWithQuery() {
Attributes startAttributes =
Attributes.builder()
.put(
SemanticAttributes.HTTP_URL,
"https://somehost/high/cardinality/12345?jsessionId=121454")
.put(SemanticAttributes.HTTP_METHOD, "GET")
.put(SemanticAttributes.HTTP_SCHEME, "https")
.put(SemanticAttributes.HTTP_HOST, "somehost")
.put(SemanticAttributes.HTTP_TARGET, "/high/cardinality/12345?jsessionId=121454")
.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(applyClientDurationView(startAttributes, endAttributes))
.containsOnly(
attributeEntry(
SemanticAttributes.HTTP_URL.getKey(), "https://somehost/high/cardinality/12345"),
attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
}

@Test
public void shouldApplyServerDurationView_withRoute() {
Attributes startAttributes =
Attributes.builder()
.put(SemanticAttributes.HTTP_METHOD, "GET")
.put(
SemanticAttributes.HTTP_URL,
"https://somehost/high/cardinality/12345?jsessionId=121454")
.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=121454")
.put(SemanticAttributes.HTTP_ROUTE, "/somehost/high/{name}/{id}")
.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_ROUTE.getKey(), "/somehost/high/{name}/{id}"),
attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
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(applyDurationView(startAttributes, endAttributes))
OpenTelemetryAssertions.assertThat(applyServerDurationView(startAttributes, endAttributes))
.containsOnly(
attributeEntry(SemanticAttributes.HTTP_SCHEME.getKey(), "https"),
attributeEntry(SemanticAttributes.HTTP_HOST.getKey(), "somehost"),
attributeEntry(
SemanticAttributes.HTTP_TARGET.getKey(), "/somehost/high/cardinality/12345"),
attributeEntry(SemanticAttributes.HTTP_METHOD.getKey(), "GET"),
attributeEntry(SemanticAttributes.NET_PEER_NAME.getKey(), "somehost2"),
attributeEntry(SemanticAttributes.HTTP_STATUS_CODE.getKey(), 500));
}

Expand Down

0 comments on commit d3f8ab6

Please sign in to comment.