Don't include log messages with log level "AUDIT" in regular service log file #4538
Conversation
This file contains 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
messages with log level "AUDIT" if log level is set to INFO or higher. This way we avoid issues with duplicate AUDIT messages in production deployments. In production deployments default log level is set to INFO which means that all service log files will also contain AUDIT log messages because AUDIT level if the highest. This is not desired, because we already log AUDIT log messages in a dedicated AUDIT log file. NOTE: Audit messages will still go in the service log file is log level is set to DEBUG (that's desired during debugging).
Kami
force-pushed
the
audit_logging_duplication_fix
branch
from
b5ed972 to
09feb5d
Compare
based filtering works correctly.
|
Added integration tests in 11111ff. |
LindsayHill
approved these changes
Feb 7, 2019
|
Randomly found out just now while debugging some other things that this doesn't work correctly on production gunicorn deployments. I believe issue is related to another change, I'm digging in. |
Kami
added a commit
that referenced
this pull request
Apr 8, 2019
Not doing that means change / fix in #4538 won't work since regular config which includes DEBUG and AUDIT will be used over gunicorn one.
|
^Second time is the charm - #4621. The code was correct, but we were using incorrect logging config for API services with packages so those log messages weren't filtered out by default. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pull request resolves an issue with log messages with log level AUDIT ending up in regular service log file (
<service>.log) in addition to the service audit log file (<service>.audit.log).Background
We implemented
AUDITlog level by adding a new log level constant which contains the highest log level number (CRITICAL+ 10).Default Python log level filtering works using "include log messages with log level and above" approach. This means messages with
AUDITlog level will always be included in the logs becauseAUDITis the highest level.In most cases that's not desired. That's especially true for production deployment where default log level is
INFOand we have dedicated log file with audit log messages. We don't want audit log messages to also end up in the regular service log file where they just cause noise.Implementation wise, I needed to implement it using a log filter - we exclude log messages with level
AUDITif log level is set toINFOor higher, but not toAUDIT. I couldn't make a simpler change like settingAUDITlog level constant to lower thanINFO, because then we would have the opposite problem (nonAUDITmessages would also end up in the audit log file).NOTE: To aid with the debugging,
AUDITlog messages are also included in a regular service log file if log level is set toDEBUGof ifsystem.debugconfig option is set toTrue. When debug is enabled there are many more log messages coming from other places soAUDITshould be the least of your concerns :)Resolves #4502 (thanks to @nmaludy for reporting it).
TODO