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