Skip to content

Remove hostname and port from HttpClientDecorator #1316

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

Merged
tylerbenson merged 2 commits into from
Mar 13, 2020
Merged

Remove hostname and port from HttpClientDecorator #1316

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
0344cff
Remove hostname and port from HttpClientDecorator
Mar 13, 2020
0069c10
oops
Mar 13, 2020
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
28 changes: 10 additions & 18 deletions .../src/main/java/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,6 @@ public abstract class HttpClientDecorator<REQUEST, RESPONSE> extends ClientDecor

protected abstract URI url(REQUEST request) throws URISyntaxException;

protected abstract String hostname(REQUEST request);

protected abstract Integer port(REQUEST request);

protected abstract Integer status(RESPONSE response);

@Override
Expand Down Expand Up @@ -48,9 +44,16 @@ public AgentSpan onRequest(final AgentSpan span, final REQUEST request) {
}
if (url.getHost() != null) {
urlNoParams.append(url.getHost());
if (url.getPort() > 0 && url.getPort() != 80 && url.getPort() != 443) {
urlNoParams.append(":");
urlNoParams.append(url.getPort());
span.setTag(Tags.PEER_HOSTNAME, url.getHost());
if (Config.get().isHttpClientSplitByDomain()) {
span.setTag(DDTags.SERVICE_NAME, url.getHost());
}
if (url.getPort() > 0) {
span.setTag(Tags.PEER_PORT, url.getPort());
if (url.getPort() != 80 && url.getPort() != 443) {
urlNoParams.append(":");
urlNoParams.append(url.getPort());
}
}
}
final String path = url.getPath();
Expand All @@ -70,17 +73,6 @@ public AgentSpan onRequest(final AgentSpan span, final REQUEST request) {
} catch (final Exception e) {
log.debug("Error tagging url", e);
}

span.setTag(Tags.PEER_HOSTNAME, hostname(request));
final Integer port = port(request);
// Negative or Zero ports might represent an unset/null value for an int type. Skip setting.
if (port != null && port > 0) {
span.setTag(Tags.PEER_PORT, port);
}

if (Config.get().isHttpClientSplitByDomain()) {
span.setTag(DDTags.SERVICE_NAME, hostname(request));
}
}
return span;
}
Expand Down
55 changes: 25 additions & 30 deletions ...t/groovy/datadog/trace/bootstrap/instrumentation/decorator/HttpClientDecoratorTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import static datadog.trace.agent.test.utils.ConfigUtils.withConfigOverride
class HttpClientDecoratorTest extends ClientDecoratorTest {

@Shared
def testUrl = new URI("http://myhost/somepath")
def testUrl = new URI("http://myhost:123/somepath")

def span = Mock(AgentSpan)

Expand All @@ -28,10 +28,10 @@ class HttpClientDecoratorTest extends ClientDecoratorTest {
if (req) {
1 * span.setTag(Tags.HTTP_METHOD, req.method)
1 * span.setTag(Tags.HTTP_URL, "$req.url")
1 * span.setTag(Tags.PEER_HOSTNAME, req.host)
1 * span.setTag(Tags.PEER_PORT, req.port)
1 * span.setTag(Tags.PEER_HOSTNAME, req.url.host)
1 * span.setTag(Tags.PEER_PORT, req.url.port)
if (renameService) {
1 * span.setTag(DDTags.SERVICE_NAME, req.host)
1 * span.setTag(DDTags.SERVICE_NAME, req.url.host)
}
}
0 * _
Expand All @@ -40,8 +40,8 @@ class HttpClientDecoratorTest extends ClientDecoratorTest {
renameService | req
false | null
true | null
false | [method: "test-method", url: testUrl, host: "test-host", port: 555]
true | [method: "test-method", url: testUrl, host: "test-host", port: 555]
false | [method: "test-method", url: testUrl]
true | [method: "test-method", url: testUrl]
}

def "test url handling for #url"() {
Expand All @@ -62,23 +62,28 @@ class HttpClientDecoratorTest extends ClientDecoratorTest {
1 * span.setTag(DDTags.HTTP_FRAGMENT, expectedFragment)
}
1 * span.setTag(Tags.HTTP_METHOD, null)
1 * span.setTag(Tags.PEER_HOSTNAME, null)
if (hostname) {
1 * span.setTag(Tags.PEER_HOSTNAME, hostname)
}
if (port) {
1 * span.setTag(Tags.PEER_PORT, port)
}
0 * _

where:
tagQueryString | url | expectedUrl | expectedQuery | expectedFragment
false | null | null | null | null
false | "" | "/" | "" | null
false | "/path?query" | "/path" | "" | null
false | "https://host:0" | "https://host/" | "" | null
false | "https://host/path" | "https://host/path" | "" | null
false | "http://host:99/path?query#fragment" | "http://host:99/path" | "" | null
true | null | null | null | null
true | "" | "/" | null | null
true | "/path?encoded+%28query%29%3F" | "/path" | "encoded+(query)?" | null
true | "https://host:0" | "https://host/" | null | null
true | "https://host/path" | "https://host/path" | null | null
true | "http://host:99/path?query#encoded+%28fragment%29%3F" | "http://host:99/path" | "query" | "encoded+(fragment)?"
tagQueryString | url | expectedUrl | expectedQuery | expectedFragment | hostname | port
false | null | null | null | null | null | null
false | "" | "/" | "" | null | null | null
false | "/path?query" | "/path" | "" | null | null | null
false | "https://host:0" | "https://host/" | "" | null | "host" | null
false | "https://host/path" | "https://host/path" | "" | null | "host" | null
false | "http://host:99/path?query#fragment" | "http://host:99/path" | "" | null | "host" | 99
true | null | null | null | null | null | null
true | "" | "/" | null | null | null | null
true | "/path?encoded+%28query%29%3F" | "/path" | "encoded+(query)?" | null | null | null
true | "https://host:0" | "https://host/" | null | null | "host" | null
true | "https://host/path" | "https://host/path" | null | null | "host" | null
true | "http://host:99/path?query#encoded+%28fragment%29%3F" | "http://host:99/path" | "query" | "encoded+(fragment)?" | "host" | 99

req = [url: url == null ? null : new URI(url)]
}
Expand Down Expand Up @@ -160,16 +165,6 @@ class HttpClientDecoratorTest extends ClientDecoratorTest {
return m.url
}

@Override
protected String hostname(Map m) {
return m.host
}

@Override
protected Integer port(Map m) {
return m.port
}

@Override
protected Integer status(Map m) {
return m.status
Expand Down
10 changes: 0 additions & 10 deletions ...tp-10.0/src/main/java/datadog/trace/instrumentation/akkahttp/AkkaHttpClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,6 @@ protected URI url(final HttpRequest httpRequest) throws URISyntaxException {
return new URI(httpRequest.uri().toString());
}

@Override
protected String hostname(final HttpRequest httpRequest) {
return httpRequest.getUri().host().address();
}

@Override
protected Integer port(final HttpRequest httpRequest) {
return httpRequest.getUri().port();
}

@Override
protected Integer status(final HttpResponse httpResponse) {
return httpResponse.status().intValue();
Expand Down
29 changes: 1 addition & 28 deletions ...a/datadog/trace/instrumentation/apachehttpasyncclient/ApacheHttpAsyncClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.apache.http.protocol.HttpCoreContext;

public class ApacheHttpAsyncClientDecorator extends HttpClientDecorator<HttpRequest, HttpContext> {

public static final ApacheHttpAsyncClientDecorator DECORATE =
new ApacheHttpAsyncClientDecorator();

Expand Down Expand Up @@ -50,34 +51,6 @@ protected URI url(final HttpRequest request) throws URISyntaxException {
}
}

@Override
protected String hostname(final HttpRequest request) {
try {
final URI uri = url(request);
if (uri != null) {
return uri.getHost();
} else {
return null;
}
} catch (final URISyntaxException e) {
return null;
}
}

@Override
protected Integer port(final HttpRequest request) {
try {
final URI uri = url(request);
if (uri != null) {
return uri.getPort();
} else {
return null;
}
} catch (final URISyntaxException e) {
return null;
}
}

@Override
protected Integer status(final HttpContext context) {
final Object responseObject = context.getAttribute(HttpCoreContext.HTTP_RESPONSE);
Expand Down
20 changes: 0 additions & 20 deletions ...c/main/java/datadog/trace/instrumentation/apachehttpclient/ApacheHttpClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,26 +28,6 @@ protected URI url(final HttpUriRequest request) {
return request.getURI();
}

@Override
protected String hostname(final HttpUriRequest httpRequest) {
final URI uri = httpRequest.getURI();
if (uri != null) {
return uri.getHost();
} else {
return null;
}
}

@Override
protected Integer port(final HttpUriRequest httpRequest) {
final URI uri = httpRequest.getURI();
if (uri != null) {
return uri.getPort();
} else {
return null;
}
}

@Override
protected Integer status(final HttpResponse httpResponse) {
return httpResponse.getStatusLine().getStatusCode();
Expand Down
10 changes: 0 additions & 10 deletions ...-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/AwsSdkClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,16 +105,6 @@ protected URI url(final Request request) {
return request.getEndpoint();
}

@Override
protected String hostname(final Request request) {
return null;
}

@Override
protected Integer port(final Request request) {
return null;
}

@Override
protected Integer status(final Response response) {
return response.getHttpResponse().getStatusCode();
Expand Down
7 changes: 7 additions & 0 deletions dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/test/groovy/AWSClientTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ class AWSClientTest extends AgentTestRunner {
"$Tags.HTTP_URL" "$server.address/"
"$Tags.HTTP_METHOD" "$method"
"$Tags.HTTP_STATUS" 200
"$Tags.PEER_PORT" server.address.port
"$Tags.PEER_HOSTNAME" "localhost"
"aws.service" { it.contains(service) }
"aws.endpoint" "$server.address"
"aws.operation" "${operation}Request"
Expand Down Expand Up @@ -240,6 +242,8 @@ class AWSClientTest extends AgentTestRunner {
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"$Tags.HTTP_URL" "http://localhost:${UNUSABLE_PORT}/"
"$Tags.HTTP_METHOD" "$method"
"$Tags.PEER_HOSTNAME" "localhost"
"$Tags.PEER_PORT" 61
"aws.service" { it.contains(service) }
"aws.endpoint" "http://localhost:${UNUSABLE_PORT}"
"aws.operation" "${operation}Request"
Expand Down Expand Up @@ -306,6 +310,7 @@ class AWSClientTest extends AgentTestRunner {
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"$Tags.HTTP_URL" "https://s3.amazonaws.com/"
"$Tags.HTTP_METHOD" "HEAD"
"$Tags.PEER_HOSTNAME" "s3.amazonaws.com"
"aws.service" "Amazon S3"
"aws.endpoint" "https://s3.amazonaws.com"
"aws.operation" "HeadBucketRequest"
Expand Down Expand Up @@ -352,6 +357,8 @@ class AWSClientTest extends AgentTestRunner {
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"$Tags.HTTP_URL" "$server.address/"
"$Tags.HTTP_METHOD" "GET"
"$Tags.PEER_PORT" server.address.port
"$Tags.PEER_HOSTNAME" "localhost"
"aws.service" "Amazon S3"
"aws.endpoint" "$server.address"
"aws.operation" "GetObjectRequest"
Expand Down
7 changes: 7 additions & 0 deletions .../instrumentation/aws-java-sdk-1.11.0/src/test_before_1_11_106/groovy/AWSClientTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ class AWSClientTest extends AgentTestRunner {
"$Tags.HTTP_URL" "$server.address/"
"$Tags.HTTP_METHOD" "$method"
"$Tags.HTTP_STATUS" 200
"$Tags.PEER_PORT" server.address.port
"$Tags.PEER_HOSTNAME" "localhost"
"aws.service" { it.contains(service) }
"aws.endpoint" "$server.address"
"aws.operation" "${operation}Request"
Expand Down Expand Up @@ -185,6 +187,8 @@ class AWSClientTest extends AgentTestRunner {
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"$Tags.HTTP_URL" "http://localhost:${UNUSABLE_PORT}/"
"$Tags.HTTP_METHOD" "$method"
"$Tags.PEER_PORT" 61
"$Tags.PEER_HOSTNAME" "localhost"
"aws.service" { it.contains(service) }
"aws.endpoint" "http://localhost:${UNUSABLE_PORT}"
"aws.operation" "${operation}Request"
Expand Down Expand Up @@ -251,6 +255,7 @@ class AWSClientTest extends AgentTestRunner {
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"$Tags.HTTP_URL" "https://s3.amazonaws.com/"
"$Tags.HTTP_METHOD" "GET"
"$Tags.PEER_HOSTNAME" "s3.amazonaws.com"
"aws.service" "Amazon S3"
"aws.endpoint" "https://s3.amazonaws.com"
"aws.operation" "GetObjectRequest"
Expand Down Expand Up @@ -298,6 +303,8 @@ class AWSClientTest extends AgentTestRunner {
"$Tags.SPAN_KIND" Tags.SPAN_KIND_CLIENT
"$Tags.HTTP_URL" "$server.address/"
"$Tags.HTTP_METHOD" "GET"
"$Tags.PEER_PORT" server.address.port
"$Tags.PEER_HOSTNAME" "localhost"
"aws.service" "Amazon S3"
"aws.endpoint" "http://localhost:$server.address.port"
"aws.operation" "GetObjectRequest"
Expand Down
10 changes: 0 additions & 10 deletions ...va-sdk-2.2/src/main/java8/datadog/trace/instrumentation/aws/v2/AwsSdkClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,6 @@ protected URI url(final SdkHttpRequest request) {
return request.getUri();
}

@Override
protected String hostname(final SdkHttpRequest request) {
return request.host();
}

@Override
protected Integer port(final SdkHttpRequest request) {
return request.port();
}

@Override
protected Integer status(final SdkHttpResponse response) {
return response.statusCode();
Expand Down
18 changes: 0 additions & 18 deletions ...main/java/datadog/trace/instrumentation/commonshttpclient/CommonsHttpClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,24 +35,6 @@ protected URI url(final HttpMethod httpMethod) throws URISyntaxException {
}
}

@Override
protected String hostname(final HttpMethod httpMethod) {
try {
return httpMethod.getURI().getHost();
} catch (final URIException e) {
return null;
}
}

@Override
protected Integer port(final HttpMethod httpMethod) {
try {
return httpMethod.getURI().getPort();
} catch (final URIException e) {
return null;
}
}

@Override
protected Integer status(final HttpMethod httpMethod) {
final StatusLine statusLine = httpMethod.getStatusLine();
Expand Down
10 changes: 0 additions & 10 deletions ...c/main/java/datadog/trace/instrumentation/googlehttpclient/GoogleHttpClientDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,6 @@ protected URI url(final HttpRequest httpRequest) throws URISyntaxException {
return new URI(fixedUrl);
}

@Override
protected String hostname(final HttpRequest httpRequest) {
return httpRequest.getUrl().getHost();
}

@Override
protected Integer port(final HttpRequest httpRequest) {
return httpRequest.getUrl().getPort();
}

@Override
protected Integer status(final HttpResponse httpResponse) {
return httpResponse.getStatusCode();
Expand Down
Loading