Don't report 400 level as error for SERVER spans (#4403)

* don't report 400 level as error for server spans * fix HttpServerTest base class * fix JspInstrumentationForward test * split HttpStatusConverter into client and server implementations, and create two HttpSpanStatusExtractor.create methods, one for server and one for client. * rebase * fix test * spotless * fix test * remove unused * use strongly typed attributes converters and rename to overloaded create() * fix tests * remove redundant assert Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
open-telemetry · Oct 20, 2021 · a50c133 · a50c133
1 parent 5d7c0dd
commit a50c133
Show file tree

Hide file tree

Showing 21 changed files with 315 additions and 63 deletions.
diff --git a/...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java
@@ -183,11 +183,10 @@ public void end(
       span.recordException(error);
     }
 
-    UnsafeAttributes attributesBuilder = new UnsafeAttributes();
+    UnsafeAttributes attributes = new UnsafeAttributes();
     for (AttributesExtractor<? super REQUEST, ? super RESPONSE> extractor : attributesExtractors) {
-      extractor.onEnd(attributesBuilder, request, response, error);
+      extractor.onEnd(attributes, request, response, error);
     }
-    Attributes attributes = attributesBuilder;
     span.setAllAttributes(attributes);
 
     Instant endTime = null;

diff --git a/.../java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java b/.../java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanStatusExtractor.java
@@ -19,30 +19,44 @@
 public final class HttpSpanStatusExtractor<REQUEST, RESPONSE>
     implements SpanStatusExtractor<REQUEST, RESPONSE> {
 
+  private final HttpStatusConverter statusConverter;
+
+  /**
+   * Returns the {@link SpanStatusExtractor} for HTTP requests, which will use the HTTP status code
+   * to determine the {@link StatusCode} if available or fallback to {@linkplain #getDefault() the
+   * default status} otherwise.
+   */
+  public static <REQUEST, RESPONSE> SpanStatusExtractor<REQUEST, RESPONSE> create(
+      HttpClientAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor) {
+    return new HttpSpanStatusExtractor<>(attributesExtractor, HttpStatusConverter.CLIENT);
+  }
+
   /**
    * Returns the {@link SpanStatusExtractor} for HTTP requests, which will use the HTTP status code
    * to determine the {@link StatusCode} if available or fallback to {@linkplain #getDefault() the
    * default status} otherwise.
    */
   public static <REQUEST, RESPONSE> SpanStatusExtractor<REQUEST, RESPONSE> create(
-      HttpCommonAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor) {
-    return new HttpSpanStatusExtractor<>(attributesExtractor);
+      HttpServerAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor) {
+    return new HttpSpanStatusExtractor<>(attributesExtractor, HttpStatusConverter.SERVER);
   }
 
   private final HttpCommonAttributesExtractor<? super REQUEST, ? super RESPONSE>
       attributesExtractor;
 
   private HttpSpanStatusExtractor(
-      HttpCommonAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor) {
+      HttpCommonAttributesExtractor<? super REQUEST, ? super RESPONSE> attributesExtractor,
+      HttpStatusConverter statusConverter) {
     this.attributesExtractor = attributesExtractor;
+    this.statusConverter = statusConverter;
   }
 
   @Override
   public StatusCode extract(REQUEST request, @Nullable RESPONSE response, Throwable error) {
     if (response != null) {
       Integer statusCode = attributesExtractor.statusCode(request, response);
       if (statusCode != null) {
-        StatusCode statusCodeObj = HttpStatusConverter.statusFromHttpStatus(statusCode);
+        StatusCode statusCodeObj = statusConverter.statusFromHttpStatus(statusCode);
         if (statusCodeObj == StatusCode.ERROR) {
           return statusCodeObj;
         }

diff --git a/.../src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientStatusConverter.java b/.../src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientStatusConverter.java
@@ -0,0 +1,25 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.tracer;
+
+import io.opentelemetry.api.trace.StatusCode;
+
+final class HttpClientStatusConverter implements HttpStatusConverter {
+
+  static final HttpStatusConverter INSTANCE = new HttpClientStatusConverter();
+
+  // https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status
+  @Override
+  public StatusCode statusFromHttpStatus(int httpStatus) {
+    if (httpStatus >= 100 && httpStatus < 400) {
+      return StatusCode.UNSET;
+    }
+
+    return StatusCode.ERROR;
+  }
+
+  private HttpClientStatusConverter() {}
+}
diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java
@@ -211,7 +211,7 @@ protected void onResponse(Span span, RESPONSE response) {
       Integer status = status(response);
       if (status != null) {
         span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, (long) status);
-        StatusCode statusCode = HttpStatusConverter.statusFromHttpStatus(status);
+        StatusCode statusCode = HttpStatusConverter.CLIENT.statusFromHttpStatus(status);
         if (statusCode != StatusCode.UNSET) {
           span.setStatus(statusCode);
         }

diff --git a/.../src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerStatusConverter.java b/.../src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerStatusConverter.java
@@ -0,0 +1,25 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.tracer;
+
+import io.opentelemetry.api.trace.StatusCode;
+
+final class HttpServerStatusConverter implements HttpStatusConverter {
+
+  static final HttpStatusConverter INSTANCE = new HttpServerStatusConverter();
+
+  // https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status
+  @Override
+  public StatusCode statusFromHttpStatus(int httpStatus) {
+    if (httpStatus >= 100 && httpStatus < 500) {
+      return StatusCode.UNSET;
+    }
+
+    return StatusCode.ERROR;
+  }
+
+  private HttpServerStatusConverter() {}
+}
diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpServerTracer.java
@@ -276,7 +276,7 @@ private static String extractIpAddress(String forwarded, int start) {
 
   private static void setStatus(Span span, int status) {
     span.setAttribute(SemanticAttributes.HTTP_STATUS_CODE, (long) status);
-    StatusCode statusCode = HttpStatusConverter.statusFromHttpStatus(status);
+    StatusCode statusCode = HttpStatusConverter.SERVER.statusFromHttpStatus(status);
     if (statusCode != StatusCode.UNSET) {
       span.setStatus(statusCode);
     }

diff --git a/...on-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpStatusConverter.java b/...on-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpStatusConverter.java
@@ -7,16 +7,10 @@
 
 import io.opentelemetry.api.trace.StatusCode;
 
-public final class HttpStatusConverter {
+public interface HttpStatusConverter {
 
-  // https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status
-  public static StatusCode statusFromHttpStatus(int httpStatus) {
-    if (httpStatus >= 100 && httpStatus < 400) {
-      return StatusCode.UNSET;
-    }
+  HttpStatusConverter SERVER = HttpServerStatusConverter.INSTANCE;
+  HttpStatusConverter CLIENT = HttpClientStatusConverter.INSTANCE;
 
-    return StatusCode.ERROR;
-  }
-
-  private HttpStatusConverter() {}
+  StatusCode statusFromHttpStatus(int httpStatus);
 }
diff --git a/...api/tracer/HttpStatusConverterTest.groovy → ...acer/HttpClientStatusConverterTest.groovy b/...api/tracer/HttpStatusConverterTest.groovy → ...acer/HttpClientStatusConverterTest.groovy
@@ -8,11 +8,11 @@ package io.opentelemetry.instrumentation.api.tracer
 import io.opentelemetry.api.trace.StatusCode
 import spock.lang.Specification
 
-class HttpStatusConverterTest extends Specification {
+class HttpClientStatusConverterTest extends Specification {
 
   def "test HTTP #httpStatus to OTel #expectedStatus"() {
     when:
-    def status = HttpStatusConverter.statusFromHttpStatus(httpStatus)
+    def status = HttpStatusConverter.CLIENT.statusFromHttpStatus(httpStatus)
 
     then:
     status == expectedStatus

diff --git a/...i/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy b/...i/src/test/groovy/io/opentelemetry/instrumentation/api/tracer/HttpClientTracerTest.groovy
@@ -111,7 +111,7 @@ class HttpClientTracerTest extends BaseTracerTest {
   def "test onResponse"() {
     setup:
     def tracer = newTracer()
-    def statusCode = status != null ? HttpStatusConverter.statusFromHttpStatus(status) : null
+    def statusCode = status != null ? HttpStatusConverter.CLIENT.statusFromHttpStatus(status) : null
 
     when:
     tracer.onResponse(span, resp)

diff --git a/...t/groovy/io/opentelemetry/instrumentation/api/tracer/HttpServerStatusConverterTest.groovy b/...t/groovy/io/opentelemetry/instrumentation/api/tracer/HttpServerStatusConverterTest.groovy
@@ -0,0 +1,94 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.tracer
+
+import io.opentelemetry.api.trace.StatusCode
+import spock.lang.Specification
+
+class HttpServerStatusConverterTest extends Specification {
+
+  def "test HTTP #httpStatus to OTel #expectedStatus"() {
+    when:
+    def status = HttpStatusConverter.SERVER.statusFromHttpStatus(httpStatus)
+
+    then:
+    status == expectedStatus
+
+    // https://en.wikipedia.org/wiki/List_of_HTTP_status_codes
+    where:
+    httpStatus | expectedStatus
+    100        | StatusCode.UNSET
+    101        | StatusCode.UNSET
+    102        | StatusCode.UNSET
+    103        | StatusCode.UNSET
+
+    200        | StatusCode.UNSET
+    201        | StatusCode.UNSET
+    202        | StatusCode.UNSET
+    203        | StatusCode.UNSET
+    204        | StatusCode.UNSET
+    205        | StatusCode.UNSET
+    206        | StatusCode.UNSET
+    207        | StatusCode.UNSET
+    208        | StatusCode.UNSET
+    226        | StatusCode.UNSET
+
+    300        | StatusCode.UNSET
+    301        | StatusCode.UNSET
+    302        | StatusCode.UNSET
+    303        | StatusCode.UNSET
+    304        | StatusCode.UNSET
+    305        | StatusCode.UNSET
+    306        | StatusCode.UNSET
+    307        | StatusCode.UNSET
+    308        | StatusCode.UNSET
+
+    400        | StatusCode.UNSET
+    401        | StatusCode.UNSET
+    403        | StatusCode.UNSET
+    404        | StatusCode.UNSET
+    405        | StatusCode.UNSET
+    406        | StatusCode.UNSET
+    407        | StatusCode.UNSET
+    408        | StatusCode.UNSET
+    409        | StatusCode.UNSET
+    410        | StatusCode.UNSET
+    411        | StatusCode.UNSET
+    412        | StatusCode.UNSET
+    413        | StatusCode.UNSET
+    414        | StatusCode.UNSET
+    415        | StatusCode.UNSET
+    416        | StatusCode.UNSET
+    417        | StatusCode.UNSET
+    418        | StatusCode.UNSET
+    421        | StatusCode.UNSET
+    422        | StatusCode.UNSET
+    423        | StatusCode.UNSET
+    424        | StatusCode.UNSET
+    425        | StatusCode.UNSET
+    426        | StatusCode.UNSET
+    428        | StatusCode.UNSET
+    429        | StatusCode.UNSET
+    431        | StatusCode.UNSET
+    451        | StatusCode.UNSET
+
+    500        | StatusCode.ERROR
+    501        | StatusCode.ERROR
+    502        | StatusCode.ERROR
+    503        | StatusCode.ERROR
+    504        | StatusCode.ERROR
+    505        | StatusCode.ERROR
+    506        | StatusCode.ERROR
+    507        | StatusCode.ERROR
+    508        | StatusCode.ERROR
+    510        | StatusCode.ERROR
+    511        | StatusCode.ERROR
+
+    // Don't exist
+    99         | StatusCode.ERROR
+    600        | StatusCode.ERROR
+  }
+}
diff --git a/...pentelemetry/instrumentation/api/instrumenter/http/HttpClientSpanStatusExtractorTest.java b/...pentelemetry/instrumentation/api/instrumenter/http/HttpClientSpanStatusExtractorTest.java
@@ -0,0 +1,70 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter.http;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.anyMap;
+import static org.mockito.Mockito.when;
+
+import io.opentelemetry.api.trace.StatusCode;
+import io.opentelemetry.instrumentation.api.tracer.HttpStatusConverter;
+import java.util.Collections;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+@ExtendWith(MockitoExtension.class)
+class HttpClientSpanStatusExtractorTest {
+  @Mock private HttpClientAttributesExtractor<Map<String, String>, Map<String, String>> extractor;
+
+  @ParameterizedTest
+  @ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
+  void hasStatus(int statusCode) {
+    when(extractor.statusCode(anyMap(), anyMap())).thenReturn(statusCode);
+    assertThat(
+            HttpSpanStatusExtractor.create(extractor)
+                .extract(Collections.emptyMap(), Collections.emptyMap(), null))
+        .isEqualTo(HttpStatusConverter.CLIENT.statusFromHttpStatus(statusCode));
+  }
+
+  @ParameterizedTest
+  @ValueSource(ints = {1, 100, 101, 200, 201, 300, 301, 400, 401, 500, 501, 600, 601})
+  void hasStatusAndException(int statusCode) {
+    when(extractor.statusCode(anyMap(), anyMap())).thenReturn(statusCode);
+
+    // Presence of exception has no effect.
+    assertThat(
+            HttpSpanStatusExtractor.create(extractor)
+                .extract(
+                    Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
+        .isEqualTo(StatusCode.ERROR);
+  }
+
+  @Test
+  void hasNoStatus_fallsBackToDefault_unset() {
+    when(extractor.statusCode(anyMap(), anyMap())).thenReturn(null);
+
+    assertThat(
+            HttpSpanStatusExtractor.create(extractor)
+                .extract(Collections.emptyMap(), Collections.emptyMap(), null))
+        .isEqualTo(StatusCode.UNSET);
+  }
+
+  @Test
+  void hasNoStatus_fallsBackToDefault_error() {
+    when(extractor.statusCode(anyMap(), anyMap())).thenReturn(null);
+
+    assertThat(
+            HttpSpanStatusExtractor.create(extractor)
+                .extract(
+                    Collections.emptyMap(), Collections.emptyMap(), new IllegalStateException()))
+        .isEqualTo(StatusCode.ERROR);
+  }
+}