Fix memory leak when the tracer is disabled (#2964)

* Fix memory leak when the tracer is disabled

The SpanAggregator will store spans in a list when on_span_start
is called. When a span is finished on_span_finish is called and
if the trace is complete the SpanAggregator will remove all of
the traces spans from the list and give them to the writer.

However, when the tracer is disabled it never calls on_span_finish
so we never get an opportunity to remove a finished span from
memory when it is finished because on_span_finish is never called.

The fix here is to update the logic for on_span_start to only be
called if the tracer is enabled.

* Add test case for memory leak

* fix up test

Author: brettlangdon

ddtrace/tracer.py

@@ -630,8 +630,10 @@ def _start_span(
         if service and service not in self._services and self._is_span_internal(span):
             self._services.add(service)
 
-        for p in self._span_processors:
-            p.on_span_start(span)
+        # Only call span processors if the tracer is enabled
+        if self.enabled:
+            for p in self._span_processors:
+                p.on_span_start(span)
 
         self._hooks.emit(self.__class__.start_span, span)
         return span
@@ -648,11 +650,10 @@ def _on_span_finish(self, span):
                 "span %r closing after its parent %r, this is an error when not using async", span, span._parent
             )
 
-        if not self.enabled:
-            return  # nothing to do
-
-        for p in self._span_processors:
-            p.on_span_finish(span)
+        # Only call span processors if the tracer is enabled
+        if self.enabled:
+            for p in self._span_processors:
+                p.on_span_finish(span)
 
         if self.log.isEnabledFor(logging.DEBUG):
             self.log.debug("finishing span %s (enabled:%s)", span.pprint(), self.enabled)

releasenotes/notes/fix-disabled-memory-leak-c793801e11c45bfe.yaml

@@ -0,0 +1,4 @@
+---
+fixes:
+  - |
+    Fix memory leak caused when the tracer is disabled.

tests/tracer/test_tracer.py

@@ -3,12 +3,14 @@
 tests for Tracer and utilities.
 """
 import contextlib
+import gc
 import multiprocessing
 import os
 from os import getpid
 import threading
 from unittest.case import SkipTest
 import warnings
+import weakref
 
 import mock
 import pytest
@@ -1706,3 +1708,35 @@ def test_tracer_api_version():
 
     t.configure(api_version="v0.4")
     assert isinstance(t.writer._encoder, MsgpackEncoderV03)
+
+
+@pytest.mark.parametrize("enabled", [True, False])
+def test_tracer_memory_leak_span_processors(enabled):
+    """
+    Test whether the tracer or span processors will hold onto
+    span references after the trace is complete.
+
+    This is a regression test for the tracer not calling on_span_finish
+    of SpanAggregator when the tracer was disabled and traces leaking.
+    """
+    spans = weakref.WeakSet()
+
+    # Filter to ensure we don't send the traces to the writer
+    class DropAllFilter:
+        def process_trace(self, trace):
+            return None
+
+    t = Tracer()
+    t.enabled = enabled
+    t.configure(settings={"FILTERS": [DropAllFilter()]})
+
+    for _ in range(5):
+        with t.trace("test") as span:
+            spans.add(span)
+
+    # Be sure to dereference the last Span held by the local variable `span`
+    span = None
+
+    # Force gc
+    gc.collect()
+    assert len(spans) == 0