docs: warn about dictConfig clearing OTel handlers in logs example#5420
docs: warn about dictConfig clearing OTel handlers in logs example#5420avinab-neogy wants to merge 2 commits into
Conversation
590cb1b to
e8465b0
Compare
| that it may clear existing handlers on the root logger, including the | ||
| OTel handler. To avoid losing telemetry, save and restore the root | ||
| logger handlers around the ``dictConfig`` call: | ||
|
|
There was a problem hiding this comment.
This is incorrect, the opposite is true. We explicitly preserve the handler by removing it before logging.config.dictConfig and logging.config.fileConfig are called, and then re-adding it back after, see
There was a problem hiding this comment.
@DylanRussell @herin049 Thanks for pointing that out, you're both totally right!
To answer your question @herin049, I went back into my local setup and my original test was just running standard Python's dictConfig function with a vanilla StreamHandler, completely bypassing the OTel instrumentation.
I tested it again using the OpenTelemetry library, and I could see how it automatically saves and restores the handler.
import logging
import logging.config
from opentelemetry.instrumentation.logging import LoggingInstrumentor
LoggingInstrumentor().instrument()
root_logger = logging.getLogger()
print('Before dictConfig:', root_logger.handlers) # [<LoggingHandler (NOTSET)>]
logging.config.dictConfig({'version': 1, 'root': {'level': 'DEBUG', 'handlers': []}})
print('After dictConfig: ', root_logger.handlers) # [<LoggingHandler (NOTSET)>] - It does not get cleared!
That said, I originally opened this to address Point 3 from Issue #4738 :
- the root logger is instrumented, so handlers on it must not be cleared and if you are loading logging settings with logging/.config.dictConfig, you must save the root loggers before applying dictConfig, then re-apply missing handlers
This issue explicitly asked for this warning to be added to the docs. I'm guessing that requirement came from someone configuring the LoggingHandler manually via the core SDK, where this monkey-patch doesn't exist?
How would you prefer to handle this? I'm happy to do either of the following:
- Close this PR, and we can update Point 3 on the original issue to say it's no longer an issue when using the instrumentation library.
- Keep the PR open but rewrite the warning to clarify that it only applies when configuring the
LoggingHandlermanually via the core SDK, sincedictConfigwill still clear their handlers without the instrumentation package.
There was a problem hiding this comment.
I think we should just close this PR, I don't see any behavior here that needs to be documented.
|
I have the same understanding as @DylanRussell. Do you have an example reproduction that actually involves using the logging instrumentation library? |
Description
Follow-up to #5344 which addressed the LoggingHandler move to contrib.
Refs #4738
This PR documents the
dictConfighandler-saving pattern from point 3of #4738. When
dictConfigis called with a root section that defineshandlers, it clears existing handlers on the root logger including the
OTel handler, silently breaking telemetry. Users need to save and
restore handlers around the call.
Tested locally — confirmed that
dictConfigwith a root section wipesall existing handlers.
Opening as draft to confirm scope before requesting review.
@xrmx does this belong in the logs example README or somewhere else?
Type of change
How Has This Been Tested?
Verified locally that calling
dictConfigwith a root section clearsexisting handlers:
Does This PR Require a Contrib Repo Change?
Checklist: