feat(tracer): deprecate escaped and timestamp attributes in record_exception API (#13798)

# Motivation

`record_exception` call was added few releases ago because some
customers were asking to be able to send manually errors. However, it
was mainly based on the OpenTelemetry implementation. The OTEL sdk are
inconsistent between tracers and some implementations are using for
example `escaped` attribute which does not make sense at all for us.
An
[RFC](https://docs.google.com/document/d/1-7CsG8GZVnNU198FfzavCNMI6IF0cGzFfS8cIz6Pu1g/edit?usp=sharing)
was written to ease the implementation across tracer. This PR is there
to make the python implementation consistent with the RFC.

# Changes
- Deprecates `escaped` and `timestamp` attributes
- Add a validation steps for attributes. If an attribute has not one of
the typed defined in this
[RFC](https://docs.google.com/document/d/1cVod_VI7Yruq8U9dfMRFJd7npDu-uBpste2IB04GyaQ/edit?tab=t.0#heading=h.z8rsw69ccced),
the span event will be dropped. Without span validation, it would be
"shadow" dropped which could make the user think the feature is not
working.
- Improves `record_exception` doc.

## 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)

Author: dubloom

ddtrace/_trace/span.py

@@ -47,12 +47,16 @@
 from ddtrace.internal.compat import NumericType
 from ddtrace.internal.compat import ensure_text
 from ddtrace.internal.compat import is_integer
+from ddtrace.internal.constants import MAX_INT_64BITS as _MAX_INT_64BITS
 from ddtrace.internal.constants import MAX_UINT_64BITS as _MAX_UINT_64BITS
+from ddtrace.internal.constants import MIN_INT_64BITS as _MIN_INT_64BITS
 from ddtrace.internal.constants import SPAN_API_DATADOG
 from ddtrace.internal.logger import get_logger
 from ddtrace.internal.sampling import SamplingMechanism
 from ddtrace.internal.sampling import set_sampling_decision_maker
+from ddtrace.internal.utils.deprecations import DDTraceDeprecationWarning
 from ddtrace.settings._config import config
+from ddtrace.vendor.debtcollector import deprecate
 
 
 class SpanEvent:
@@ -604,42 +608,93 @@ def record_exception(
         exception: BaseException,
         attributes: Optional[Dict[str, _AttributeValueType]] = None,
         timestamp: Optional[int] = None,
-        escaped=False,
+        escaped: bool = False,
     ) -> None:
         """
-        Records an exception as span event.
-        If the exception is uncaught, :obj:`escaped` should be set :obj:`True`. It
-        will tag the span with an error tuple.
-
-        :param Exception exception: the exception to record
-        :param dict attributes: optional attributes to add to the span event. It will override
-            the base attributes if :obj:`attributes` contains existing keys.
-        :param int timestamp: the timestamp of the span event. Will be set to now() if timestamp is :obj:`None`.
-        :param bool escaped: sets to :obj:`False` for a handled exception and :obj:`True` for a uncaught exception.
+        Records an exception as a span event. Multiple exceptions can be recorded on a span.
+
+        :param exception: The exception to record.
+        :param attributes: Optional dictionary of additional attributes to add to the exception event.
+            These attributes will override the default exception attributes if they contain the same keys.
+            Valid attribute values include (homogeneous array of) strings, booleans, integers, floats.
+        :param timestamp: Deprecated.
+        :param escaped: Deprecated.
         """
-        if timestamp is None:
-            timestamp = time_ns()
-
-        exc_type, exc_val, exc_tb = type(exception), exception, exception.__traceback__
-
         if escaped:
-            self.set_exc_info(exc_type, exc_val, exc_tb)
+            deprecate(
+                prefix="The escaped argument is deprecated for record_exception",
+                message="""If an exception exits the scope of the span, it will automatically be
+                reported in the span tags.""",
+                category=DDTraceDeprecationWarning,
+                removal_version="4.0.0",
+            )
+        if timestamp is not None:
+            deprecate(
+                prefix="The timestamp argument is deprecated for record_exception",
+                message="""The timestamp of the span event should correspond to the time when the
+                error is recorded which is set automatically.""",
+                category=DDTraceDeprecationWarning,
+                removal_version="4.0.0",
+            )
 
-        tb = self._get_traceback(exc_type, exc_val, exc_tb)
+        tb = self._get_traceback(type(exception), exception, exception.__traceback__)
 
-        # Set exception attributes in a manner that is consistent with the opentelemetry sdk
-        # https://github.com/open-telemetry/opentelemetry-python/blob/v1.24.0/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L998
-        attrs = {
+        attrs: Dict[str, _AttributeValueType] = {
             "exception.type": "%s.%s" % (exception.__class__.__module__, exception.__class__.__name__),
             "exception.message": str(exception),
-            "exception.escaped": escaped,
             "exception.stacktrace": tb,
         }
         if attributes:
+            attributes = {k: v for k, v in attributes.items() if self._validate_attribute(k, v)}
+
             # User provided attributes must take precedence over attrs
             attrs.update(attributes)
 
-        self._add_event(name="exception", attributes=attrs, timestamp=timestamp)
+        self._add_event(name="exception", attributes=attrs, timestamp=time_ns())
+
+    def _validate_attribute(self, key: str, value: object) -> bool:
+        if isinstance(value, (str, bool, int, float)):
+            return self._validate_scalar(key, value)
+
+        if not isinstance(value, list):
+            log.warning("record_exception: Attribute %s must be a string, number, or boolean: %s.", key, value)
+            return False
+
+        if len(value) == 0:
+            return True
+
+        if not isinstance(value[0], (str, bool, int, float)):
+            log.warning("record_exception: List values %s must be string, number, or boolean: %s.", key, value)
+            return False
+
+        first_type = type(value[0])
+        for val in value:
+            if not isinstance(val, first_type) or not self._validate_scalar(key, val):
+                log.warning("record_exception: Attribute %s array must be homogenous: %s.", key, value)
+                return False
+        return True
+
+    def _validate_scalar(self, key: str, value: Union[bool, str, int, float]) -> bool:
+        if isinstance(value, (bool, str)):
+            return True
+
+        if isinstance(value, int):
+            if value < _MIN_INT_64BITS or value > _MAX_INT_64BITS:
+                log.warning(
+                    "record_exception: Attribute %s must be within the range of a signed 64-bit integer: %s.",
+                    key,
+                    value,
+                )
+                return False
+            return True
+
+        if isinstance(value, float):
+            if not math.isfinite(value):
+                log.warning("record_exception: Attribute %s must be a finite number: %s.", key, value)
+                return False
+            return True
+
+        return False
 
     def _pprint(self) -> str:
         """Return a human readable version of the span."""

ddtrace/internal/constants.py

@@ -40,6 +40,8 @@
 COMPONENT = "component"
 HIGHER_ORDER_TRACE_ID_BITS = "_dd.p.tid"
 MAX_UINT_64BITS = (1 << 64) - 1
+MIN_INT_64BITS = -(2**63)
+MAX_INT_64BITS = 2**63 - 1
 SPAN_LINKS_KEY = "_dd.span_links"
 SPAN_EVENTS_KEY = "events"
 SPAN_API_DATADOG = "datadog"

releasenotes/notes/record-exception-deprecate-arguments-c82d62d15485e60a.yaml

@@ -0,0 +1,4 @@
+---
+deprecations:
+  - |
+    tracing: The ``escaped`` and ``timestamp`` arguments in the ``record_exception`` method are deprecated and will be removed in version 4.0.0.

tests/tracer/test_span.py

@@ -554,9 +554,7 @@ def test_span_record_exception(self):
         span.finish()
 
         span.assert_span_event_count(1)
-        span.assert_span_event_attributes(
-            0, {"exception.type": "builtins.RuntimeError", "exception.message": "bim", "exception.escaped": False}
-        )
+        span.assert_span_event_attributes(0, {"exception.type": "builtins.RuntimeError", "exception.message": "bim"})
 
     def test_span_record_multiple_exceptions(self):
         span = self.start_span("span")
@@ -572,33 +570,49 @@ def test_span_record_multiple_exceptions(self):
         span.finish()
 
         span.assert_span_event_count(2)
+        span.assert_span_event_attributes(0, {"exception.type": "builtins.RuntimeError", "exception.message": "bim"})
+        span.assert_span_event_attributes(1, {"exception.type": "builtins.RuntimeError", "exception.message": "bam"})
+
+    def test_span_record_exception_with_attributes(self):
+        span = self.start_span("span")
+        try:
+            raise RuntimeError("bim")
+        except RuntimeError as e:
+            span.record_exception(e, {"foo": "bar"})
+        span.finish()
+
+        span.assert_span_event_count(1)
         span.assert_span_event_attributes(
-            0, {"exception.type": "builtins.RuntimeError", "exception.message": "bim", "exception.escaped": False}
-        )
-        span.assert_span_event_attributes(
-            1, {"exception.type": "builtins.RuntimeError", "exception.message": "bam", "exception.escaped": False}
+            0, {"exception.type": "builtins.RuntimeError", "exception.message": "bim", "foo": "bar"}
         )
 
-    def test_span_record_escaped_exception(self):
-        exc = RuntimeError("bim")
+    @mock.patch("ddtrace._trace.span.log")
+    def test_span_record_exception_with_invalid_attributes(self, span_log):
         span = self.start_span("span")
         try:
-            raise exc
+            raise RuntimeError("bim")
         except RuntimeError as e:
-            span.record_exception(e, escaped=True)
+            span.record_exception(
+                e, {"foo": "bar", "toto": ["titi", 1], "bim": 2**100, "tata": [[1]], "tutu": {"a": "b"}}
+            )
         span.finish()
 
-        span.assert_matches(
-            error=1,
-            meta={
-                "error.message": str(exc),
-                "error.type": "%s.%s" % (exc.__class__.__module__, exc.__class__.__name__),
-            },
-        )
         span.assert_span_event_count(1)
         span.assert_span_event_attributes(
-            0, {"exception.type": "builtins.RuntimeError", "exception.message": "bim", "exception.escaped": True}
+            0, {"exception.type": "builtins.RuntimeError", "exception.message": "bim", "foo": "bar"}
         )
+        expected_calls = [
+            mock.call("record_exception: Attribute %s must be a string, number, or boolean: %s.", "tutu", {"a": "b"}),
+            mock.call("record_exception: Attribute %s array must be homogenous: %s.", "toto", ["titi", 1]),
+            mock.call("record_exception: List values %s must be string, number, or boolean: %s.", "tata", [[1]]),
+            mock.call(
+                "record_exception: Attribute %s must be within the range of a signed 64-bit integer: %s.",
+                "bim",
+                2**100,
+            ),
+        ]
+        span_log.warning.assert_has_calls(expected_calls, any_order=True)
+        assert span_log.warning.call_count == 4
 
 
 @pytest.mark.parametrize(