Don't use HttpStatusConverter outside of instrumentation-api (#4870)

* Don't use HttpStatusConverter outside of instrumentation-api * add comment
open-telemetry · Dec 14, 2021 · 331ce28 · 331ce28
1 parent ce257b7
commit 331ce28
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 19 deletions.
diff --git a/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java b/...io/opentelemetry/instrumentation/api/instrumenter/http/HttpCommonAttributesExtractor.java
@@ -61,7 +61,7 @@ public void onEnd(
 
     if (response != null) {
       Integer statusCode = statusCode(request, response);
-      if (statusCode != null) {
+      if (statusCode != null && statusCode > 0) {
         set(attributes, SemanticAttributes.HTTP_STATUS_CODE, (long) statusCode);
       }
       set(

diff --git a/...pentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java b/...pentelemetry/instrumentation/api/instrumenter/http/HttpClientAttributesExtractorTest.java
@@ -7,6 +7,7 @@
 
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
 import static java.util.Arrays.asList;
+import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.entry;
 
@@ -40,18 +41,21 @@ protected String url(Map<String, String> request) {
 
     @Override
     protected List<String> requestHeader(Map<String, String> request, String name) {
-      return asList(request.get("header." + name).split(","));
+      String value = request.get("header." + name);
+      return value == null ? emptyList() : asList(value.split(","));
     }
 
     @Override
     protected Long requestContentLength(Map<String, String> request, Map<String, String> response) {
-      return Long.parseLong(request.get("requestContentLength"));
+      String value = request.get("requestContentLength");
+      return value == null ? null : Long.parseLong(value);
     }
 
     @Override
     protected Long requestContentLengthUncompressed(
         Map<String, String> request, Map<String, String> response) {
-      return Long.parseLong(request.get("requestContentLengthUncompressed"));
+      String value = request.get("requestContentLengthUncompressed");
+      return value == null ? null : Long.parseLong(value);
     }
 
     @Override
@@ -67,19 +71,22 @@ protected String flavor(Map<String, String> request, Map<String, String> respons
     @Override
     protected Long responseContentLength(
         Map<String, String> request, Map<String, String> response) {
-      return Long.parseLong(response.get("responseContentLength"));
+      String value = response.get("responseContentLength");
+      return value == null ? null : Long.parseLong(value);
     }
 
     @Override
     protected Long responseContentLengthUncompressed(
         Map<String, String> request, Map<String, String> response) {
-      return Long.parseLong(response.get("responseContentLengthUncompressed"));
+      String value = response.get("responseContentLengthUncompressed");
+      return value == null ? null : Long.parseLong(value);
     }
 
     @Override
     protected List<String> responseHeader(
         Map<String, String> request, Map<String, String> response, String name) {
-      return asList(response.get("header." + name).split(","));
+      String value = response.get("header." + name);
+      return value == null ? emptyList() : asList(value.split(","));
     }
   }
 
@@ -135,4 +142,22 @@ void normal() {
                 AttributeKey.stringArrayKey("http.response.header.custom_response_header"),
                 asList("654", "321")));
   }
+
+  @Test
+  void invalidStatusCode() {
+    Map<String, String> request = new HashMap<>();
+
+    Map<String, String> response = new HashMap<>();
+    response.put("statusCode", "0");
+
+    TestHttpClientAttributesExtractor extractor =
+        new TestHttpClientAttributesExtractor(CapturedHttpHeaders.empty());
+
+    AttributesBuilder attributes = Attributes.builder();
+    extractor.onStart(attributes, request);
+    assertThat(attributes.build()).isEmpty();
+
+    extractor.onEnd(attributes, request, response, null);
+    assertThat(attributes.build()).isEmpty();
+  }
 }
diff --git a/...lemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentation.java b/...lemetry/javaagent/instrumentation/httpurlconnection/HttpUrlConnectionInstrumentation.java
@@ -15,17 +15,12 @@
 import static net.bytebuddy.matcher.ElementMatchers.namedOneOf;
 import static net.bytebuddy.matcher.ElementMatchers.not;
 
-import io.opentelemetry.api.trace.Span;
-import io.opentelemetry.api.trace.StatusCode;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.instrumentation.api.field.VirtualField;
-import io.opentelemetry.instrumentation.api.tracer.HttpStatusConverter;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import io.opentelemetry.javaagent.instrumentation.api.CallDepth;
-import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;
-import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import java.net.HttpURLConnection;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.type.TypeDescription;
@@ -137,7 +132,7 @@ public static void methodExit(
                 .end(
                     httpUrlState.context,
                     connection,
-                    responseCode > 0 ? responseCode : null,
+                    responseCode > 0 ? responseCode : httpUrlState.statusCode,
                     throwable);
           }
           httpUrlState.finished = true;
@@ -167,12 +162,7 @@ public static void methodExit(
           VirtualField.find(HttpURLConnection.class, HttpUrlState.class);
       HttpUrlState httpUrlState = storage.get(connection);
       if (httpUrlState != null) {
-        Span span = Java8BytecodeBridge.spanFromContext(httpUrlState.context);
-        span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, returnValue);
-        StatusCode statusCode = HttpStatusConverter.CLIENT.statusFromHttpStatus(returnValue);
-        if (statusCode != StatusCode.UNSET) {
-          span.setStatus(statusCode);
-        }
+        httpUrlState.statusCode = returnValue;
       }
     }
   }

diff --git a/.../main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlState.java b/.../main/java/io/opentelemetry/javaagent/instrumentation/httpurlconnection/HttpUrlState.java
@@ -12,6 +12,8 @@
 public class HttpUrlState {
   public final Context context;
   public boolean finished;
+  // by default 0 is ignored
+  public int statusCode = 0;
 
   public HttpUrlState(Context context) {
     this.context = context;