Skip to content

ref: Store tracked spans on start not finish #738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 25, 2020

Conversation

rhcarvalho
Copy link
Contributor

This matches the JS implementation, and without it we cannot use the span recorder of a span to find its parent transaction (required to implement #734).

@rhcarvalho
Copy link
Contributor Author

It turns out that the test failures are due to an off-by-one difference in interpretation of what the max_spans config means.

max_spans = 3 in JS means transaction + 3 child spans.
max_spans = 3 in Python means transaction + 2 child spans.

Going to preserve the inconsistent behavior for now, with a note, since it is not part of the intended work in this PR.

@rhcarvalho rhcarvalho force-pushed the rhcarvalho/ref-spanrecorder branch from 3e46f97 to a45b7eb Compare June 25, 2020 11:26
@rhcarvalho
Copy link
Contributor Author

rhcarvalho commented Jun 25, 2020

Changes to get tests passing:

diff --git a/sentry_sdk/tracing.py b/sentry_sdk/tracing.py
index 9f0e197..5e9ae8a 100644
--- a/sentry_sdk/tracing.py
+++ b/sentry_sdk/tracing.py
@@ -73,7 +73,12 @@ class _SpanRecorder(object):
 
     def __init__(self, maxlen):
         # type: (int) -> None
-        self.maxlen = maxlen
+        # FIXME: this is `maxlen - 1` only to preserve historical behavior
+        # enforced by tests.
+        # Either this should be changed to `maxlen` or the JS SDK implementation
+        # should be changed to match a consistent interpretation of what maxlen
+        # limits: either transaction+spans or only child spans.
+        self.maxlen = maxlen - 1
         self.spans = []  # type: List[Span]
 
     def add(self, span):
diff --git a/tests/integrations/stdlib/test_subprocess.py b/tests/integrations/stdlib/test_subprocess.py
index ee6e7c8..e2ae005 100644
--- a/tests/integrations/stdlib/test_subprocess.py
+++ b/tests/integrations/stdlib/test_subprocess.py
@@ -140,13 +140,15 @@ def test_subprocess_basic(
 
     (
         subprocess_init_span,
-        subprocess_wait_span,
         subprocess_communicate_span,
+        subprocess_wait_span,
     ) = transaction_event["spans"]
 
-    assert subprocess_init_span["op"] == "subprocess"
-    assert subprocess_communicate_span["op"] == "subprocess.communicate"
-    assert subprocess_wait_span["op"] == "subprocess.wait"
+    assert (
+        subprocess_init_span["op"],
+        subprocess_communicate_span["op"],
+        subprocess_wait_span["op"],
+    ) == ("subprocess", "subprocess.communicate", "subprocess.wait")
 
     # span hierarchy
     assert (

Plus updated commit message.

This matches the JS implementation. Without it, we cannot use the span
recorder of a span to find its parent transaction.

Note about test changes

Instrumented subprocess methods are called in this order: __init__,
communicate, wait. Because we now store the spans on start, that's the
order we expect the spans to be in. The previous order was based on
finish time.

Grouping the assertion of "op" values together produces better output on
failure, because one can easily detect what all the "op" values are,
instead of being left with only the first one that is different.

Similar to subprocess changes, the order of expected middleware spans in
Django is now sorted by start time.
@rhcarvalho
Copy link
Contributor Author

And hopefully last test fix accounting for the change in order of spans.

diff --git a/tests/integrations/django/test_basic.py b/tests/integrations/django/test_basic.py
index b3a08f5..3c26b42 100644
--- a/tests/integrations/django/test_basic.py
+++ b/tests/integrations/django/test_basic.py
@@ -518,10 +518,10 @@ def test_middleware_spans(sentry_init, client, capture_events):
 
     if DJANGO_VERSION >= (1, 10):
         reference_value = [
-            "tests.integrations.django.myapp.settings.TestFunctionMiddleware.__call__",
-            "tests.integrations.django.myapp.settings.TestMiddleware.__call__",
-            "django.contrib.auth.middleware.AuthenticationMiddleware.__call__",
             "django.contrib.sessions.middleware.SessionMiddleware.__call__",
+            "django.contrib.auth.middleware.AuthenticationMiddleware.__call__",
+            "tests.integrations.django.myapp.settings.TestMiddleware.__call__",
+            "tests.integrations.django.myapp.settings.TestFunctionMiddleware.__call__",
         ]
     else:
         reference_value = [

@rhcarvalho rhcarvalho force-pushed the rhcarvalho/ref-spanrecorder branch from a45b7eb to 724e61c Compare June 25, 2020 12:38
@rhcarvalho rhcarvalho merged commit b539ecb into master Jun 25, 2020
@rhcarvalho rhcarvalho deleted the rhcarvalho/ref-spanrecorder branch June 25, 2020 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants