From 557dca523e44ae97a6f9cb744c3e7c7cbea18e93 Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Mon, 9 Sep 2024 22:28:00 +0000 Subject: [PATCH 01/13] Avoid assigning a span's local root to itself so that the python GC doesn't need to kick in. --- ddtrace/_trace/span.py | 19 +++++++++++++++++-- ddtrace/_trace/tracer.py | 3 --- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/ddtrace/_trace/span.py b/ddtrace/_trace/span.py index 8435c7b66eb..88c558fef7e 100644 --- a/ddtrace/_trace/span.py +++ b/ddtrace/_trace/span.py @@ -110,7 +110,7 @@ class Span(object): "duration_ns", # Internal attributes "_context", - "_local_root", + "_local_root_value", "_parent", "_ignored_exceptions", "_on_finish_callbacks", @@ -201,7 +201,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: @@ -589,6 +589,21 @@ def context(self) -> Context: if self._context is None: 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 + + @_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""" 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()) From fe586f98236809a7ac4a1bcbbfc39cd72294abf8 Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Mon, 9 Sep 2024 22:37:02 +0000 Subject: [PATCH 02/13] Add a test for the new root span GC behavior. --- tests/tracer/test_tracer.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/tracer/test_tracer.py b/tests/tracer/test_tracer.py index bb4bffdf5b9..5aff31cab3c 100644 --- a/tests/tracer/test_tracer.py +++ b/tests/tracer/test_tracer.py @@ -2060,3 +2060,36 @@ 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 + + objects = gc.get_objects() + assert len(objects) == 2 + # It probably doesn't make sense to check the exact nature of the objects, but importantly, everything should get + # cleaned up (the root span, trace, context, etc.). If this is not the case, you can uncomment the following code + # to inspect the objects that are not being cleaned up. + + # for i, obj in enumerate(objects): + # print("--------------------") + # print(f"object {i}:", obj) + # print("referrers:", [f"object {objects.index(r)}" for r in gc.get_referrers(obj)[:-2]]) + # print("referents:", [f"object {objects.index(r)}" if r in objects else r for r in gc.get_referents(obj)]) + # print("--------------------") + + # Expected output: + # -------------------- + # object 0: + # referrers: ['object 1'] + # referents: [, , None, ] + # -------------------- + # -------------------- + # object 1: + # referrers: [] + # referents: ['object 0'] + # -------------------- \ No newline at end of file From 7f36f27ec171a38db0b2a9b59802dbb9c15032f2 Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Wed, 11 Sep 2024 12:42:12 +0000 Subject: [PATCH 03/13] Add release note for unnecessary gc fix. --- releasenotes/notes/fix-span-unnecessary-gc.yaml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 releasenotes/notes/fix-span-unnecessary-gc.yaml diff --git a/releasenotes/notes/fix-span-unnecessary-gc.yaml b/releasenotes/notes/fix-span-unnecessary-gc.yaml new file mode 100644 index 00000000000..640f9c051d0 --- /dev/null +++ b/releasenotes/notes/fix-span-unnecessary-gc.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - | + tracing: Removed a reference cycle that caused unnecessary garbage collection for top-level spans. From 618f2b8ca130361553ae781bd1e0290eeb8b5989 Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Wed, 11 Sep 2024 12:47:50 +0000 Subject: [PATCH 04/13] Run black formatter. --- ddtrace/_trace/span.py | 4 ++-- tests/tracer/test_tracer.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ddtrace/_trace/span.py b/ddtrace/_trace/span.py index 88c558fef7e..3646bb90785 100644 --- a/ddtrace/_trace/span.py +++ b/ddtrace/_trace/span.py @@ -589,13 +589,13 @@ def context(self) -> Context: if self._context is None: 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: diff --git a/tests/tracer/test_tracer.py b/tests/tracer/test_tracer.py index 5aff31cab3c..9bc57af6b9c 100644 --- a/tests/tracer/test_tracer.py +++ b/tests/tracer/test_tracer.py @@ -2092,4 +2092,4 @@ def test_gc_not_used_on_root_spans(): # object 1: # referrers: [] # referents: ['object 0'] - # -------------------- \ No newline at end of file + # -------------------- From 51b74db83f29f380e3a0fd28b003c50ccb057f35 Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Wed, 11 Sep 2024 12:51:13 +0000 Subject: [PATCH 05/13] Reduce comment line length. --- tests/tracer/test_tracer.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/tracer/test_tracer.py b/tests/tracer/test_tracer.py index 9bc57af6b9c..a662999ef79 100644 --- a/tests/tracer/test_tracer.py +++ b/tests/tracer/test_tracer.py @@ -2086,7 +2086,8 @@ def test_gc_not_used_on_root_spans(): # -------------------- # object 0: # referrers: ['object 1'] - # referents: [, , None, ] + # referents: [, , None, ] # -------------------- # -------------------- # object 1: From d164265ea08a410d7d5cff8fa24c63a180956a3b Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Wed, 11 Sep 2024 12:54:59 +0000 Subject: [PATCH 06/13] Reformat test file again. --- tests/tracer/test_tracer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tracer/test_tracer.py b/tests/tracer/test_tracer.py index a662999ef79..d5d4e7589c1 100644 --- a/tests/tracer/test_tracer.py +++ b/tests/tracer/test_tracer.py @@ -2086,7 +2086,7 @@ def test_gc_not_used_on_root_spans(): # -------------------- # object 0: # referrers: ['object 1'] - # referents: [, , , None, ] # -------------------- # -------------------- From cd66fc2b647f05a9f990697cdfee0c2503a0cf86 Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Wed, 11 Sep 2024 13:34:43 +0000 Subject: [PATCH 07/13] Fix type hint to preserve old typing behavior. --- ddtrace/_trace/span.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ddtrace/_trace/span.py b/ddtrace/_trace/span.py index 3646bb90785..e336e8d0038 100644 --- a/ddtrace/_trace/span.py +++ b/ddtrace/_trace/span.py @@ -591,7 +591,8 @@ def context(self) -> Context: return self._context @property - def _local_root(self) -> "Span": + # This can't actually return None, but we say it can to satisfy type checking. + def _local_root(self) -> Optional["Span"]: if self._local_root_value is None: return self return self._local_root_value From 4520ab82bf2637108eae5700e81fff0d519db544 Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Wed, 18 Sep 2024 20:17:09 +0000 Subject: [PATCH 08/13] Make span gc test clearer. --- tests/tracer/test_tracer.py | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/tests/tracer/test_tracer.py b/tests/tracer/test_tracer.py index d5d4e7589c1..eb993c6b569 100644 --- a/tests/tracer/test_tracer.py +++ b/tests/tracer/test_tracer.py @@ -2069,11 +2069,10 @@ def test_gc_not_used_on_root_spans(): with tracer.trace("test-event"): pass - objects = gc.get_objects() - assert len(objects) == 2 - # It probably doesn't make sense to check the exact nature of the objects, but importantly, everything should get - # cleaned up (the root span, trace, context, etc.). If this is not the case, you can uncomment the following code - # to inspect the objects that are not being cleaned up. + # There should be no more span objects lingering around. + assert not any(str(object).startswith(" - # referrers: ['object 1'] - # referents: [, , None, ] - # -------------------- - # -------------------- - # object 1: - # referrers: [] - # referents: ['object 0'] - # -------------------- From ba5e3636d8a65d342de4096a2075078e623624ad Mon Sep 17 00:00:00 2001 From: Daniel Shaar Date: Wed, 18 Sep 2024 20:20:46 +0000 Subject: [PATCH 09/13] Rename object to obj to avoid shadowing python builtin. --- tests/tracer/test_tracer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tracer/test_tracer.py b/tests/tracer/test_tracer.py index eb993c6b569..5d4cc743596 100644 --- a/tests/tracer/test_tracer.py +++ b/tests/tracer/test_tracer.py @@ -2070,7 +2070,7 @@ def test_gc_not_used_on_root_spans(): pass # There should be no more span objects lingering around. - assert not any(str(object).startswith(" Date: Wed, 25 Sep 2024 16:10:53 -0400 Subject: [PATCH 10/13] set return type for _local_root getter to Span --- ddtrace/_trace/span.py | 6 +++--- ddtrace/propagation/http.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/ddtrace/_trace/span.py b/ddtrace/_trace/span.py index c3601377cb6..2ef0cca9e92 100644 --- a/ddtrace/_trace/span.py +++ b/ddtrace/_trace/span.py @@ -596,11 +596,11 @@ def context(self) -> Context: return self._context @property - # This can't actually return None, but we say it can to satisfy type checking. - def _local_root(self) -> Optional["Span"]: + def _local_root(self) -> "Span": if self._local_root_value is None: return self - return self._local_root_value + else: + return self._local_root_value @_local_root.setter def _local_root(self, value: "Span") -> None: 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: From ea624df9bfb2eaf28c3160d589230498dc397c3f Mon Sep 17 00:00:00 2001 From: erikayasuda Date: Wed, 25 Sep 2024 16:14:14 -0400 Subject: [PATCH 11/13] nit fix for release note --- releasenotes/notes/fix-span-unnecessary-gc.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/releasenotes/notes/fix-span-unnecessary-gc.yaml b/releasenotes/notes/fix-span-unnecessary-gc.yaml index 640f9c051d0..20ec741dd96 100644 --- a/releasenotes/notes/fix-span-unnecessary-gc.yaml +++ b/releasenotes/notes/fix-span-unnecessary-gc.yaml @@ -1,4 +1,4 @@ --- fixes: - | - tracing: Removed a reference cycle that caused unnecessary garbage collection for top-level spans. + tracing: Removes a reference cycle that caused unnecessary garbage collection for top-level spans. From 2689c7f7c4f3aa61456f37079bee6b33d3bfe365 Mon Sep 17 00:00:00 2001 From: erikayasuda Date: Wed, 25 Sep 2024 16:16:52 -0400 Subject: [PATCH 12/13] remove redundant else --- ddtrace/_trace/span.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ddtrace/_trace/span.py b/ddtrace/_trace/span.py index e17746f0ffb..aec5418ed18 100644 --- a/ddtrace/_trace/span.py +++ b/ddtrace/_trace/span.py @@ -599,8 +599,7 @@ def context(self) -> Context: def _local_root(self) -> "Span": if self._local_root_value is None: return self - else: - return self._local_root_value + return self._local_root_value @_local_root.setter def _local_root(self, value: "Span") -> None: From c5d94a768f8b2fbe57ee6cf58098f3dbb07cec60 Mon Sep 17 00:00:00 2001 From: erikayasuda <153395705+erikayasuda@users.noreply.github.com> Date: Wed, 25 Sep 2024 17:07:45 -0400 Subject: [PATCH 13/13] Update ddtrace/_trace/span.py --- ddtrace/_trace/span.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ddtrace/_trace/span.py b/ddtrace/_trace/span.py index aec5418ed18..ae642b97a19 100644 --- a/ddtrace/_trace/span.py +++ b/ddtrace/_trace/span.py @@ -605,6 +605,8 @@ def _local_root(self) -> "Span": 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: