From c3bf9c5d6b1df0b6a1df7cb8005a38f5859d6cc2 Mon Sep 17 00:00:00 2001 From: Hangzhi <1454678938@qq.com> Date: Sat, 3 Apr 2021 12:31:05 +0800 Subject: [PATCH 1/6] http.url must not contain credentials --- .../instrumentation/api/tracer/HttpClientTracer.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java index 9256135908b1..1a546bb5912b 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java @@ -182,9 +182,13 @@ private void setUrl(AttributeSetter setter, REQUEST request) { try { URI url = url(request); if (url != null) { + if (url.getUserInfo() != null) { + throw new IllegalArgumentException("http.url MUST NOT contain credentials!"); + } netPeerAttributes.setNetPeer(setter, url.getHost(), null, url.getPort()); setter.setAttribute(SemanticAttributes.HTTP_URL, url.toString()); } + } catch (Exception e) { log.debug("Error tagging url", e); } From 11d539ad18376b34a65bb16afc3ce9f4fa194b5c Mon Sep 17 00:00:00 2001 From: Hangzhi <1454678938@qq.com> Date: Tue, 6 Apr 2021 15:31:51 +0800 Subject: [PATCH 2/6] remove user info from url --- .../instrumentation/api/tracer/HttpClientTracer.java | 8 +++++--- .../api/tracer/HttpClientTracerTest.groovy | 1 + 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java index 1a546bb5912b..916002460ca3 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java @@ -182,11 +182,13 @@ private void setUrl(AttributeSetter setter, REQUEST request) { try { URI url = url(request); if (url != null) { + netPeerAttributes.setNetPeer(setter, url.getHost(), null, url.getPort()); if (url.getUserInfo() != null) { - throw new IllegalArgumentException("http.url MUST NOT contain credentials!"); + setter.setAttribute( + SemanticAttributes.HTTP_URL, url.toString().replace(url.getUserInfo() + '@', "")); + } else { + setter.setAttribute(SemanticAttributes.HTTP_URL, url.toString()); } - netPeerAttributes.setNetPeer(setter, url.getHost(), null, url.getPort()); - setter.setAttribute(SemanticAttributes.HTTP_URL, url.toString()); } } catch (Exception e) { diff --git a/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy b/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy index fe3916eed43c..905fa4fd8450 100644 --- a/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy +++ b/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy @@ -101,6 +101,7 @@ class HttpClientTracerTest extends BaseTracerTest { false | "https://host:0" | "https://host:0" | "" | null | "host" | null false | "https://host/path" | "https://host/path" | "" | null | "host" | null false | "http://host:99/path?query#fragment" | "http://host:99/path?query#fragment" | "" | null | "host" | 99 + false | "http://usr:pswd@host/path" | "https://host/path" | "" | null | "host" | null req = [url: url == null ? null : new URI(url)] } From 56736f8fc8946189145833df6e5c8753f5fb5a8d Mon Sep 17 00:00:00 2001 From: Hangzhi <1454678938@qq.com> Date: Tue, 6 Apr 2021 16:31:09 +0800 Subject: [PATCH 3/6] Update HttpClientTracerTest.groovy fix inconsistency in protocol --- .../instrumentation/api/tracer/HttpClientTracerTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy b/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy index 905fa4fd8450..852db73e0b0a 100644 --- a/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy +++ b/instrumentation-api/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy @@ -101,7 +101,7 @@ class HttpClientTracerTest extends BaseTracerTest { false | "https://host:0" | "https://host:0" | "" | null | "host" | null false | "https://host/path" | "https://host/path" | "" | null | "host" | null false | "http://host:99/path?query#fragment" | "http://host:99/path?query#fragment" | "" | null | "host" | 99 - false | "http://usr:pswd@host/path" | "https://host/path" | "" | null | "host" | null + false | "https://usr:pswd@host/path" | "https://host/path" | "" | null | "host" | null req = [url: url == null ? null : new URI(url)] } From 6cea3555cc0af3ecf3e45015eecfa5bad2357700 Mon Sep 17 00:00:00 2001 From: Hangzhi <1454678938@qq.com> Date: Sat, 17 Apr 2021 22:24:59 +0800 Subject: [PATCH 4/6] fix httpClient and httpServer --- .../api/tracer/HttpClientTracer.java | 18 ++++++++----- .../api/tracer/HttpServerTracer.java | 26 +++++++++++++++++-- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java index 916002460ca3..c3dc18747727 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java @@ -183,14 +183,18 @@ private void setUrl(AttributeSetter setter, REQUEST request) { URI url = url(request); if (url != null) { netPeerAttributes.setNetPeer(setter, url.getHost(), null, url.getPort()); - if (url.getUserInfo() != null) { - setter.setAttribute( - SemanticAttributes.HTTP_URL, url.toString().replace(url.getUserInfo() + '@', "")); - } else { - setter.setAttribute(SemanticAttributes.HTTP_URL, url.toString()); - } + setter.setAttribute( + SemanticAttributes.HTTP_URL, + new URI( + url.getScheme(), + null, + url.getHost(), + url.getPort(), + url.getPath(), + url.getQuery(), + url.getFragment()) + .toString()); } - } catch (Exception e) { log.debug("Error tagging url", e); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java index 0ee1f41a96de..9d6e1b048888 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java @@ -14,8 +14,12 @@ import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import java.lang.reflect.Method; +import java.net.URI; +import java.net.URISyntaxException; import java.util.concurrent.TimeUnit; import org.checkerframework.checker.nullness.qual.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; // TODO In search for a better home package @@ -32,6 +36,8 @@ */ public abstract class HttpServerTracer extends BaseTracer { + private static final Logger log = LoggerFactory.getLogger(HttpClientTracer.class); + // the class name is part of the attribute name, so that it will be shaded when used in javaagent // instrumentation, and won't conflict with usage outside javaagent instrumentation public static final String CONTEXT_ATTRIBUTE = HttpServerTracer.class.getName() + ".Context"; @@ -185,7 +191,22 @@ protected void onRequest(SpanBuilder spanBuilder, REQUEST request) { recommended combinations of attributes and are forced to use http.url. */ private void setUrl(SpanBuilder spanBuilder, REQUEST request) { - spanBuilder.setAttribute(SemanticAttributes.HTTP_URL, url(request)); + try { + URI url = url(request); + spanBuilder.setAttribute( + SemanticAttributes.HTTP_URL, + new URI( + url.getScheme(), + null, + url.getHost(), + url.getPort(), + url.getPath(), + url.getQuery(), + url.getFragment()) + .toString()); + } catch (Exception e) { + log.debug("Error tagging url", e); + } } protected void onConnectionAndRequest( @@ -267,7 +288,8 @@ private static void setStatus(Span span, int status) { protected abstract TextMapGetter getGetter(); - protected abstract String url(REQUEST request); + @Nullable + protected abstract URI url(REQUEST request) throws URISyntaxException; protected abstract String method(REQUEST request); From 739ad02a1d333f30b07aec2f86547537f854e478 Mon Sep 17 00:00:00 2001 From: Hangzhi <1454678938@qq.com> Date: Sat, 17 Apr 2021 22:35:42 +0800 Subject: [PATCH 5/6] Update HttpServerTracer.java --- .../api/tracer/HttpServerTracer.java | 26 ++----------------- 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java index 9d6e1b048888..0ee1f41a96de 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java @@ -14,12 +14,8 @@ import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; import java.lang.reflect.Method; -import java.net.URI; -import java.net.URISyntaxException; import java.util.concurrent.TimeUnit; import org.checkerframework.checker.nullness.qual.Nullable; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; // TODO In search for a better home package @@ -36,8 +32,6 @@ */ public abstract class HttpServerTracer extends BaseTracer { - private static final Logger log = LoggerFactory.getLogger(HttpClientTracer.class); - // the class name is part of the attribute name, so that it will be shaded when used in javaagent // instrumentation, and won't conflict with usage outside javaagent instrumentation public static final String CONTEXT_ATTRIBUTE = HttpServerTracer.class.getName() + ".Context"; @@ -191,22 +185,7 @@ protected void onRequest(SpanBuilder spanBuilder, REQUEST request) { recommended combinations of attributes and are forced to use http.url. */ private void setUrl(SpanBuilder spanBuilder, REQUEST request) { - try { - URI url = url(request); - spanBuilder.setAttribute( - SemanticAttributes.HTTP_URL, - new URI( - url.getScheme(), - null, - url.getHost(), - url.getPort(), - url.getPath(), - url.getQuery(), - url.getFragment()) - .toString()); - } catch (Exception e) { - log.debug("Error tagging url", e); - } + spanBuilder.setAttribute(SemanticAttributes.HTTP_URL, url(request)); } protected void onConnectionAndRequest( @@ -288,8 +267,7 @@ private static void setStatus(Span span, int status) { protected abstract TextMapGetter getGetter(); - @Nullable - protected abstract URI url(REQUEST request) throws URISyntaxException; + protected abstract String url(REQUEST request); protected abstract String method(REQUEST request); From 724c27cb222ec5d29aa1e07198ae0ca932fc6e41 Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Fri, 7 May 2021 12:40:56 +0900 Subject: [PATCH 6/6] Only scrub userinfo if present --- .../api/tracer/HttpClientTracer.java | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java index c3dc18747727..ac18029122b5 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java @@ -183,17 +183,21 @@ private void setUrl(AttributeSetter setter, REQUEST request) { URI url = url(request); if (url != null) { netPeerAttributes.setNetPeer(setter, url.getHost(), null, url.getPort()); - setter.setAttribute( - SemanticAttributes.HTTP_URL, - new URI( - url.getScheme(), - null, - url.getHost(), - url.getPort(), - url.getPath(), - url.getQuery(), - url.getFragment()) - .toString()); + final URI sanitized; + if (url.getUserInfo() != null) { + sanitized = + new URI( + url.getScheme(), + null, + url.getHost(), + url.getPort(), + url.getPath(), + url.getQuery(), + url.getFragment()); + } else { + sanitized = url; + } + setter.setAttribute(SemanticAttributes.HTTP_URL, sanitized.toString()); } } catch (Exception e) { log.debug("Error tagging url", e);