Repository: scoutapp/scout_apm_python
Pull request #195 (2019-06-27 17:42:11)

Improve Pyramid integration
* Rearrange `try/except` so `stop_span()` will only be called if it was started
* Use and test the full `user_ip` algorithm rather than just `request.remote_addr`, and reduce scope of its `try/except`
* Improve some tests


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2b81531..d1f5e9d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,8 +5,8 @@
 ### Added
 
 - Track user IP on Flask
-- Make user IP tracking on Bottle use the same algorithm that checks for the
-  `client-ip` header
+- Make user IP tracking on Bottle and Pyramid use the same algorithm as other
+  integrations, checking for the `client-ip` header
 
 ### Fixed
 
diff --git a/README.md b/README.md
index 4e1c398..b9c3955 100644
--- a/README.md
+++ b/README.md
@@ -6,21 +6,18 @@ Monitor the performance of Python Django apps, Flask apps, and Celery workers wi
 
 ## Requirements
 
-Python Versions:
+Python 2.7 or 3.4+.
 
-* Python 3.4+
-* Python 2.7
-
-Scout APM works with the following frameworks:
+Scout APM has integrations for the following frameworks:
 
+* Bottle 0.12+
+* Celery 3.1+
 * Django 1.8+
 * Falcon 2.0+
 * Flask 0.10+
-* Celery 3.1+
 * Pyramid 1.8+
-* Bottle 0.12+
 
-For frameworks not listed above, you can use the agent's instrumentation API. See the [Python help docs](https://docs.scoutapm.com/#python-agent) for more information.
+For other frameworks, you can use the agent's instrumentation API. See the [Python help docs](https://docs.scoutapm.com/#python-agent) for more information.
 
 ## Quick Start
 
@@ -119,12 +116,11 @@ if __name__ == "__main__":
         # Rest of your config...
 ```
 
-For full installation instructions, including information on configuring Scout via environment variables, see our [Python docs](https://docs.scoutapm.com/#python-agent).
-
 ## Documentation
 
-For full installation and troubleshooting documentation, visit our
-[help site](https://docs.scoutapm.com/#python-agent).
+For full installation instructions, including information on configuring Scout
+via environment variables and troubleshooting documentation, see our
+[Python docs](https://docs.scoutapm.com/#python-agent).
 
 ## Support
 
diff --git a/src/scout_apm/pyramid.py b/src/scout_apm/pyramid.py
index 078e623..4f49dc8 100644
--- a/src/scout_apm/pyramid.py
+++ b/src/scout_apm/pyramid.py
@@ -2,7 +2,6 @@
 from __future__ import absolute_import, division, print_function, unicode_literals
 
 import scout_apm.core
-from scout_apm.api.context import Context
 from scout_apm.core.config import ScoutConfig
 from scout_apm.core.ignore import ignore_path
 from scout_apm.core.tracked_request import TrackedRequest
@@ -24,34 +23,42 @@ def includeme(config):
 
 def instruments(handler, registry):
     def scout_tween(request):
-        try:
-            tr = TrackedRequest.instance()
-            span = tr.start_span(operation="Controller/Pyramid")
+        tracked_request = TrackedRequest.instance()
+        span = tracked_request.start_span(operation="Controller/Pyramid")
 
+        try:
             if ignore_path(request.path):
-                tr.tag("ignore_transaction", True)
+                tracked_request.tag("ignore_transaction", True)
+
+            tracked_request.tag("path", request.path)
 
-            # Capture what we can from the request, but never fail
             try:
-                Context.add("path", request.path)
-                Context.add("user_ip", request.remote_addr)
+                # Determine a remote IP to associate with the request. The value is
+                # spoofable by the requester so this is not suitable to use in any
+                # security sensitive context.
+                user_ip = (
+                    request.headers.get("x-forwarded-for", default="").split(",")[0]
+                    or request.headers.get("client-ip", default="").split(",")[0]
+                    or request.remote_addr
+                )
+                tracked_request.tag("user_ip", user_ip)
             except Exception:
                 pass
 
             try:
                 response = handler(request)
             except Exception:
-                tr.tag("error", "true")
+                tracked_request.tag("error", "true")
                 raise
 
             # This happens further down the call chain. So time it starting
             # above, but only name it if it gets to here.
             if request.matched_route is not None:
-                tr.mark_real_request()
+                tracked_request.mark_real_request()
                 span.operation = "Controller/" + request.matched_route.name
 
         finally:
-            tr.stop_span()
+            tracked_request.stop_span()
 
         return response
 


Test patch:
diff --git a/tests/integration/test_pyramid.py b/tests/integration/test_pyramid.py
index 4b24387..3127c05 100644
--- a/tests/integration/test_pyramid.py
+++ b/tests/integration/test_pyramid.py
@@ -1,6 +1,7 @@
 # coding=utf-8
 from __future__ import absolute_import, division, print_function, unicode_literals
 
+import sys
 from contextlib import contextmanager
 
 import pytest
@@ -9,7 +10,9 @@ from pyramid.response import Response
 from webtest import TestApp
 
 from scout_apm.api import Config
+from scout_apm.core.tracked_request import TrackedRequest
 from tests.compat import mock
+from tests.integration.util import parametrize_user_ip_headers
 
 
 @contextmanager
@@ -69,21 +72,35 @@ def test_home(tracked_requests):
     assert span.operation == "Controller/home"
 
 
-@mock.patch(
-    "pyramid.request.Request.remote_addr",
-    new_callable=mock.PropertyMock,
-    side_effect=ValueError,
-)
-def test_user_ip_exception(remote_addr, tracked_requests):
+@parametrize_user_ip_headers
+def test_user_ip(headers, extra_environ, expected, tracked_requests):
+    if sys.version_info[0] == 2:
+        # Required for WebTest lint
+        headers = {str(k): str(v) for k, v in headers.items()}
+        extra_environ = {str(k): str(v) for k, v in extra_environ.items()}
+
+    with app_with_scout() as app:
+        TestApp(app).get("/", headers=headers, extra_environ=extra_environ)
+
+    tracked_request = tracked_requests[0]
+    assert tracked_request.tags["user_ip"] == expected
+
+
+def test_user_ip_exception(tracked_requests):
     """
     Scout doesn't crash if pyramid.request.Request.remote_addr raises an exception.
 
     This cannot be tested without mocking because it should never happen.
 
     It's implemented in webob.request.Request and it's just a lookup in environ.
-
     """
-    with app_with_scout() as app:
+    remote_addr_patcher = mock.patch(
+        "pyramid.request.Request.remote_addr",
+        new_callable=mock.PropertyMock,
+        side_effect=ValueError,
+    )
+
+    with app_with_scout() as app, remote_addr_patcher:
         response = TestApp(app).get("/hello/")
 
     assert response.status_int == 200
@@ -106,10 +123,11 @@ def test_hello(tracked_requests):
     assert response.status_int == 200
     assert response.text == "Hello World!"
     assert len(tracked_requests) == 1
-    assert len(tracked_requests[0].complete_spans) == 1
-    span = tracked_requests[0].complete_spans[0]
+    tracked_request = tracked_requests[0]
+    assert tracked_request.tags["path"] == "/hello/"
+    assert len(tracked_request.complete_spans) == 1
+    span = tracked_request.complete_spans[0]
     assert span.operation == "Controller/hello"
-    # assert span.tags["path"] == "/hello/"
 
 
 def test_not_found(tracked_requests):
@@ -118,7 +136,7 @@ def test_not_found(tracked_requests):
 
     assert response.status_int == 404
     assert tracked_requests == []
-    # TODO: assert that tracked request is released
+    assert TrackedRequest._thread_lookup.instance is None
 
 
 def test_server_error(tracked_requests):
@@ -127,7 +145,7 @@ def test_server_error(tracked_requests):
         TestApp(app).get("/crash/", expect_errors=True)
 
     assert tracked_requests == []
-    # TODO: assert that tracked request is released
+    assert TrackedRequest._thread_lookup.instance is None
 
 
 def test_no_monitor(tracked_requests):