Add logger exception support for logs API/SDK#4908
Add logger exception support for logs API/SDK#4908xrmx merged 35 commits intoopen-telemetry:mainfrom
Conversation
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
|
Haven't looked in detail at the PR but we are usually cautious on adding in development stuff to the api / sdk. |
|
hey @xrmx! check out open-telemetry/opentelemetry-specification#4886 if you can give this PR a basic review and "lgtm" without approving/merging it, then we can treat it as a "qualifying prototype" for marking it stable in the spec. we already have 3 qualifying prototypes which is the min bar for stabilizing the spec, but the more the better! |
Fixes #4858 Implementations / Prototypes: - Java: [`ExtendedLogRecordBuilder.setException(Throwable)`](https://github.com/open-telemetry/opentelemetry-java/blob/main/api/incubator/src/main/java/io/opentelemetry/api/incubator/logs/ExtendedLogRecordBuilder.java#L137) - Builder method called before `emit()` - Merged and available to users - .NET: [`LogRecordAttributeList.RecordException(Exception)`](https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry.Api/Logs/LogRecordAttributeList.cs#L191-L198) - Attribute list method called before `EmitLog()` - JavaScript: [opentelemetry-js#6385](open-telemetry/opentelemetry-js#6385) - Field on LogRecord passed to `emit()` - Python: [opentelemetry-python#4908](open-telemetry/opentelemetry-python#4908) - Parameter on LogRecord passed to `emit()` (only the Python prototype doesn't yet meet the definition of [qualifying prototype](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CONTRIBUTING.md#proposing-a-change) yet) Co-authored-by: Robert Pająk <pellared@hotmail.com>
Missed this comment but already approved the other PR 😅 |
nagkumar91
left a comment
There was a problem hiding this comment.
Nice implementation — spec compliance looks solid. A couple of nits below.
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
|
Looks like there are some merge conflicts that need fixing - otherwise I think this is ready for review / merge by a @open-telemetry/python-maintainers 👍🏻 |
… into 4907 # Conflicts: # CHANGELOG.md
xrmx
left a comment
There was a problem hiding this comment.
With something like this, tests should still pass:
diff --git a/opentelemetry-api/src/opentelemetry/_logs/_internal/__init__.py b/opentelemetry-api/src/opentelemetry/_logs/_internal/__init__.py
index 6264b6eca..12a8cc08f 100644
--- a/opentelemetry-api/src/opentelemetry/_logs/_internal/__init__.py
+++ b/opentelemetry-api/src/opentelemetry/_logs/_internal/__init__.py
@@ -167,8 +167,6 @@ class Logger(ABC):
def emit(
self,
record: LogRecord,
- *,
- exception: BaseException | None = None,
) -> None: ...
@abstractmethod
@@ -214,8 +212,6 @@ class NoOpLogger(Logger):
def emit( # pylint:disable=arguments-differ
self,
record: LogRecord,
- *,
- exception: BaseException | None = None,
) -> None: ...
def emit(
@@ -284,8 +280,6 @@ class ProxyLogger(Logger):
def emit( # pylint:disable=arguments-differ
self,
record: LogRecord,
- *,
- exception: BaseException | None = None,
) -> None: ...
def emit(
@@ -303,7 +297,7 @@ class ProxyLogger(Logger):
exception: BaseException | None = None,
) -> None:
if record:
- self._logger.emit(record, exception=exception)
+ self._logger.emit(record)
else:
self._logger.emit(
timestamp=timestamp,
diff --git a/opentelemetry-api/tests/logs/test_proxy.py b/opentelemetry-api/tests/logs/test_proxy.py
index f7744e5f3..1b5d0c22c 100644
--- a/opentelemetry-api/tests/logs/test_proxy.py
+++ b/opentelemetry-api/tests/logs/test_proxy.py
@@ -77,15 +77,12 @@ class TestProxy(LoggingGlobalsTest, unittest.TestCase):
real_logger = provider.get_logger("proxy-test")
self.assertIsInstance(real_logger, LoggerTest)
- def test_proxy_logger_forwards_exception_with_record(self):
+ def test_proxy_logger_forwards_record_with_exception(self):
logger = _logs_internal.ProxyLogger("proxy-test")
logger._real_logger = Mock(spec=LoggerTest("proxy-test"))
- record = _logs.LogRecord()
- exception = ValueError("boom")
+ record = _logs.LogRecord(exception=ValueError("boom"))
self.assertIsNotNone(logger._real_logger)
- logger.emit(record, exception=exception)
+ logger.emit(record)
- logger._real_logger.emit.assert_called_once_with(
- record, exception=exception
- )
+ logger._real_logger.emit.assert_called_once_with(record)
diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py
index 3633a308c..4ed043e67 100644
--- a/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py
+++ b/opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py
@@ -793,19 +793,12 @@ class Logger(APILogger):
return
# If a record is provided, use it directly
if record is not None:
- record_exception = exception or getattr(record, "exception", None)
- if record_exception is None and isinstance(
- record, ReadWriteLogRecord
- ):
- record_exception = getattr(
- record.log_record, "exception", None
- )
if not isinstance(record, ReadWriteLogRecord):
- if record_exception is not None:
+ if record.exception is not None:
record = _copy_log_record(
record,
_get_attributes_with_exception(
- record.attributes, record_exception
+ record.attributes, record.exception
),
)
# pylint:disable=protected-access
@@ -816,7 +809,7 @@ class Logger(APILogger):
)
else:
record.log_record.attributes = _get_attributes_with_exception(
- record.log_record.attributes, record_exception
+ record.log_record.attributes, record.log_record.exception
)
writable_record = record
else:
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
* Add logger exception support for logs API/SDK Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Apply changes requested in code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix CI Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix ci Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Apply feedback from code review Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix lint Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Remove unrelated entry from changelog Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix lint Signed-off-by: Israel Blancas <iblancasa@gmail.com> * Fix lint Signed-off-by: Israel Blancas <iblancasa@gmail.com> --------- Signed-off-by: Israel Blancas <iblancasa@gmail.com> Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com> Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com>
Description
Fixes #4907
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Contrib Repo Change?
Checklist: