Don't set STATUS on SpanKind SERVER for 4XX status (open-telemetry#776)

nicholasgribanov · Oct 29, 2021 · 581f2fe · 581f2fe
1 parent dc6ce53
commit 581f2fe
Show file tree

Hide file tree

Showing 8 changed files with 56 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   ([#774](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/774))
 - `opentelemetry-instrumentation-django` Fixed carrier usage on ASGI requests.
   ([#767](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/767))
+- Don't set Span Status on 4xx http status code for SpanKind.SERVER spans
+  ([#776](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/776))
 
 ## [1.6.2-0.25b2](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.6.2-0.25b2) - 2021-10-19
 

diff --git a/...ion/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py b/...ion/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
@@ -218,7 +218,9 @@ def set_status_code(span, status_code):
         )
     else:
         span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
-        span.set_status(Status(http_status_to_status_code(status_code)))
+        span.set_status(
+            Status(http_status_to_status_code(status_code, server_span=True))
+        )
 
 
 def get_default_span_details(scope: dict) -> Tuple[str, dict]:

diff --git a/...opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py b/...opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py
@@ -299,7 +299,9 @@ def process_response(
             span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
             span.set_status(
                 Status(
-                    status_code=http_status_to_status_code(status_code),
+                    status_code=http_status_to_status_code(
+                        status_code, server_span=True
+                    ),
                     description=reason,
                 )
             )

diff --git a/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py b/instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py
@@ -114,10 +114,7 @@ def test_404(self):
         self.assertEqual(len(spans), 1)
         span = spans[0]
         self.assertEqual(span.name, "HTTP GET")
-        self.assertEqual(span.status.status_code, StatusCode.ERROR)
-        self.assertEqual(
-            span.status.description, "NotFound",
-        )
+        self.assertEqual(span.status.status_code, StatusCode.UNSET)
         self.assertSpanHasAttributes(
             span,
             {

diff --git a/...entelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py b/...entelemetry-instrumentation-tornado/src/opentelemetry/instrumentation/tornado/__init__.py
@@ -345,7 +345,9 @@ def _finish_span(tracer, handler, error=None):
 
     if ctx.span.is_recording():
         ctx.span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
-        otel_status_code = http_status_to_status_code(status_code)
+        otel_status_code = http_status_to_status_code(
+            status_code, server_span=True
+        )
         otel_status_description = None
         if otel_status_code is StatusCode.ERROR:
             otel_status_description = reason

diff --git a/...ion/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py b/...ion/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py
@@ -226,7 +226,9 @@ def add_response_attributes(
         )
     else:
         span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
-        span.set_status(Status(http_status_to_status_code(status_code)))
+        span.set_status(
+            Status(http_status_to_status_code(status_code, server_span=True))
+        )
 
 
 def get_default_span_name(environ):

diff --git a/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py b/opentelemetry-instrumentation/src/opentelemetry/instrumentation/utils.py
@@ -36,7 +36,7 @@ def extract_attributes_from_object(
 
 
 def http_status_to_status_code(
-    status: int, allow_redirect: bool = True
+    status: int, allow_redirect: bool = True, server_span: bool = False,
 ) -> StatusCode:
     """Converts an HTTP status code to an OpenTelemetry canonical status code
 
@@ -50,6 +50,8 @@ def http_status_to_status_code(
         return StatusCode.UNSET
     if status <= 399 and allow_redirect:
         return StatusCode.UNSET
+    if status <= 499 and server_span:
+        return StatusCode.UNSET
     return StatusCode.ERROR
 
 

diff --git a/opentelemetry-instrumentation/tests/test_utils.py b/opentelemetry-instrumentation/tests/test_utils.py
@@ -43,3 +43,41 @@ def test_http_status_to_status_code(self):
             with self.subTest(status_code=status_code):
                 actual = http_status_to_status_code(int(status_code))
                 self.assertEqual(actual, expected, status_code)
+
+    def test_http_status_to_status_code_redirect(self):
+        for status_code, expected in (
+            (HTTPStatus.MULTIPLE_CHOICES, StatusCode.ERROR),
+            (HTTPStatus.MOVED_PERMANENTLY, StatusCode.ERROR),
+            (HTTPStatus.TEMPORARY_REDIRECT, StatusCode.ERROR),
+            (HTTPStatus.PERMANENT_REDIRECT, StatusCode.ERROR),
+        ):
+            with self.subTest(status_code=status_code):
+                actual = http_status_to_status_code(
+                    int(status_code), allow_redirect=False
+                )
+                self.assertEqual(actual, expected, status_code)
+
+    def test_http_status_to_status_code_server(self):
+        for status_code, expected in (
+            (HTTPStatus.OK, StatusCode.UNSET),
+            (HTTPStatus.ACCEPTED, StatusCode.UNSET),
+            (HTTPStatus.IM_USED, StatusCode.UNSET),
+            (HTTPStatus.MULTIPLE_CHOICES, StatusCode.UNSET),
+            (HTTPStatus.BAD_REQUEST, StatusCode.UNSET),
+            (HTTPStatus.UNAUTHORIZED, StatusCode.UNSET),
+            (HTTPStatus.FORBIDDEN, StatusCode.UNSET),
+            (HTTPStatus.NOT_FOUND, StatusCode.UNSET),
+            (HTTPStatus.UNPROCESSABLE_ENTITY, StatusCode.UNSET,),
+            (HTTPStatus.TOO_MANY_REQUESTS, StatusCode.UNSET,),
+            (HTTPStatus.NOT_IMPLEMENTED, StatusCode.ERROR),
+            (HTTPStatus.SERVICE_UNAVAILABLE, StatusCode.ERROR),
+            (HTTPStatus.GATEWAY_TIMEOUT, StatusCode.ERROR,),
+            (HTTPStatus.HTTP_VERSION_NOT_SUPPORTED, StatusCode.ERROR,),
+            (600, StatusCode.ERROR),
+            (99, StatusCode.ERROR),
+        ):
+            with self.subTest(status_code=status_code):
+                actual = http_status_to_status_code(
+                    int(status_code), server_span=True
+                )
+                self.assertEqual(actual, expected, status_code)