Apply changes requested in code review

Signed-off-by: Israel Blancas <[email protected]>

Author: iblancasa

opentelemetry-api/src/opentelemetry/_logs/_internal/__init__.py

@@ -95,7 +95,6 @@ def __init__(
         severity_number: Optional[SeverityNumber] = None,
         body: AnyValue = None,
         attributes: Optional[_ExtendedAttributes] = None,
-        exception: Optional[BaseException] = None,
     ) -> None: ...
 
     def __init__(
@@ -298,7 +297,7 @@ def emit(
         exception: BaseException | None = None,
     ) -> None:
         if record:
-            self._logger.emit(record)
+            self._logger.emit(record, exception=exception)
         else:
             self._logger.emit(
                 timestamp=timestamp,

opentelemetry-api/tests/logs/test_proxy.py

@@ -15,6 +15,7 @@
 # pylint: disable=W0212,W0222,W0221
 import typing
 import unittest
+from unittest.mock import Mock
 
 import opentelemetry._logs._internal as _logs_internal
 from opentelemetry import _logs
@@ -75,3 +76,15 @@ def test_proxy_logger(self):
         # references to the old provider still work but return real logger now
         real_logger = provider.get_logger("proxy-test")
         self.assertIsInstance(real_logger, LoggerTest)
+
+    def test_proxy_logger_forwards_exception_with_record(self):
+        logger = _logs_internal.ProxyLogger("proxy-test")
+        logger._real_logger = Mock(spec=LoggerTest("proxy-test"))
+        record = _logs.LogRecord()
+        exception = ValueError("boom")
+
+        logger.emit(record, exception=exception)
+
+        logger._real_logger.emit.assert_called_once_with(
+            record, exception=exception
+        )

opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py

@@ -534,28 +534,50 @@ def _get_exception_attributes(
     }
 
 
-def _apply_exception_attributes(
-    log_record: LogRecord,
+def _get_attributes_with_exception(
+    attributes: _ExtendedAttributes | None,
     exception: BaseException | None,
-) -> None:
+) -> _ExtendedAttributes | None:
     if exception is None:
-        return
+        return attributes
 
     exception_attributes_map = _get_exception_attributes(exception)
-    attributes = log_record.attributes
-    if attributes:
-        if isinstance(attributes, BoundedAttributes):
-            for key, value in exception_attributes_map.items():
-                if key not in attributes:
-                    attributes[key] = value
-            return
-        merged = dict(attributes)
+    attributes = attributes or {}
+    if isinstance(attributes, BoundedAttributes):
+        merged = BoundedAttributes(
+            maxlen=attributes.maxlen,
+            attributes=attributes,
+            immutable=False,
+            max_value_len=attributes.max_value_len,
+            extended_attributes=attributes._extended_attributes,  # pylint: disable=protected-access
+        )
+        merged.dropped = attributes.dropped
         for key, value in exception_attributes_map.items():
-            merged.setdefault(key, value)
-        log_record.attributes = merged
-        return
-
-    log_record.attributes = exception_attributes_map
+            if key not in merged:
+                merged[key] = value
+        return merged
+
+    return exception_attributes_map | dict(attributes)
+
+
+def _copy_log_record(
+    record: LogRecord,
+    attributes: _ExtendedAttributes | None,
+) -> LogRecord:
+    return LogRecord(
+        timestamp=record.timestamp,
+        observed_timestamp=record.observed_timestamp,
+        context=record.context,
+        trace_id=record.trace_id,
+        span_id=record.span_id,
+        trace_flags=record.trace_flags,
+        severity_text=record.severity_text,
+        severity_number=record.severity_number,
+        body=record.body,
+        attributes=attributes,
+        event_name=record.event_name,
+        exception=getattr(record, "exception", None),
+    )
 
 
 class LoggingHandler(logging.Handler):
@@ -725,32 +747,40 @@ def emit(
                     record.log_record, "exception", None
                 )
             if not isinstance(record, ReadWriteLogRecord):
-                _apply_exception_attributes(record, record_exception)
+                if record_exception is not None:
+                    record = _copy_log_record(
+                        record,
+                        _get_attributes_with_exception(
+                            record.attributes, record_exception
+                        ),
+                    )
                 # pylint:disable=protected-access
                 writable_record = ReadWriteLogRecord._from_api_log_record(
                     record=record,
                     resource=self._resource,
                     instrumentation_scope=self._instrumentation_scope,
                 )
             else:
-                _apply_exception_attributes(
-                    record.log_record, record_exception
+                record.log_record.attributes = _get_attributes_with_exception(
+                    record.log_record.attributes, record_exception
                 )
                 writable_record = record
         else:
             # Create a record from individual parameters
+            log_record_attributes = _get_attributes_with_exception(
+                attributes, exception
+            )
             log_record = LogRecord(
                 timestamp=timestamp,
                 observed_timestamp=observed_timestamp,
                 context=context,
                 severity_number=severity_number,
                 severity_text=severity_text,
                 body=body,
-                attributes=attributes,
+                attributes=log_record_attributes,
                 event_name=event_name,
                 exception=exception,
             )
-            _apply_exception_attributes(log_record, exception)
             # pylint:disable=protected-access
             writable_record = ReadWriteLogRecord._from_api_log_record(
                 record=log_record,

opentelemetry-sdk/tests/logs/test_logs.py

@@ -18,6 +18,7 @@
 from unittest.mock import Mock, patch
 
 from opentelemetry._logs import LogRecord, SeverityNumber
+from opentelemetry.attributes import BoundedAttributes
 from opentelemetry.context import get_current
 from opentelemetry.sdk._logs import (
     Logger,
@@ -236,6 +237,22 @@ def test_emit_with_exception_adds_attributes(self):
             attributes[exception_attributes.EXCEPTION_STACKTRACE],
         )
 
+    def test_emit_with_raised_exception_has_stacktrace(self):
+        logger, log_record_processor_mock = self._get_logger()
+
+        try:
+            raise ValueError("boom")
+        except ValueError as exc:
+            logger.emit(body="error", exception=exc)
+
+        log_record_processor_mock.on_emit.assert_called_once()
+        log_data = log_record_processor_mock.on_emit.call_args.args[0]
+        stacktrace = dict(log_data.log_record.attributes)[
+            exception_attributes.EXCEPTION_STACKTRACE
+        ]
+        self.assertIn("Traceback (most recent call last)", stacktrace)
+        self.assertIn("raise ValueError", stacktrace)
+
     def test_emit_logrecord_exception_preserves_user_attributes(self):
         logger, log_record_processor_mock = self._get_logger()
         exc = ValueError("boom")
@@ -257,6 +274,34 @@ def test_emit_logrecord_exception_preserves_user_attributes(self):
             attributes[exception_attributes.EXCEPTION_MESSAGE], "boom"
         )
 
+    def test_emit_logrecord_exception_with_immutable_attributes(self):
+        logger, log_record_processor_mock = self._get_logger()
+        exc = ValueError("boom")
+        original_attributes = BoundedAttributes(
+            attributes={"custom": "value"},
+            immutable=True,
+            extended_attributes=True,
+        )
+        log_record = LogRecord(
+            observed_timestamp=0,
+            body="a log line",
+            attributes=original_attributes,
+            exception=exc,
+        )
+
+        logger.emit(log_record)
+
+        self.assertNotIn(
+            exception_attributes.EXCEPTION_TYPE, log_record.attributes
+        )
+        log_record_processor_mock.on_emit.assert_called_once()
+        log_data = log_record_processor_mock.on_emit.call_args.args[0]
+        attributes = dict(log_data.log_record.attributes)
+        self.assertEqual(attributes["custom"], "value")
+        self.assertEqual(
+            attributes[exception_attributes.EXCEPTION_TYPE], "ValueError"
+        )
+
     def test_emit_readwrite_logrecord_uses_exception(self):
         logger, log_record_processor_mock = self._get_logger()
         exc = RuntimeError("kaput")