fix(tracer): use RLOCK in span aggregator (#6325)

This fix resolves an issue where sqlalchemy rollbacks could
intermittently cause deadlocks due SpanAggregator's `on_span_start`
using a Lock. This fix was put behind a flag
`DD_TRACE_SPAN_AGGREGATOR_RLOCK` which defaults to false due to possible
performance impacts. While our benchmarks don't show anything major, and
we suspect this change will not impact performance much at all, we want
to be cautious and test on internal services before making it true by
default.

We plan on gathering feedback and testing on internal services, as long
as we don't see a negative impact, we plan on making this the new
default.

Should fix: #4978


## Checklist

- [x] Change(s) are motivated and described in the PR description.
- [x] Testing strategy is described if automated tests are not included
in the PR.
- [x] Risk is outlined (performance impact, potential for breakage,
maintainability, etc).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed. If no release note is required, add label
`changelog/no-changelog`.
- [x] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/)).
- [x] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist

- [x] Title is accurate.
- [x] No unnecessary changes are introduced.
- [x] Description motivates each change.
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes unless absolutely necessary.
- [x] Testing strategy adequately addresses listed risk(s).
- [x] Change is maintainable (easy to change, telemetry, documentation).
- [x] Release note makes sense to a user of the library.
- [x] Reviewer has explicitly acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment.
- [x] 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: Brett Langdon <[email protected]>

Author: ZStriker19

ddtrace/internal/processor/trace.py

@@ -1,10 +1,12 @@
 import abc
 from collections import defaultdict
-import threading
+from threading import Lock
+from threading import RLock
 from typing import Dict
 from typing import Iterable
 from typing import List
 from typing import Optional
+from typing import Union
 
 import attr
 import six
@@ -180,7 +182,10 @@ class _Trace(object):
         type=DefaultDict[int, "_Trace"],
         repr=False,
     )
-    _lock = attr.ib(init=False, factory=threading.Lock, repr=False)
+    if config._span_aggregator_rlock:
+        _lock = attr.ib(init=False, factory=RLock, repr=False, type=Union[RLock, Lock])
+    else:
+        _lock = attr.ib(init=False, factory=Lock, repr=False, type=Union[RLock, Lock])
     # Tracks the number of spans created and tags each count with the api that was used
     # ex: otel api, opentracing api, datadog api
     _span_metrics = attr.ib(

ddtrace/settings/config.py

@@ -320,6 +320,7 @@ def __init__(self):
                 self._propagation_style_inject.append(_PROPAGATION_STYLE_W3C_TRACECONTEXT)
 
         self._ddtrace_bootstrapped = False
+        self._span_aggregator_rlock = asbool(os.getenv("DD_TRACE_SPAN_AGGREGATOR_RLOCK", False))
 
     def __getattr__(self, name):
         if name not in self._config:

docs/configuration.rst

@@ -413,6 +413,11 @@ The following environment variables for the tracer are supported:
      type: Boolean
      default: True
      description: Send query strings in http.url tag in http server integrations.
+    
+   DD_TRACE_SPAN_AGGREGATOR_RLOCK:
+     type: Boolean
+     default: False
+     description: Whether the ``SpanAggregator`` should use an RLock or a Lock.
 
    DD_IAST_ENABLED:
      type: Boolean

releasenotes/notes/add_flag_for_span_aggregator_rlock-b08c88e0da85dc66.yaml

@@ -0,0 +1,6 @@
+---
+issues:
+  - |
+    sqlalchemy: sqlalchemy rollbacks can intermittently cause deadlocks in some cases.
+    If experiencing this issue, set ``DD_TRACE_SPAN_AGGREGATOR_RLOCK=True``. 
+    After testing and feedback we intend to make True the default value.

tests/contrib/gunicorn/test_gunicorn.py

@@ -57,6 +57,7 @@ def _gunicorn_settings_factory(
     debug_mode=False,  # type: bool
     dd_service=None,  # type: Optional[str]
     schema_version=None,  # type: Optional[str]
+    rlock=False,  # type: bool
 ):
     # type: (...) -> GunicornServerSettings
     """Factory for creating gunicorn settings with simple defaults if settings are not defined."""
@@ -73,6 +74,8 @@ def _gunicorn_settings_factory(
         env["DD_SERVICE"] = dd_service
     if schema_version is not None:
         env["DD_TRACE_SPAN_ATTRIBUTE_SCHEMA"] = schema_version
+    if rlock is not None:
+        env["DD_TRACE_SPAN_AGGREGATOR_RLOCK"] = "true"
     return GunicornServerSettings(
         env=env,
         directory=directory,
@@ -164,6 +167,12 @@ def gunicorn_server(gunicorn_server_settings, tmp_path):
     debug_mode=True,
     enable_module_cloning=True,
 )
+SETTINGS_GEVENT_SPANAGGREGATOR_RLOCK = _gunicorn_settings_factory(
+    worker_class="gevent",
+    use_ddtracerun=False,
+    import_auto_in_app=True,
+    rlock=True,
+)
 
 
 @pytest.mark.skipif(sys.version_info >= (3, 11), reason="Gunicorn is only supported up to 3.10")
@@ -175,6 +184,7 @@ def gunicorn_server(gunicorn_server_settings, tmp_path):
         SETTINGS_GEVENT_DDTRACERUN,
         SETTINGS_GEVENT_DDTRACERUN_MODULE_CLONE,
         SETTINGS_GEVENT_DDTRACERUN_DEBUGMODE_MODULE_CLONE,
+        SETTINGS_GEVENT_SPANAGGREGATOR_RLOCK,
     ],
 )
 def test_no_known_errors_occur(gunicorn_server_settings, tmp_path):