fix: Fix a bunch of bugs in subprocess integration (getsentry#449)

untitaker · web-flow · commit 8ecc159fda36 · 2019-08-07T09:36:54.000+02:00
* fix: Fix a bunch of bugs in subprocess integration

* fix: Lints

* fix: Add comment

* fix: Skip test on pypy
diff --git a/sentry_sdk/integrations/stdlib.py b/sentry_sdk/integrations/stdlib.py
@@ -7,12 +7,15 @@
 from sentry_sdk.integrations import Integration
 from sentry_sdk.scope import add_global_event_processor
 from sentry_sdk.tracing import EnvironHeaders, record_http_request
+from sentry_sdk.utils import capture_internal_exceptions, safe_repr
+
 
 try:
     from httplib import HTTPConnection  # type: ignore
 except ImportError:
     from http.client import HTTPConnection
 
+
 _RUNTIME_CONTEXT = {
     "name": platform.python_implementation(),
     "version": "%s.%s.%s" % (sys.version_info[:3]),
@@ -123,14 +126,18 @@ def _init_argument(args, kwargs, name, position, setdefault_callback=None):
 
     if name in kwargs:
         rv = kwargs[name]
-        if rv is None and setdefault_callback is not None:
-            rv = kwargs[name] = setdefault_callback()
+        if setdefault_callback is not None:
+            rv = setdefault_callback(rv)
+        if rv is not None:
+            kwargs[name] = rv
     elif position < len(args):
         rv = args[position]
-        if rv is None and setdefault_callback is not None:
-            rv = args[position] = setdefault_callback()
+        if setdefault_callback is not None:
+            rv = setdefault_callback(rv)
+        if rv is not None:
+            args[position] = rv
     else:
-        rv = setdefault_callback and setdefault_callback()
+        rv = setdefault_callback and setdefault_callback(None)
         if rv is not None:
             kwargs[name] = rv
 
@@ -145,20 +152,37 @@ def sentry_patched_popen_init(self, *a, **kw):
         if hub.get_integration(StdlibIntegration) is None:
             return old_popen_init(self, *a, **kw)
 
-        # do not setdefault! args is required by Popen, doing setdefault would
-        # make invalid calls valid
+        # Convert from tuple to list to be able to set values.
+        a = list(a)
+
         args = _init_argument(a, kw, "args", 0) or []
-        cwd = _init_argument(a, kw, "cwd", 10)
+        cwd = _init_argument(a, kw, "cwd", 9)
+
+        # if args is not a list or tuple (and e.g. some iterator instead),
+        # let's not use it at all. There are too many things that can go wrong
+        # when trying to collect an iterator into a list and setting that list
+        # into `a` again.
+        #
+        # Also invocations where `args` is not a sequence are not actually
+        # legal. They just happen to work under CPython.
+        description = None
+
+        if isinstance(args, (list, tuple)) and len(args) < 100:
+            with capture_internal_exceptions():
+                description = " ".join(map(str, args))
+
+        if description is None:
+            description = safe_repr(args)
 
         env = None
 
         for k, v in hub.iter_trace_propagation_headers():
             if env is None:
-                env = _init_argument(a, kw, "env", 11, lambda: dict(os.environ))
+                env = _init_argument(a, kw, "env", 10, lambda x: dict(x or os.environ))
             env["SUBPROCESS_" + k.upper().replace("-", "_")] = v
 
-        with hub.span(op="subprocess", description=" ".join(map(str, args))) as span:
-            span.set_tag("subprocess.cwd", cwd)
+        with hub.span(op="subprocess", description=description) as span:
+            span.set_data("subprocess.cwd", cwd)
 
             return old_popen_init(self, *a, **kw)
 
diff --git a/tests/integrations/stdlib/test_subprocess.py b/tests/integrations/stdlib/test_subprocess.py
@@ -1,4 +1,5 @@
 import os
+import platform
 import subprocess
 import sys
 
@@ -9,45 +10,137 @@
 from sentry_sdk.integrations.stdlib import StdlibIntegration
 
 
-def test_subprocess_basic(sentry_init, capture_events, monkeypatch):
+if PY2:
+    from collections import Mapping
+else:
+    from collections.abc import Mapping
+
+
+class ImmutableDict(Mapping):
+    def __init__(self, inner):
+        self.inner = inner
+
+    def __getitem__(self, key):
+        return self.inner[key]
+
+    def __iter__(self):
+        return iter(self.inner)
+
+    def __len__(self):
+        return len(self.inner)
+
+
+@pytest.mark.parametrize("positional_args", [True, False])
+@pytest.mark.parametrize(
+    "iterator",
+    [
+        pytest.param(
+            True,
+            marks=pytest.mark.skipif(
+                platform.python_implementation() == "PyPy",
+                reason="https://github.com/getsentry/sentry-python/pull/449",
+            ),
+        ),
+        False,
+    ],
+)
+@pytest.mark.parametrize("env_mapping", [None, os.environ, ImmutableDict(os.environ)])
+@pytest.mark.parametrize("with_cwd", [True, False])
+def test_subprocess_basic(
+    sentry_init,
+    capture_events,
+    monkeypatch,
+    positional_args,
+    iterator,
+    env_mapping,
+    with_cwd,
+):
     monkeypatch.setenv("FOO", "bar")
 
     old_environ = dict(os.environ)
 
     sentry_init(integrations=[StdlibIntegration()], traces_sample_rate=1.0)
+    events = capture_events()
 
     with Hub.current.span(transaction="foo", op="foo") as span:
-        output = subprocess.check_output(
-            [
-                sys.executable,
-                "-c",
-                "import os; "
-                "import sentry_sdk; "
-                "from sentry_sdk.integrations.stdlib import get_subprocess_traceparent_headers; "
-                "sentry_sdk.init(); "
-                "assert os.environ['FOO'] == 'bar'; "
-                "print(dict(get_subprocess_traceparent_headers()))",
-            ]
-        )
+        args = [
+            sys.executable,
+            "-c",
+            "import os; "
+            "import sentry_sdk; "
+            "from sentry_sdk.integrations.stdlib import get_subprocess_traceparent_headers; "
+            "sentry_sdk.init(); "
+            "assert os.environ['FOO'] == 'bar'; "
+            "print(dict(get_subprocess_traceparent_headers()))",
+        ]
+
+        if iterator:
+            args = iter(args)
+
+        if positional_args:
+            a = (
+                args,
+                0,  # bufsize
+                None,  # executable
+                None,  # stdin
+                subprocess.PIPE,  # stdout
+                None,  # stderr
+                None,  # preexec_fn
+                False,  # close_fds
+                False,  # shell
+                os.getcwd() if with_cwd else None,  # cwd
+            )
+
+            if env_mapping is not None:
+                a += (env_mapping,)
+
+            popen = subprocess.Popen(*a)
+
+        else:
+            kw = {"args": args, "stdout": subprocess.PIPE}
+
+            if with_cwd:
+                kw["cwd"] = os.getcwd()
+
+            if env_mapping is not None:
+                kw["env"] = env_mapping
+
+            popen = subprocess.Popen(**kw)
+
+        output, unused_err = popen.communicate()
+        retcode = popen.poll()
+        assert not retcode
 
     assert os.environ == old_environ
 
     assert span.trace_id in str(output)
 
-    events = capture_events()
-
     capture_message("hi")
 
-    event, = events
+    transaction_event, message_event, = events
+
+    assert message_event["message"] == "hi"
 
-    crumb, = event["breadcrumbs"]
+    data = {"subprocess.cwd": os.getcwd()} if with_cwd else {}
+    crumb, = message_event["breadcrumbs"]
     assert crumb == {
         "category": "subprocess",
-        "data": {},
+        "data": data,
         "timestamp": crumb["timestamp"],
         "type": "subprocess",
     }
 
+    assert transaction_event["type"] == "transaction"
+
+    subprocess_span, = transaction_event["spans"]
+
+    assert subprocess_span["data"] == data
+    if iterator:
+        assert "iterator" in subprocess_span["description"]
+        assert subprocess_span["description"].startswith("<")
+    else:
+        assert sys.executable + " -c" in subprocess_span["description"]
+
 
 def test_subprocess_invalid_args(sentry_init):
     sentry_init(integrations=[StdlibIntegration()])
