Fix custom logging color attributes raising ValueError (#68129)#69433
Open
dwoz wants to merge 1 commit into
Open
Fix custom logging color attributes raising ValueError (#68129)#69433dwoz wants to merge 1 commit into
dwoz wants to merge 1 commit into
Conversation
When ``log_fmt_console`` (or ``log_fmt_logfile``) was set to a format string using the documented color attributes (for example ``%(colorlevel)s %(colormsg)s``), Salt printed a ``ValueError: Formatting field not found in record: 'colorlevel'`` for every log message that had been buffered by the temporary ``DeferredStreamHandler`` before logging was fully configured. The bug stems from ``setup_console_handler`` switching the active log record factory from ``SaltLogRecord`` to ``SaltColorLogRecord`` only after the deferred handler had been populated. When ``shutdown_temp_handler`` flushed those buffered records into the newly installed console handler -- whose formatter referenced the ``color*`` attributes -- python's logging machinery failed because the records had been constructed by the plain ``SaltLogRecord`` factory and therefore lacked any ``color*`` attribute. Make ``SaltLogRecord`` always populate ``colorname``, ``colorlevel``, ``colorprocess`` and ``colormsg`` with non-colored fallback values. ``SaltColorLogRecord`` continues to override them with colorized values for records created after color logging has been configured. Fixes saltstack#68129
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Stops Salt from emitting
ValueError: Formatting field not found in record: 'colorlevel'(and equivalent forcolormsg,colorname,colorprocess) on every buffered log message whenlog_fmt_console/log_fmt_logfileuses one of the documented color attributes.What issues does this PR fix or reference?
Fixes #68129
Previous Behavior
With a configuration such as:
Salt printed a
--- Logging error ---traceback for every log record emitted before the console handler was set up, while still managing to color the records emitted afterwards.New Behavior
The buffered records are formatted normally (without color escapes for the records that pre-date the console handler), and the records emitted after console logging is configured continue to be colorized as before. No tracebacks are printed.
Root cause
setup_console_handlerswitches the active log record factory fromSaltLogRecordtoSaltColorLogRecord. Records that were buffered by the temporaryDeferredStreamHandlerbefore that point were instances ofSaltLogRecord, which had nocolor*attributes.shutdown_temp_handlerflushed those records to the now-color-format console handler, and python's logging machinery raisedValueErrorfor each missing attribute.SaltLogRecordnow always populatescolorname,colorlevel,colorprocessandcolormsgwith non-colored fallbacks;SaltColorLogRecordkeeps overriding them with colorized values for records created after color logging is set up.Merge requirements satisfied?
Commits signed with GPG?
Yes