From e160b36c12fc2c7cca1ade143c498d37a538e0a0 Mon Sep 17 00:00:00 2001 From: erikayasuda <153395705+erikayasuda@users.noreply.github.com> Date: Thu, 26 Sep 2024 14:51:35 -0400 Subject: [PATCH] fix(tracing): avoid assigning span's local root to self, so that the python GC doesn't kick in (#10809) _<>_ We (Modal Labs) noticed that enabling tracing in parts of our code caused the Python GC to kick in a lot and add large delays to our event loop. A simple repro (see test) showed us that there was a self reference cycle in span when a top-level span's local root was assigned to the span itself, preventing Python from cleaning up the objects by reference counting. The change itself is pretty straightforward - we use None to represent when a span's local root is itself. The span still exposes _local_root with the same behavior via a property, and internally stores a _local_root_value that holds a non-self-referential span. This also means that the tracer no longer needs to explicitly set the local root for top-level spans. This should be fairly non-risky as it doesn't change any code behavior. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) --------- Co-authored-by: Daniel Shaar Co-authored-by: Daniel Shaar --- ddtrace/_trace/span.py | 21 +++++++++++++++++-- ddtrace/_trace/tracer.py | 3 --- ddtrace/propagation/http.py | 1 + .../notes/fix-span-unnecessary-gc.yaml | 4 ++++ tests/tracer/test_tracer.py | 20 ++++++++++++++++++ 5 files changed, 44 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/fix-span-unnecessary-gc.yaml diff --git a/ddtrace/_trace/span.py b/ddtrace/_trace/span.py index fae163e748f..ae642b97a19 100644 --- a/ddtrace/_trace/span.py +++ b/ddtrace/_trace/span.py @@ -113,7 +113,7 @@ class Span(object): "duration_ns", # Internal attributes "_context", - "_local_root", + "_local_root_value", "_parent", "_ignored_exceptions", "_on_finish_callbacks", @@ -206,7 +206,7 @@ def __init__( self._events: List[SpanEvent] = [] self._parent: Optional["Span"] = None self._ignored_exceptions: Optional[List[Type[Exception]]] = None - self._local_root: Optional["Span"] = None + self._local_root_value: Optional["Span"] = None # None means this is the root span. self._store: Optional[Dict[str, Any]] = None def _ignore_exception(self, exc: Type[Exception]) -> None: @@ -595,6 +595,23 @@ def context(self) -> Context: self._context = Context(trace_id=self.trace_id, span_id=self.span_id, is_remote=False) return self._context + @property + def _local_root(self) -> "Span": + if self._local_root_value is None: + return self + return self._local_root_value + + @_local_root.setter + def _local_root(self, value: "Span") -> None: + if value is not self: + self._local_root_value = value + else: + self._local_root_value = None + + @_local_root.deleter + def _local_root(self) -> None: + del self._local_root_value + def link_span(self, context: Context, attributes: Optional[Dict[str, Any]] = None) -> None: """Defines a causal relationship between two spans""" if not context.trace_id or not context.span_id: diff --git a/ddtrace/_trace/tracer.py b/ddtrace/_trace/tracer.py index 493d161f058..bc4caea46f8 100644 --- a/ddtrace/_trace/tracer.py +++ b/ddtrace/_trace/tracer.py @@ -797,8 +797,6 @@ def _start_span( span._parent = parent span._local_root = parent._local_root - if span._local_root is None: - span._local_root = span for k, v in _get_metas_to_propagate(context): # We do not want to propagate AppSec propagation headers # to children spans, only across distributed spans @@ -815,7 +813,6 @@ def _start_span( span_api=span_api, on_finish=[self._on_span_finish], ) - span._local_root = span if config.report_hostname: span.set_tag_str(HOSTNAME_KEY, hostname.get_hostname()) diff --git a/ddtrace/propagation/http.py b/ddtrace/propagation/http.py index de3c2a91325..9448311a8ab 100644 --- a/ddtrace/propagation/http.py +++ b/ddtrace/propagation/http.py @@ -988,6 +988,7 @@ def parent_call(): span_context = non_active_span.context if hasattr(ddtrace, "tracer") and hasattr(ddtrace.tracer, "sample"): + root_span: Optional[Span] = None if non_active_span is not None: root_span = non_active_span._local_root else: diff --git a/releasenotes/notes/fix-span-unnecessary-gc.yaml b/releasenotes/notes/fix-span-unnecessary-gc.yaml new file mode 100644 index 00000000000..20ec741dd96 --- /dev/null +++ b/releasenotes/notes/fix-span-unnecessary-gc.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + tracing: Removes a reference cycle that caused unnecessary garbage collection for top-level spans. diff --git a/tests/tracer/test_tracer.py b/tests/tracer/test_tracer.py index 5e5a052751a..59bcbf345cb 100644 --- a/tests/tracer/test_tracer.py +++ b/tests/tracer/test_tracer.py @@ -2057,3 +2057,23 @@ def test_asm_standalone_configuration(): assert tracer._compute_stats is False # reset tracer values tracer.configure(appsec_enabled=False, appsec_standalone_enabled=False) + + +def test_gc_not_used_on_root_spans(): + tracer = ddtrace.Tracer() + gc.freeze() + + with tracer.trace("test-event"): + pass + + # There should be no more span objects lingering around. + assert not any(str(obj).startswith("