From ba2b6e99c74f0b3823cf7a9bdff51fa11147854c Mon Sep 17 00:00:00 2001 From: Utkarsh Date: Fri, 31 Jul 2026 16:50:57 +0530 Subject: [PATCH 1/2] fix(logging): do not unlink log record factories chained after ours _uninstrument restored _old_factory unconditionally. Python's log record factory is a single global slot that callers chain by closing over the previous value, so any factory installed after LoggingInstrumentor was silently cut out of the chain. Track the factory we installed and only restore while we are still the head of the chain. When something has been chained on top, leave the chain alone and warn: a node cannot be removed from the middle without the cooperation of the factory that wrapped it. Fixes #3808 --- .../instrumentation/logging/__init__.py | 27 ++++++++++++-- .../tests/test_logging.py | 35 +++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/instrumentation/opentelemetry-instrumentation-logging/src/opentelemetry/instrumentation/logging/__init__.py b/instrumentation/opentelemetry-instrumentation-logging/src/opentelemetry/instrumentation/logging/__init__.py index 93087e9867..957d14d337 100644 --- a/instrumentation/opentelemetry-instrumentation-logging/src/opentelemetry/instrumentation/logging/__init__.py +++ b/instrumentation/opentelemetry-instrumentation-logging/src/opentelemetry/instrumentation/logging/__init__.py @@ -134,6 +134,7 @@ def log_hook(span: Span, record: LogRecord): """ _old_factory = None + _our_factory = None _log_hook = None _logging_handler = None @@ -217,6 +218,7 @@ def record_factory(*args, **kwargs): return record + LoggingInstrumentor._our_factory = record_factory logging.setLogRecordFactory(record_factory) # Here we need to handle 3 scenarios: @@ -265,9 +267,28 @@ def record_factory(*args, **kwargs): LoggingInstrumentor._logging_handler = handler def _uninstrument(self, **kwargs): - if LoggingInstrumentor._old_factory: - logging.setLogRecordFactory(LoggingInstrumentor._old_factory) - LoggingInstrumentor._old_factory = None + # `logging.setLogRecordFactory` is a single global slot that callers + # chain by closing over whatever factory preceded them. Restoring + # `_old_factory` unconditionally would therefore unlink every factory + # installed after ours, so only restore while we are still the head of + # the chain. Otherwise we leave the chain alone: a node cannot be + # removed from the middle without the cooperation of the factory that + # wrapped it. + if LoggingInstrumentor._our_factory is not None: + if ( + logging.getLogRecordFactory() + is LoggingInstrumentor._our_factory + ): + logging.setLogRecordFactory(LoggingInstrumentor._old_factory) + else: + _logger.warning( + "Another log record factory was installed after " + "LoggingInstrumentor. Leaving the log record factory chain " + "untouched to avoid unlinking it; log records may continue " + "to carry OpenTelemetry attributes." + ) + LoggingInstrumentor._old_factory = None + LoggingInstrumentor._our_factory = None if LoggingInstrumentor._logging_handler: logging.getLogger().removeHandler( diff --git a/instrumentation/opentelemetry-instrumentation-logging/tests/test_logging.py b/instrumentation/opentelemetry-instrumentation-logging/tests/test_logging.py index 7ad851d3fa..b4cea0bded 100644 --- a/instrumentation/opentelemetry-instrumentation-logging/tests/test_logging.py +++ b/instrumentation/opentelemetry-instrumentation-logging/tests/test_logging.py @@ -282,6 +282,41 @@ def test_uninstrumented(self): ] self.assertEqual(logging_handler_instances, []) + def test_uninstrument_keeps_factories_chained_after_ours(self): + # A factory installed after ours must survive uninstrument: the + # logging module offers no way to unlink a factory from the middle of + # the chain, so restoring the old factory here would silently drop it. + chained_onto = logging.getLogRecordFactory() + + def app_factory(*args, **kwargs): + record = chained_onto(*args, **kwargs) + record.custom_app_attribute = "some-value" + return record + + logging.setLogRecordFactory(app_factory) + try: + LoggingInstrumentor().uninstrument() + + self.assertIs(logging.getLogRecordFactory(), app_factory) + with self.caplog.at_level(level=logging.INFO): + logging.getLogger("test logger").info("hello") + records = [ + record + for record in self.caplog.records + if record.name == "test logger" + ] + self.assertEqual(len(records), 1) + self.assertEqual(records[0].custom_app_attribute, "some-value") + finally: + logging.setLogRecordFactory(chained_onto) + + def test_uninstrument_restores_factory_when_nothing_chained(self): + original_factory = LoggingInstrumentor._old_factory + + LoggingInstrumentor().uninstrument() + + self.assertIs(logging.getLogRecordFactory(), original_factory) + @mock.patch("logging.basicConfig") def test_no_op_tracer_provider(self, basic_config_mock): LoggingInstrumentor().uninstrument() From 20fa1bfda2b37fe32cde762f258211dd96cbf87d Mon Sep 17 00:00:00 2001 From: Utkarsh Date: Fri, 31 Jul 2026 16:54:09 +0530 Subject: [PATCH 2/2] Add changelog entry for logging uninstrument fix --- .changelog/4905.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 .changelog/4905.fixed diff --git a/.changelog/4905.fixed b/.changelog/4905.fixed new file mode 100644 index 0000000000..6c5160a3bb --- /dev/null +++ b/.changelog/4905.fixed @@ -0,0 +1 @@ +`opentelemetry-instrumentation-logging`: Keep log record factories chained after ours on uninstrument