fix(tracing): Break `transaction` / `span` circular references before garbage collection #1184

lobsterkatie · 2021-09-07T23:44:00Z

Background for anyone not familiar with Python's internal workings (skip ahead if you that's not you):

Python handles memory deallocation through a combination of reference counting and cyclic garbage collection, the former taking way fewer resources than the latter. Having circular references forces the cyclic garbage collector to run, since anything involved in a reference cycle will never have a refcount of 0, even once everything outside of the cycle is done with all of its members. The gc module used in the test mentioned below is a window specifically into the cyclic garbage collector part of the memory deallocation system, and its collect method returns the number of objects it was forced to deal with. See https://devguide.python.org/garbage_collector/ and https://docs.python.org/3/library/gc.html?highlight=gc#module-gc for more info.

There are currently a few places in the SDK where we have circular references:

Transaction -> span recorder -> spans including transaction itself
Child span -> span recorder -> spans including child span itself
Transaction -> span recorder -> spans -> containing transaction
In serializer.py, _serialize_node() -> _serialize_node_impl() -> _serialize_node(), making each a closure for the other.

This PR addresses points 1-3*, by making the following changes:

Transaction -> span recorder -> spans ~~including transaction itself~~
Transactions are no longer added to their own span recorders. (The SDK doesn't ever use the fact that they're there, and they're stripped out before the event is sent to Sentry.)
~~Child span ->~~ span recorder -> spans including child span itself
Child spans no longer have their own span recorder pointer, and instead access the recorder through their containing transaction.
Transaction ~~-> span recorder -> spans -> containing transaction~~
When a transaction ends, after it harvests any completed spans, it now jettisons its link to its span recorder before it (the transaction) goes out of scope.

It also adds to/modifies tests covering all three scenarios

*Point 4, which we only discovered in the process of fixing 1-3, concerns a different system than the rest, and therefore will need to be fixed in a separate PR. (h/t @untitaker for tracking this down to the serializer)

…cycle

untitaker · 2021-09-10T13:16:07Z

tests/integrations/sqlalchemy/test_sqlalchemy.py


    # Some spans have their descriptions truncated. Because the test always
    # generates the same amount of descriptions and truncation is deterministic,
    # the number here should never change across test runs.
    #
    # Which exact span descriptions are truncated depends on the span durations
    # of each SQL query and is non-deterministic.
-    assert len(event["_meta"]["spans"]) == 536


Why does this change? If this report changed because of spanrecorder changes I wonder if we should subtract 1 just to not change reporting format

I'm not sure why testing for x - 1 = y is better than testing x = y + 1. We're making a change either way...

I guess I'm not clear on the reason for this particular assertion, though. This is measuring how many spans get their description truncated, I take it? Why is that something we want to test against?

untitaker · 2021-09-10T13:16:25Z

tests/tracing/test_misc.py

+    # immediately after the initial collection below, so we can see what new
+    # objects the garbage collecter has to clean up once `transaction.finish` is
+    # called and the serializer runs.)
+    monkeypatch.setattr(


interesting way to deal with this! like it

untitaker · 2021-09-10T13:17:56Z

sentry_sdk/tracing.py

-        child._span_recorder = recorder = self._span_recorder
-        if recorder:
-            recorder.add(child)
+        span_recorder = (


Does this mean the entire Span._span_recorder field should be moved to Transaction?

In the long run, yes, I think so.

… garbage collection (#1184) There are a few places in the SDK where we have circular references: 1) Transaction -> span recorder -> spans including transaction itself 2) Child span -> span recorder -> spans including child span itself 3) Transaction -> span recorder -> spans -> containing transaction 4) In `serializer.py`, `_serialize_node()` -> `_serialize_node_impl()` -> `_serialize_node()`, making each a closure for the other. This PR addresses points 1-3*, by making the following changes: 1) Transactions are no longer added to their own span recorders. (The SDK doesn't ever use the fact that they're there, and they're stripped out before the event is sent to Sentry.) 2) Child spans no longer have their own span recorder pointer, and instead access the recorder through their containing transaction. 3) When a transaction ends, after it harvests any completed spans, it now jettisons its link to its span recorder before it (the transaction) goes out of scope. It also adds to/modifies tests covering all three scenarios *Point 4, which we only discovered in the process of fixing 1-3, concerns a different system than the rest, and therefore will need a separate fix.

This introduces handling of the `tracestate` header, as described in the W3C Trace Context spec[1] and our own corresponding spec[2]. Key features: - Deprecation of `from_traceparent` in favor of `continue_from_headers`, which now propagates both incoming `sentry-trace` and incoming `tracestate` headers. - Propagation of `tracestate` value as a header on outgoing HTTP requests when they're made during a transaction. - Addition of `tracestate` data to transaction envelope headers. Supporting changes: - New utility methods for converting strings to and from base64. - Some refactoring vis-à-vis the links between transactions, span recorders, and spans. See #1173 and #1184. - Moving of some tracing code to a separate `tracing_utils` file. Note: `tracestate` handling is currently feature-gated by the flag `propagate_tracestate` in the `_experiments` SDK option. More details can be found in the main PR on this branch, #971. [1] https://www.w3.org/TR/trace-context/#tracestate-header [2] https://develop.sentry.dev/sdk/performance/trace-context/

untitaker force-pushed the kmclb-fix-transaction-span-circular-reference branch from bd3123d to 531aee1 Compare September 8, 2021 11:51

lobsterkatie force-pushed the kmclb-fix-transaction-span-circular-reference branch from ebf9b56 to 3d50085 Compare September 10, 2021 06:17

lobsterkatie added 3 commits September 10, 2021 00:15

stop adding transaction to span recorder

114e251

get to span recorder through transaction rather than storing it on span

dfdf9aa

break transaction -> span recorder -> span -> containing transaction …

4f5c1bf

…cycle

lobsterkatie force-pushed the kmclb-fix-transaction-span-circular-reference branch from 3d50085 to 1cd961f Compare September 10, 2021 07:19

lobsterkatie added 2 commits September 10, 2021 01:49

add circular reference test

577600b

fix sqlalchemy test

9fe89d0

lobsterkatie force-pushed the kmclb-fix-transaction-span-circular-reference branch from 1cd961f to 9fe89d0 Compare September 10, 2021 08:50

untitaker approved these changes Sep 10, 2021

View reviewed changes

mock uuid4

ef45c85

lobsterkatie force-pushed the kmclb-fix-transaction-span-circular-reference branch from 999b5ca to ef45c85 Compare September 13, 2021 19:54

lobsterkatie merged commit 48cadf1 into kmclb-add-tracestate-header-handling Sep 13, 2021

lobsterkatie deleted the kmclb-fix-transaction-span-circular-reference branch September 13, 2021 20:43

lobsterkatie mentioned this pull request Sep 13, 2021

feat(tracing): Add tracestate header handling #1179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(tracing): Break `transaction` / `span` circular references before garbage collection #1184

fix(tracing): Break `transaction` / `span` circular references before garbage collection #1184

lobsterkatie commented Sep 7, 2021 •

edited

Loading

Uh oh!

untitaker Sep 10, 2021

Uh oh!

lobsterkatie Sep 13, 2021 •

edited

Loading

Uh oh!

untitaker Sep 10, 2021

Uh oh!

untitaker Sep 10, 2021

Uh oh!

lobsterkatie Sep 13, 2021

Uh oh!

Uh oh!

fix(tracing): Break transaction / span circular references before garbage collection #1184

fix(tracing): Break transaction / span circular references before garbage collection #1184

Conversation

lobsterkatie commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

untitaker Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

untitaker Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

untitaker Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fix(tracing): Break `transaction` / `span` circular references before garbage collection #1184

fix(tracing): Break `transaction` / `span` circular references before garbage collection #1184

lobsterkatie commented Sep 7, 2021 •

edited

Loading

lobsterkatie Sep 13, 2021 •

edited

Loading