From 32a04b505911be628b0f9c6f44371eaf187f4c71 Mon Sep 17 00:00:00 2001 From: Munir Abdinur Date: Wed, 18 Dec 2024 13:48:25 -0500 Subject: [PATCH] chore(span): make fields immutable after serialization --- ddtrace/_trace/processor/__init__.py | 9 ++- ddtrace/_trace/span.py | 25 ++++++++ ddtrace/_trace/types.py | 35 ++++++++++++ .../djangorestframework/test_appsec.py | 2 - tests/tracer/test_span.py | 57 +++++++++++++++++++ 5 files changed, 124 insertions(+), 4 deletions(-) diff --git a/ddtrace/_trace/processor/__init__.py b/ddtrace/_trace/processor/__init__.py index 3657ee79c39..eef45ed3ae6 100644 --- a/ddtrace/_trace/processor/__init__.py +++ b/ddtrace/_trace/processor/__init__.py @@ -335,16 +335,21 @@ def on_span_finish(self, span: Span) -> None: finished[0].set_metric("_dd.py.partial_flush", num_finished) spans: Optional[List[Span]] = finished + if not spans: + return for tp in self._trace_processors: try: - if spans is None: - return spans = tp.process_trace(spans) except Exception: log.error("error applying processor %r", tp, exc_info=True) self._queue_span_count_metrics("spans_finished", "integration_name") self._writer.write(spans) + # At this point the span object has been sent to the writer for encoding + # and should be considered immutable. Updating the span after this point + # will may not be reflected in the encoded span. + for span in spans: + span._make_immutable() return log.debug("trace %d has %d spans, %d finished", span.trace_id, len(trace.spans), trace.num_finished) diff --git a/ddtrace/_trace/span.py b/ddtrace/_trace/span.py index db90a769cd9..81b0f5f442d 100644 --- a/ddtrace/_trace/span.py +++ b/ddtrace/_trace/span.py @@ -19,6 +19,8 @@ from ddtrace._trace._span_pointer import _SpanPointer from ddtrace._trace._span_pointer import _SpanPointerDirection from ddtrace._trace.context import Context +from ddtrace._trace.types import _ImmutableDict +from ddtrace._trace.types import _ImmutableList from ddtrace._trace.types import _MetaDictType from ddtrace._trace.types import _MetricDictType from ddtrace._trace.types import _TagNameType @@ -108,6 +110,7 @@ class Span(object): "error", "_metrics", "_store", + "_mutable", "span_type", "start_ns", "duration_ns", @@ -176,6 +179,7 @@ def __init__( self._resource = [resource or name] self.span_type = span_type self._span_api = span_api + self._mutable = True self._meta: _MetaDictType = {} self.error = 0 @@ -279,6 +283,27 @@ def duration(self) -> Optional[float]: def duration(self, value: float) -> None: self.duration_ns = int(value * 1e9) + def _make_immutable(self) -> None: + self._meta = _ImmutableDict(self._meta) + self._metrics = _ImmutableDict(self._metrics) + self._store = _ImmutableDict(self._store or {}) + self._meta_struct = _ImmutableDict(self._meta_struct) + self._events = _ImmutableList(self._events) + self._links = _ImmutableList(self._links) + self._mutable = False + + def __setattr__(self, key, value): + if hasattr(self, "_mutable") and not self._mutable: + log.warning( + "Span with the name %s has been serialized and is now immutable, " + "ignoring set attribute %s and value %r", + self.name, + key, + value, + ) + else: + super(Span, self).__setattr__(key, value) + @property def sampled(self) -> Optional[bool]: deprecate( diff --git a/ddtrace/_trace/types.py b/ddtrace/_trace/types.py index 516c94e5b26..4c15de5b286 100644 --- a/ddtrace/_trace/types.py +++ b/ddtrace/_trace/types.py @@ -3,8 +3,43 @@ from typing import Union from ddtrace.internal.compat import NumericType +from ddtrace.internal.logger import get_logger +log = get_logger(__name__) + _TagNameType = Union[Text, bytes] _MetaDictType = Dict[_TagNameType, Text] _MetricDictType = Dict[_TagNameType, NumericType] + + +class _ImmutableDict(dict): + def __setitem__(self, name, value): + # log a warning when trying to set an attribute on an immutable map, do not raise a ValueError + log.warning("Map is immutable, ignoring set attribute %s and value %r", name, value) + + +class _ImmutableList(list): + def __setitem__(self, name, value): + # log a warning when trying to set an attribute on an immutable list, do not raise a ValueError + log.warning("List is immutable, ignoring set attribute %s and value %r", name, value) + + def append(self, value): + # log a warning when trying to append to an immutable list, do not raise a ValueError + log.warning("List is immutable, ignoring append of value %r", value) + + def extend(self, values): + # log a warning when trying to extend an immutable list, do not raise a ValueError + log.warning("List is immutable, ignoring extend with iterable %r", values) + + def insert(self, index, value): + # log a warning when trying to insert into an immutable list, do not raise a ValueError + log.warning("List is immutable, ignoring insert at index %d with value %r", index, value) + + def remove(self, value): + # log a warning when trying to remove from an immutable list, do not raise a ValueError + log.warning("List is immutable, ignoring remove of value %r", value) + + def pop(self, index=-1): + # log a warning when trying to pop from an immutable list, do not raise a ValueError + log.warning("List is immutable, ignoring pop at index %d", index) diff --git a/tests/contrib/djangorestframework/test_appsec.py b/tests/contrib/djangorestframework/test_appsec.py index cf2985f32f3..b7d47630479 100644 --- a/tests/contrib/djangorestframework/test_appsec.py +++ b/tests/contrib/djangorestframework/test_appsec.py @@ -11,7 +11,6 @@ @pytest.mark.skipif(django.VERSION < (1, 10), reason="requires django version >= 1.10") def test_djangorest_request_body_urlencoded(client, test_spans, tracer): with override_global_config(dict(_asm_enabled=True)): - tracer._asm_enabled = True # Hack: need to pass an argument to configure so that the processors are recreated tracer.configure(api_version="v0.4") payload = urlencode({"mytestingbody_key": "mytestingbody_value"}) @@ -29,7 +28,6 @@ def test_djangorest_request_body_urlencoded(client, test_spans, tracer): @pytest.mark.skipif(django.VERSION < (1, 10), reason="requires django version >= 1.10") def test_djangorest_request_body_custom_parser(client, test_spans, tracer): with override_global_config(dict(_asm_enabled=True)): - tracer._asm_enabled = True # Hack: need to pass an argument to configure so that the processors are recreated tracer.configure(api_version="v0.4") payload, content_type = ( diff --git a/tests/tracer/test_span.py b/tests/tracer/test_span.py index e746632037e..d77f20b743c 100644 --- a/tests/tracer/test_span.py +++ b/tests/tracer/test_span.py @@ -874,3 +874,60 @@ def _(span, *exc_info): raise AssertionError("should have raised") finally: core.reset_listeners("span.exception") + + +def test_span_make_immutable(): + """ + Ensure that a span can be made immutable + """ + s = Span( + "test.span", service="s", resource="r", span_type=SpanTypes.WEB, trace_id=1, span_id=2, parent_id=3, start=123 + ) + s.set_tag("a", "a") + s.set_metric("b", 1) + s._add_event("message") + s.set_link(trace_id=11, span_id=12) + s._set_ctx_item("ctx1", "val1") + s._make_immutable() + + s.service = "new-service" + s.resource = "new-resource" + s.span_type = "new-type" + s.trace_id = 10000000 + s.span_id = 20000000 + s.parent_id = 30000000 + s.start = 45600000 + s.set_tag("a", "banana") + s.set_metric("b", 10000000) + s._add_event("message2") + s.set_link(trace_id=1100000, span_id=12000) + s._set_ctx_item("ctx2", "val2") + + assert s.service == "s" + assert s.resource == "r" + assert s.span_type == "web" + assert s.trace_id == 1 + assert s.span_id == 2 + assert s.parent_id == 3 + assert s.start == 123 + assert s.get_tag("a") == "a" + assert s.get_metric("b") == 1 + assert len(s._events) == 1 + assert len(s._links) == 1 + assert len(s._store) == 1 + + +def test_span_make_immutable_warning(): + """ + Ensure that an immutable span logs a warning when trying to set an attribute + """ + with mock.patch("ddtrace._trace.span.log") as log: + s = Span("test.span") + s._make_immutable() + s.name = "new-name" + log.warning.assert_called_once_with( + "Span with the name %s has been serialized and is now immutable, ignoring set attribute %s and value %r", + "test.span", + "name", + "new-name", + )