feat(subprocess): Instrument more methods (getsentry#497)

untitaker · web-flow · commit ab3de3a540c3 · 2019-09-13T14:40:27.000+02:00
* feat(subprocess): Instrument more methods

Avoiding parsing arguments for now because that really backfired in the
past and it's probably not that useful.

* fix: Fix copypaste error

* fix: Linting
diff --git a/sentry_sdk/integrations/stdlib.py b/sentry_sdk/integrations/stdlib.py
@@ -184,10 +184,41 @@ def sentry_patched_popen_init(self, *a, **kw):
         with hub.start_span(op="subprocess", description=description) as span:
             span.set_data("subprocess.cwd", cwd)
 
-            return old_popen_init(self, *a, **kw)
+            rv = old_popen_init(self, *a, **kw)
+
+            span.set_tag("subprocess.pid", self.pid)
+            return rv
 
     subprocess.Popen.__init__ = sentry_patched_popen_init  # type: ignore
 
+    old_popen_wait = subprocess.Popen.wait
+
+    def sentry_patched_popen_wait(self, *a, **kw):
+        hub = Hub.current
+
+        if hub.get_integration(StdlibIntegration) is None:
+            return old_popen_wait(self, *a, **kw)
+
+        with hub.start_span(op="subprocess.wait") as span:
+            span.set_tag("subprocess.pid", self.pid)
+            return old_popen_wait(self, *a, **kw)
+
+    subprocess.Popen.wait = sentry_patched_popen_wait  # type: ignore
+
+    old_popen_communicate = subprocess.Popen.communicate
+
+    def sentry_patched_popen_communicate(self, *a, **kw):
+        hub = Hub.current
+
+        if hub.get_integration(StdlibIntegration) is None:
+            return old_popen_communicate(self, *a, **kw)
+
+        with hub.start_span(op="subprocess.communicate") as span:
+            span.set_tag("subprocess.pid", self.pid)
+            return old_popen_communicate(self, *a, **kw)
+
+    subprocess.Popen.communicate = sentry_patched_popen_communicate  # type: ignore
+
 
 def get_subprocess_traceparent_headers():
     return EnvironHeaders(os.environ, prefix="SUBPROCESS_")
diff --git a/tests/integrations/stdlib/test_subprocess.py b/tests/integrations/stdlib/test_subprocess.py
@@ -137,14 +137,38 @@ def test_subprocess_basic(
 
     assert transaction_event["type"] == "transaction"
 
-    subprocess_span, = transaction_event["spans"]
-
-    assert subprocess_span["data"] == data
+    subprocess_init_span, subprocess_wait_span, subprocess_communicate_span = transaction_event[
+        "spans"
+    ]
+
+    assert subprocess_init_span["op"] == "subprocess"
+    assert subprocess_communicate_span["op"] == "subprocess.communicate"
+    assert subprocess_wait_span["op"] == "subprocess.wait"
+
+    # span hierarchy
+    assert (
+        subprocess_wait_span["parent_span_id"] == subprocess_communicate_span["span_id"]
+    )
+    assert (
+        subprocess_communicate_span["parent_span_id"]
+        == subprocess_init_span["parent_span_id"]
+        == transaction_event["contexts"]["trace"]["span_id"]
+    )
+
+    # common data
+    assert (
+        subprocess_init_span["tags"]["subprocess.pid"]
+        == subprocess_wait_span["tags"]["subprocess.pid"]
+        == subprocess_communicate_span["tags"]["subprocess.pid"]
+    )
+
+    # data of init span
+    assert subprocess_init_span["data"] == data
     if iterator:
-        assert "iterator" in subprocess_span["description"]
-        assert subprocess_span["description"].startswith("<")
+        assert "iterator" in subprocess_init_span["description"]
+        assert subprocess_init_span["description"].startswith("<")
     else:
-        assert sys.executable + " -c" in subprocess_span["description"]
+        assert sys.executable + " -c" in subprocess_init_span["description"]
 
 
 def test_subprocess_invalid_args(sentry_init):
