code review comments

open-telemetry · Apr 6, 2023 · 9280672 · 9280672
1 parent 546135c
commit 9280672
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 40 deletions.
diff --git a/.../java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientRequest.java b/.../java/io/opentelemetry/instrumentation/apachehttpclient/v4_3/ApacheHttpClientRequest.java
@@ -14,7 +14,6 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Locale;
 import java.util.logging.Logger;
 import javax.annotation.Nullable;
 import org.apache.http.Header;
@@ -77,7 +76,7 @@ String getUrl() {
   }
 
   String getProtocolName() {
-    return delegate.getProtocolVersion().getProtocol().toLowerCase(Locale.ROOT);
+    return delegate.getProtocolVersion().getProtocol();
   }
 
   String getProtocolVersion() {

diff --git a/...lemetry/javaagent/instrumentation/apachehttpclient/v5_0/AbstractApacheHttpClientTest.java b/...lemetry/javaagent/instrumentation/apachehttpclient/v5_0/AbstractApacheHttpClientTest.java
@@ -11,7 +11,6 @@
 import io.opentelemetry.instrumentation.testing.junit.http.AbstractHttpClientTest;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientResult;
 import io.opentelemetry.instrumentation.testing.junit.http.HttpClientTestOptions;
-import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.URI;
 import java.time.Duration;
 import java.util.HashSet;
@@ -37,17 +36,17 @@ protected void configure(HttpClientTestOptions.Builder optionsBuilder) {
     optionsBuilder.setHttpAttributes(this::getHttpAttributes);
   }
 
-  protected Set<AttributeKey<?>> getHttpAttributes(URI endpoint) {
-    Set<AttributeKey<?>> attributes = new HashSet<>();
-    attributes.add(SemanticAttributes.NET_PEER_NAME);
-    attributes.add(SemanticAttributes.NET_PEER_PORT);
-    attributes.add(SemanticAttributes.HTTP_URL);
-    attributes.add(SemanticAttributes.HTTP_METHOD);
-    if (endpoint.toString().contains("/success")) {
-      attributes.add(stringKey("net.protocol.name"));
-      attributes.add(stringKey("net.protocol.version"));
+  protected Set<AttributeKey<?>> getHttpAttributes(URI uri) {
+    Set<AttributeKey<?>> attributes = new HashSet<>(HttpClientTestOptions.DEFAULT_HTTP_ATTRIBUTES);
+    // unopened port or non routable address; or timeout
+    // circular redirects don't report protocol information as well
+    if ("http://localhost:61/".equals(uri.toString())
+        || "https://192.0.2.1/".equals(uri.toString())
+        || uri.toString().contains("/read-timeout")
+        || uri.toString().contains("/circular-redirect")) {
+      attributes.remove(stringKey("net.protocol.name"));
+      attributes.remove(stringKey("net.protocol.version"));
     }
-    attributes.add(SemanticAttributes.HTTP_USER_AGENT);
     return attributes;
   }
 

diff --git a/...ry/javaagent/instrumentation/asynchttpclient/v2_0/AsyncHttpClientNetAttributesGetter.java b/...ry/javaagent/instrumentation/asynchttpclient/v2_0/AsyncHttpClientNetAttributesGetter.java
@@ -5,12 +5,11 @@
 
 package io.opentelemetry.javaagent.instrumentation.asynchttpclient.v2_0;
 
-import io.netty.handler.codec.http.HttpMessage;
+import io.netty.handler.codec.http.HttpRequest;
 import io.netty.handler.codec.http.HttpVersion;
 import io.opentelemetry.instrumentation.api.instrumenter.net.InetSocketAddressNetClientAttributesGetter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.InetSocketAddress;
-import java.util.Optional;
 import javax.annotation.Nullable;
 import org.asynchttpclient.Response;
 import org.asynchttpclient.netty.request.NettyRequest;
@@ -26,23 +25,34 @@ public String getTransport(RequestContext request, @Nullable Response response)
   @Nullable
   @Override
   public String getProtocolName(RequestContext request, @Nullable Response response) {
-    return Optional.of(request)
-        .map(RequestContext::getNettyRequest)
-        .map(NettyRequest::getHttpRequest)
-        .map(HttpMessage::getProtocolVersion)
-        .map(HttpVersion::protocolName)
-        .orElse(null);
+    HttpVersion httpVersion = getHttpVersion(request);
+    if (httpVersion == null) {
+      return null;
+    }
+    return httpVersion.protocolName();
   }
 
   @Nullable
   @Override
   public String getProtocolVersion(RequestContext request, @Nullable Response response) {
-    return Optional.of(request)
-        .map(RequestContext::getNettyRequest)
-        .map(NettyRequest::getHttpRequest)
-        .map(HttpMessage::getProtocolVersion)
-        .map(p -> p.majorVersion() + "." + p.minorVersion())
-        .orElse(null);
+    HttpVersion httpVersion = getHttpVersion(request);
+    if (httpVersion == null) {
+      return null;
+    }
+    return httpVersion.majorVersion() + "." + httpVersion.minorVersion();
+  }
+
+  @Nullable
+  private HttpVersion getHttpVersion(RequestContext request) {
+    NettyRequest nettyRequest = request.getNettyRequest();
+    if (nettyRequest == null) {
+      return null;
+    }
+    HttpRequest httpRequest = nettyRequest.getHttpRequest();
+    if (httpRequest == null) {
+      return null;
+    }
+    return httpRequest.getProtocolVersion();
   }
 
   @Nullable

diff --git a/...rc/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AwsSdkNetAttributesGetter.java b/...rc/main/java/io/opentelemetry/instrumentation/awssdk/v1_11/AwsSdkNetAttributesGetter.java
@@ -10,7 +10,6 @@
 import com.amazonaws.http.HttpResponse;
 import io.opentelemetry.instrumentation.api.instrumenter.net.NetClientAttributesGetter;
 import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
-import java.util.Optional;
 import javax.annotation.Nullable;
 import org.apache.http.ProtocolVersion;
 import org.apache.http.client.methods.HttpRequestBase;
@@ -25,23 +24,37 @@ public String getTransport(Request<?> request, @Nullable Response<?> response) {
   @Nullable
   @Override
   public String getProtocolName(Request<?> request, @Nullable Response<?> response) {
-    return Optional.ofNullable(response)
-        .map(Response::getHttpResponse)
-        .map(HttpResponse::getHttpRequest)
-        .map(HttpRequestBase::getProtocolVersion)
-        .map(ProtocolVersion::getProtocol)
-        .orElse(null);
+    ProtocolVersion protocolVersion = getProtocolVersion(response);
+    if (protocolVersion == null) {
+      return null;
+    }
+    return protocolVersion.getProtocol();
   }
 
   @Nullable
   @Override
   public String getProtocolVersion(Request<?> request, @Nullable Response<?> response) {
-    return Optional.ofNullable(response)
-        .map(Response::getHttpResponse)
-        .map(HttpResponse::getHttpRequest)
-        .map(HttpRequestBase::getProtocolVersion)
-        .map(p -> p.getMajor() + "." + p.getMinor())
-        .orElse(null);
+    ProtocolVersion protocolVersion = getProtocolVersion(response);
+    if (protocolVersion == null) {
+      return null;
+    }
+    return protocolVersion.getMajor() + "." + protocolVersion.getMinor();
+  }
+
+  @Nullable
+  private ProtocolVersion getProtocolVersion(@Nullable Response<?> response) {
+    if (response == null) {
+      return null;
+    }
+    HttpResponse httpResponse = response.getHttpResponse();
+    if (httpResponse == null) {
+      return null;
+    }
+    HttpRequestBase httpRequest = httpResponse.getHttpRequest();
+    if (httpRequest == null) {
+      return null;
+    }
+    return httpRequest.getProtocolVersion();
   }
 
   @Override