fix(tornado) ensure error status codes mark the span as an error (#2984) (#2989)

* fix(tornado) ensure error status codes mark the span as an error

We were only marking the tornado spans as an error if the request raised an exception.
While this is the typical way users will return an error we were not looking to see
if users were manually set the status code to 500 <= x < 600 and setting span.error.

This change has tornado use `set_http_meta` to ensure a consistent experience
across web server integrations.

* remove unused import

(cherry picked from commit 9f8b827)

Co-authored-by: Brett Langdon <[email protected]>

Author: mergify[bot]

ddtrace/contrib/tornado/handlers.py

@@ -6,9 +6,9 @@
 from ...constants import ANALYTICS_SAMPLE_RATE_KEY
 from ...constants import SPAN_MEASURED_KEY
 from ...ext import SpanTypes
-from ...ext import http
 from ...utils import ArgumentError
 from ...utils import get_argument_value
+from ..trace_utils import set_http_meta
 from .constants import CONFIG_KEY
 from .constants import REQUEST_SPAN_KEY
 from .stack_context import TracerStackContext
@@ -63,11 +63,14 @@ def on_finish(func, handler, args, kwargs):
         # space here
         klass = handler.__class__
         request_span.resource = "{}.{}".format(klass.__module__, klass.__name__)
-        request_span.set_tag("http.method", request.method)
-        request_span.set_tag("http.status_code", handler.get_status())
-        request_span.set_tag(http.URL, request.full_url().rsplit("?", 1)[0])
-        if config.tornado.trace_query_string:
-            request_span.set_tag(http.QUERY_STRING, request.query)
+        set_http_meta(
+            request_span,
+            config.tornado,
+            method=request.method,
+            url=request.full_url().rsplit("?", 1)[0],
+            status_code=handler.get_status(),
+            query=request.query,
+        )
         request_span.finish()
 
     return func(*args, **kwargs)

releasenotes/notes/fix-tornado-set-span-error-2e82f62e94e31791.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    Ensure tornado spans are marked as an error if the response status code is 500 <= x < 600.

tests/contrib/tornado/test_tornado_web.py

@@ -55,6 +55,27 @@ def test_success_handler_query_string_trace(self):
         with self.override_http_config("tornado", dict(trace_query_string=True)):
             self.test_success_handler("foo=bar")
 
+    def test_status_code_500_handler(self):
+        """
+        Test an endpoint which sets the status code to 500 but doesn't raise an exception
+
+        We expect the resulting span to be marked as an error
+        """
+        response = self.fetch("/status_code/500")
+        assert 500 == response.code
+
+        traces = self.pop_traces()
+        assert 1 == len(traces)
+        assert 1 == len(traces[0])
+
+        request_span = traces[0][0]
+        assert_is_measured(request_span)
+        assert "tests.contrib.tornado.web.app.ResponseStatusHandler" == request_span.resource
+        assert "GET" == request_span.get_tag("http.method")
+        assert_span_http_status_code(request_span, 500)
+        assert self.get_url("/status_code/500") == request_span.get_tag(http.URL)
+        assert 1 == request_span.error
+
     def test_nested_handler(self):
         # it should trace a handler that calls the tracer.trace() method
         # using the automatic Context retrieval

tests/contrib/tornado/web/app.py

@@ -19,6 +19,13 @@ def get(self):
         self.write("OK")
 
 
+class ResponseStatusHandler(tornado.web.RequestHandler):
+    @tornado.gen.coroutine
+    def get(self, status_code):
+        self.set_status(int(status_code))
+        self.write("status_code: {}".format(status_code))
+
+
 class NestedHandler(tornado.web.RequestHandler):
     @tornado.gen.coroutine
     def get(self):
@@ -307,6 +314,7 @@ def make_app(settings={}):
         [
             # custom handlers
             (r"/success/", SuccessHandler),
+            (r"/status_code/([0-9]+)", ResponseStatusHandler),
             (r"/nested/", NestedHandler),
             (r"/nested_wrap/", NestedWrapHandler),
             (r"/nested_exception_wrap/", NestedExceptionWrapHandler),