diff --git a/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy b/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy index a8ff65e9bd..7674852d96 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy +++ b/instrumentation/apache-httpclient/apache-httpclient-4.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy @@ -92,20 +92,15 @@ class ApacheClientHostRequest extends ApacheHttpClientTest { @Override HttpResponse executeRequest(BasicHttpRequest request, URI uri) { - return client.execute(new HttpHost(uri.getHost(), uri.getPort()), request) + return client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request) } @Override void executeRequestWithCallback(BasicHttpRequest request, URI uri, Consumer callback) { - client.execute(new HttpHost(uri.getHost(), uri.getPort()), request) { + client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request) { callback.accept(it) } } - - @Override - boolean testRemoteConnection() { - return false - } } class ApacheClientHostRequestContext extends ApacheHttpClientTest { @@ -116,20 +111,15 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest callback) { - client.execute(new HttpHost(uri.getHost(), uri.getPort()), request, { + client.execute(new HttpHost(uri.getHost(), uri.getPort(), uri.getScheme()), request, { callback.accept(it) }, new BasicHttpContext()) } - - @Override - boolean testRemoteConnection() { - return false - } } class ApacheClientUriRequest extends ApacheHttpClientTest { diff --git a/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy b/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy index 43af812763..e4c908ccd9 100644 --- a/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy +++ b/instrumentation/apache-httpclient/apache-httpclient-5.0/javaagent/src/test/groovy/ApacheHttpClientTest.groovy @@ -97,20 +97,15 @@ class ApacheClientHostRequest extends ApacheHttpClientTest { @Override ClassicHttpResponse executeRequest(ClassicHttpRequest request, URI uri) { - return client.execute(new HttpHost(uri.getHost(), uri.getPort()), request) + return client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request) } @Override void executeRequestWithCallback(ClassicHttpRequest request, URI uri, Consumer callback) { - client.execute(new HttpHost(uri.getHost(), uri.getPort()), request) { + client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request) { callback.accept(it) } } - - @Override - boolean testRemoteConnection() { - return false - } } class ApacheClientHostRequestContext extends ApacheHttpClientTest { @@ -121,20 +116,15 @@ class ApacheClientHostRequestContext extends ApacheHttpClientTest callback) { - client.execute(new HttpHost(uri.getHost(), uri.getPort()), request, new BasicHttpContext()) { + client.execute(new HttpHost(uri.getScheme(), uri.getHost(), uri.getPort()), request, new BasicHttpContext()) { callback.accept(it) } } - - @Override - boolean testRemoteConnection() { - return false - } } class ApacheClientUriRequest extends ApacheHttpClientTest { diff --git a/instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.groovy b/instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.groovy index 28f43bee95..5c7c0f8a67 100644 --- a/instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.groovy +++ b/instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpClientTest.groovy @@ -63,12 +63,6 @@ abstract class AbstractArmeriaHttpClientTest extends HttpClientTest false } - // TODO(anuraaga): Enable after fixing the test https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/2344 - @Override - boolean testRemoteConnection() { - false - } - // TODO: context not propagated to callback @Override boolean testErrorWithCallback() { diff --git a/instrumentation/java-http-client/javaagent/src/test/groovy/JdkHttpClientTest.groovy b/instrumentation/java-http-client/javaagent/src/test/groovy/JdkHttpClientTest.groovy index 82d9c94825..b16ca779be 100644 --- a/instrumentation/java-http-client/javaagent/src/test/groovy/JdkHttpClientTest.groovy +++ b/instrumentation/java-http-client/javaagent/src/test/groovy/JdkHttpClientTest.groovy @@ -51,12 +51,6 @@ class JdkHttpClientTest extends HttpClientTest implements AgentTest return false } - //We override this test below because it produces somewhat different attributes - @Override - boolean testRemoteConnection() { - return false - } - // TODO nested client span is not created, but context is still injected // which is not what the test expects @Override diff --git a/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/Netty40ClientTest.groovy b/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/Netty40ClientTest.groovy index bae9869b7a..8e5df688a6 100644 --- a/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/Netty40ClientTest.groovy +++ b/instrumentation/netty/netty-4.0/javaagent/src/test/groovy/Netty40ClientTest.groovy @@ -60,7 +60,7 @@ class Netty40ClientTest extends HttpClientTest implement @Override int sendRequest(DefaultFullHttpRequest request, String method, URI uri, Map headers) { - def channel = bootstrap.connect(uri.host, uri.port).sync().channel() + def channel = bootstrap.connect(uri.host, getPort(uri)).sync().channel() def result = new CompletableFuture() channel.pipeline().addLast(new ClientHandler(result)) channel.writeAndFlush(request).get() @@ -69,7 +69,7 @@ class Netty40ClientTest extends HttpClientTest implement @Override void sendRequestWithCallback(DefaultFullHttpRequest request, String method, URI uri, Map headers, RequestResult requestResult) { - Channel ch = bootstrap.connect(uri.host, uri.port).sync().channel() + Channel ch = bootstrap.connect(uri.host, getPort(uri)).sync().channel() def result = new CompletableFuture() result.whenComplete { status, throwable -> requestResult.complete({ status }, throwable) @@ -78,6 +78,18 @@ class Netty40ClientTest extends HttpClientTest implement ch.writeAndFlush(request) } + private static int getPort(URI uri) { + if (uri.port != -1) { + return uri.port + } else if (uri.scheme == "http") { + return 80 + } else if (uri.scheme == "https") { + 443 + } else { + throw new IllegalArgumentException("Unexpected uri: $uri") + } + } + @Override String userAgent() { return "Netty" diff --git a/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/Netty41ClientTest.groovy b/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/Netty41ClientTest.groovy index 25698b2335..cd89ba3f01 100644 --- a/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/Netty41ClientTest.groovy +++ b/instrumentation/netty/netty-4.1/javaagent/src/test/groovy/Netty41ClientTest.groovy @@ -68,7 +68,7 @@ class Netty41ClientTest extends HttpClientTest implement @Override int sendRequest(DefaultFullHttpRequest request, String method, URI uri, Map headers) { - def channel = bootstrap.connect(uri.host, uri.port).sync().channel() + def channel = bootstrap.connect(uri.host, getPort(uri)).sync().channel() def result = new CompletableFuture() channel.pipeline().addLast(new ClientHandler(result)) channel.writeAndFlush(request).get() @@ -77,7 +77,7 @@ class Netty41ClientTest extends HttpClientTest implement @Override void sendRequestWithCallback(DefaultFullHttpRequest request, String method, URI uri, Map headers, RequestResult requestResult) { - Channel ch = bootstrap.connect(uri.host, uri.port).sync().channel() + Channel ch = bootstrap.connect(uri.host, getPort(uri)).sync().channel() def result = new CompletableFuture() result.whenComplete { status, throwable -> requestResult.complete({ status }, throwable) @@ -86,6 +86,18 @@ class Netty41ClientTest extends HttpClientTest implement ch.writeAndFlush(request) } + private static int getPort(URI uri) { + if (uri.port != -1) { + return uri.port + } else if (uri.scheme == "http") { + return 80 + } else if (uri.scheme == "https") { + 443 + } else { + throw new IllegalArgumentException("Unexpected uri: $uri") + } + } + @Override boolean testRedirects() { false diff --git a/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy b/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy index 7018cc760d..0f7847e9cc 100644 --- a/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy +++ b/instrumentation/spring/spring-web-3.1/library/src/test/groovy/RestTemplateInstrumentationTest.groovy @@ -60,11 +60,6 @@ class RestTemplateInstrumentationTest extends HttpClientTest> false } - @Override - boolean testRemoteConnection() { - false - } - // library instrumentation doesn't have a good way of suppressing nested CLIENT spans yet @Override boolean testWithClientParent() { diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy index 59632d02fd..6630f71d6b 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpClientTest.groovy @@ -797,20 +797,29 @@ abstract class HttpClientTest extends InstrumentationSpecification { } attributes { "${SemanticAttributes.NET_TRANSPORT.key}" "IP.TCP" - if (uri.port == UNUSABLE_PORT) { - // TODO(anuraaga): For the unusable port, there isn't actually a peer so we shouldn't be + if (uri.port == UNUSABLE_PORT || uri.host == "192.0.2.1" || (uri.host == "www.google.com" && uri.port == 81)) { + // TODO(anuraaga): For theses cases, there isn't actually a peer so we shouldn't be // filling in peer information but some instrumentation does so based on the URL itself // which is present in HTTP attributes. We should fix this. "${SemanticAttributes.NET_PEER_NAME.key}" { it == null || it == uri.host } - "${SemanticAttributes.NET_PEER_PORT.key}" { it == null || it == UNUSABLE_PORT } + "${SemanticAttributes.NET_PEER_PORT.key}" { it == null || it == uri.port } } else { "${SemanticAttributes.NET_PEER_NAME.key}" uri.host "${SemanticAttributes.NET_PEER_PORT.key}" uri.port > 0 ? uri.port : { it == null || it == 443 } } - "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" } // Optional + if (uri.host == "www.google.com") { + // unpredictable IP address (or can be none if no connection is made, see comment above) + "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it instanceof String } + } else { + "${SemanticAttributes.NET_PEER_IP.key}" { it == null || it == "127.0.0.1" } // Optional + } "${SemanticAttributes.HTTP_URL.key}" { it == "${uri}" || it == "${removeFragment(uri)}" } "${SemanticAttributes.HTTP_METHOD.key}" method - "${SemanticAttributes.HTTP_FLAVOR.key}" httpFlavor + if (uri.host == "www.google.com") { + "${SemanticAttributes.HTTP_FLAVOR.key}" { it == httpFlavor || it == "2.0" } // google https request can be http 2.0 + } else { + "${SemanticAttributes.HTTP_FLAVOR.key}" httpFlavor + } if (userAgent) { "${SemanticAttributes.HTTP_USER_AGENT.key}" { it.startsWith(userAgent) } } @@ -819,7 +828,7 @@ abstract class HttpClientTest extends InstrumentationSpecification { } if (extraAttributes.contains(SemanticAttributes.HTTP_HOST)) { - "${SemanticAttributes.HTTP_HOST}" "localhost:${uri.port}" + "${SemanticAttributes.HTTP_HOST}" { it == uri.host || it == "${uri.host}:${uri.port}" } } if (extraAttributes.contains(SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH)) { "${SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH}" Long @@ -828,7 +837,7 @@ abstract class HttpClientTest extends InstrumentationSpecification { "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH}" Long } if (extraAttributes.contains(SemanticAttributes.HTTP_SCHEME)) { - "${SemanticAttributes.HTTP_SCHEME}" "http" + "${SemanticAttributes.HTTP_SCHEME}" uri.scheme } if (extraAttributes.contains(SemanticAttributes.HTTP_TARGET)) { "${SemanticAttributes.HTTP_TARGET}" uri.path + "${uri.query != null ? "?${uri.query}" : ""}"