Repository: scoutapp/scout_apm_python
Pull request #386 (2019-11-23 11:47:49)

Tag Falcon 5XX responses as errors
Ref #383.

Also, since Falcon allows directly setting the status line, test the case where a bad one is set, and report that as an error too.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1777a91..176ec21 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,11 @@
 - Add RQ integration
   ([PR #375](https://github.com/scoutapp/scout_apm_python/pull/375)).
 
+### Fixed
+
+- Track better names for Django REST Framework `ViewSet`s
+  ([PR #380](https://github.com/scoutapp/scout_apm_python/pull/380)).
+
 ## [2.8.1] 2019-11-19
 
 ### Fixed
diff --git a/src/scout_apm/bottle.py b/src/scout_apm/bottle.py
index 2b92462..44e73b8 100644
--- a/src/scout_apm/bottle.py
+++ b/src/scout_apm/bottle.py
@@ -2,7 +2,7 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 
 import wrapt
-from bottle import request
+from bottle import request, response
 
 import scout_apm.core
 from scout_apm.core.config import scout_config
@@ -82,9 +82,13 @@ def wrap_callback(wrapped, instance, args, kwargs):
         track_amazon_request_queue_time(amazon_queue_time, tracked_request)
 
     try:
-        return wrapped(*args, **kwargs)
+        value = wrapped(*args, **kwargs)
     except Exception:
         tracked_request.tag("error", "true")
         raise
+    else:
+        if 500 <= response.status_code <= 599:
+            tracked_request.tag("error", "true")
+        return value
     finally:
         tracked_request.stop_span()
diff --git a/src/scout_apm/django/middleware.py b/src/scout_apm/django/middleware.py
index 02d25f5..88b1166 100644
--- a/src/scout_apm/django/middleware.py
+++ b/src/scout_apm/django/middleware.py
@@ -37,6 +37,12 @@ def get_operation_name(request):
             + view_func.__name__
         )
 
+    django_rest_framework_name = _get_django_rest_framework_name(
+        request, view_func, view_name
+    )
+    if django_rest_framework_name is not None:
+        return django_rest_framework_name
+
     # Seems to be a Tastypie Resource. Need to resort to some stack inspection
     # to find a better name since its decorators don't wrap very well
     if view_name == "tastypie.resources.wrapper":
@@ -47,6 +53,28 @@ def get_operation_name(request):
     return "Controller/" + view_name
 
 
+def _get_django_rest_framework_name(request, view_func, view_name):
+    try:
+        from rest_framework.viewsets import ViewSetMixin
+    except ImportError:
+        return None
+
+    kls = getattr(view_func, "cls", None)
+    if isinstance(kls, type) and not issubclass(kls, ViewSetMixin):
+        return None
+
+    # Get 'actions' set in ViewSetMixin.as_view
+    actions = getattr(view_func, "actions", None)
+    if not actions or not isinstance(actions, dict):
+        return None
+
+    method_lower = request.method.lower()
+    if method_lower not in actions:
+        return None
+
+    return "Controller/{}.{}".format(view_name, actions[method_lower])
+
+
 def _get_tastypie_operation_name(request, view_func):
     try:
         from tastypie.resources import Resource
diff --git a/src/scout_apm/falcon.py b/src/scout_apm/falcon.py
index 4340906..fe8c97b 100644
--- a/src/scout_apm/falcon.py
+++ b/src/scout_apm/falcon.py
@@ -107,7 +107,15 @@ class ScoutMiddleware(object):
             # Somehow we didn't start a request
             return
 
-        if not req_succeeded:
+        # Falcon only stores the response status line, we have to parse it
+        try:
+            status_code = int(resp.status.split(" ")[0])
+        except ValueError:
+            # Bad status line - force it to be tagged as an error because
+            # client will experience it as one
+            status_code = 500
+
+        if not req_succeeded or 500 <= status_code <= 599:
             tracked_request.tag("error", "true")
 
         span = getattr(req.context, "scout_resource_span", None)
diff --git a/tox.ini b/tox.ini
index f9bcc4a..1357bab 100644
--- a/tox.ini
+++ b/tox.ini
@@ -28,14 +28,20 @@ deps =
     bottle
     celery
     django18: Django>=1.8,<1.9
+    django18: djangorestframework>=3.6,<3.7  # https://www.django-rest-framework.org/community/release-notes/
     django111: Django>=1.11,<2.0
+    django111: djangorestframework
     django111: django-tastypie
     django20: Django>=2.0,<2.1
+    django20: djangorestframework
     django20: django-tastypie
     django21: Django>=2.1,<2.2
+    django21: djangorestframework
     django21: django-tastypie
     django22: Django>=2.2,<2.3
+    django22: djangorestframework
     django30: Django>=3.0a1,<3.1
+    django30: djangorestframework
     dramatiq>=1.0.0 ; python_version >= "3.5"
     elasticsearch
     fakeredis


Test patch:
diff --git a/tests/integration/django_app.py b/tests/integration/django_app.py
index 577db62..ebfcf80 100644
--- a/tests/integration/django_app.py
+++ b/tests/integration/django_app.py
@@ -154,6 +154,30 @@ def template_response(request):
     return TemplateResponse(request, template, context)
 
 
+@SimpleLazyObject
+def drf_router():
+    """
+    DRF Router as a lazy object because it needs to import User model which
+    can't be done until after django.setup()
+    """
+    from django.contrib.auth.models import User
+    from rest_framework import routers
+    from rest_framework import serializers
+    from rest_framework import viewsets
+
+    class UserSerializer(serializers.Serializer):
+        id = serializers.IntegerField(label="ID", read_only=True)
+        username = serializers.CharField(max_length=200)
+
+    class UserViewSet(viewsets.ModelViewSet):
+        queryset = User.objects.all()
+        serializer_class = UserSerializer
+
+    router = routers.SimpleRouter()
+    router.register(r"users", UserViewSet)
+    return router
+
+
 @SimpleLazyObject
 def tastypie_api():
     """
@@ -199,6 +223,7 @@ def urlpatterns():
             path("template/", template),
             path("template-response/", template_response),
             path("admin/", admin.site.urls),
+            path("drf-router/", include(drf_router.urls)),
         ]
         if tastypie_api:
             patterns.append(path("tastypie-api/", include(tastypie_api.urls)))
@@ -219,6 +244,7 @@ def urlpatterns():
             url(r"^template/$", template),
             url(r"^template-response/$", template_response),
             url(r"^admin/", admin.site.urls),
+            url(r"^drf-router/", include(drf_router.urls)),
         ]
         if tastypie_api:
             patterns.append(url(r"^tastypie-api/", include(tastypie_api.urls)))
diff --git a/tests/integration/test_bottle.py b/tests/integration/test_bottle.py
index 5aa329a..c44579d 100644
--- a/tests/integration/test_bottle.py
+++ b/tests/integration/test_bottle.py
@@ -4,7 +4,7 @@ from __future__ import absolute_import, division, print_function, unicode_litera
 import datetime as dt
 from contextlib import contextmanager
 
-from bottle import Bottle
+from bottle import Bottle, response
 from webtest import TestApp
 
 from scout_apm.api import Config
@@ -45,6 +45,11 @@ def app_with_scout(config=None, catchall=False):
     def crash():
         raise ValueError("BØØM!")  # non-ASCII
 
+    @app.route("/return-error/")
+    def return_error():
+        response.status = 503
+        return "Something went wrong"
+
     @app.route("/named/", name="named_route")
     def named():
         return "Response from a named route."
@@ -181,6 +186,19 @@ def test_server_error(tracked_requests):
     assert span.operation == "Controller/crash/"
 
 
+def test_return_error(tracked_requests):
+    with app_with_scout() as app:
+        response = TestApp(app).get("/return-error/", expect_errors=True)
+
+    assert response.status_int == 503
+    assert len(tracked_requests) == 1
+    tracked_request = tracked_requests[0]
+    assert tracked_request.tags["error"] == "true"
+    assert len(tracked_request.complete_spans) == 1
+    span = tracked_requests[0].complete_spans[0]
+    assert span.operation == "Controller/return-error/"
+
+
 def test_named(tracked_requests):
     with app_with_scout() as app:
         response = TestApp(app).get("/named/")
diff --git a/tests/integration/test_django.py b/tests/integration/test_django.py
index 10cef4a..bc96d1e 100644
--- a/tests/integration/test_django.py
+++ b/tests/integration/test_django.py
@@ -387,7 +387,40 @@ def test_admin_view_operation_name(url, expected_op_name, tracked_requests):
     assert span.operation == expected_op_name
 
 
+@pytest.mark.parametrize(
+    "url, expected_op_name",
+    [
+        [
+            "/drf-router/users/",
+            "Controller/tests.integration.django_app.UserViewSet.list",
+        ],
+        [
+            "/drf-router/users/1/",
+            "Controller/tests.integration.django_app.UserViewSet.retrieve",
+        ],
+    ],
+)
+def test_django_rest_framework_api_operation_name(
+    url, expected_op_name, tracked_requests
+):
+    with app_with_scout() as app:
+        make_admin_user()
+        response = TestApp(app).get(url)
+
+    assert response.status_int == 200
+    assert len(tracked_requests) == 1
+    spans = tracked_requests[0].complete_spans
+    assert [s.operation for s in spans] == [
+        "SQL/Query",
+        expected_op_name,
+        "Middleware",
+    ]
+
+
 def skip_if_no_tastypie():
+    # This would make more sense as a test decorator, but can't be one because
+    # it requires the Django application to be constructed first, under
+    # app_with_scout()
     if not django_app.tastypie_api:
         pytest.skip("No Tastypie")
 
diff --git a/tests/integration/test_falcon.py b/tests/integration/test_falcon.py
index 6efcd8e..2fcb50a 100644
--- a/tests/integration/test_falcon.py
+++ b/tests/integration/test_falcon.py
@@ -67,6 +67,19 @@ def app_with_scout(config=None, middleware=None, set_api=True):
 
     app.add_route("/error", ErrorResource())
 
+    class ReturnErrorResource(object):
+        def on_get(self, req, resp):
+            resp.status = "503 Something went wrong"
+            resp.body = "Something went wrong"
+
+    app.add_route("/return-error", ReturnErrorResource())
+
+    class BadStatusResource(object):
+        def on_get(self, req, resp):
+            resp.status = "bad"
+
+    app.add_route("/bad-status", BadStatusResource())
+
     try:
         yield app
     finally:
@@ -345,3 +358,40 @@ def test_error(tracked_requests):
         span.operation
         == "Controller/tests.integration.test_falcon.ErrorResource.on_get"
     )
+
+
+def test_return_error(tracked_requests):
+    with app_with_scout() as app:
+        response = TestApp(app).get("/return-error", expect_errors=True)
+
+    assert response.status_int == 503
+    assert response.text == "Something went wrong"
+    assert len(tracked_requests) == 1
+    tracked_request = tracked_requests[0]
+    assert tracked_request.tags["path"] == "/return-error"
+    assert tracked_request.tags["error"] == "true"
+    assert tracked_request.active_spans == []
+    assert len(tracked_request.complete_spans) == 1
+    span = tracked_request.complete_spans[0]
+    assert (
+        span.operation
+        == "Controller/tests.integration.test_falcon.ReturnErrorResource.on_get"
+    )
+
+
+def test_bad_status(tracked_requests):
+    with app_with_scout() as app, pytest.raises(AssertionError) as excinfo:
+        TestApp(app).get("/bad-status", expect_errors=True)
+
+    assert isinstance(excinfo.value, AssertionError)
+    assert "should be a three-digit integer" in str(excinfo.value)
+    tracked_request = tracked_requests[0]
+    assert tracked_request.tags["path"] == "/bad-status"
+    assert tracked_request.tags["error"] == "true"
+    assert tracked_request.active_spans == []
+    assert len(tracked_request.complete_spans) == 1
+    span = tracked_request.complete_spans[0]
+    assert (
+        span.operation
+        == "Controller/tests.integration.test_falcon.BadStatusResource.on_get"
+    )