-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A draft for making a decision on managing logging configurations #28603
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
Status | ||
====== | ||
|
||
Draft | ||
|
||
|
||
Context | ||
======= | ||
|
||
Logging configuration needs more options to be useful for devops/SRE work. | ||
|
||
Decision | ||
======== | ||
|
||
Let us add an optional override dictionary to `get_logging_config`. This would | ||
allow us to change filters, log options, and add custom log handlers, with | ||
varying utility for differing edx platform installations. | ||
|
||
Specification of the override dictionary can be done within the YAML configs, | ||
to keep things consistant with how the rest of managing Django settings. | ||
|
||
Explanation and examples for adding logging customization to Django settings | ||
should be included in any relevent documentation. | ||
|
||
|
||
Consequences | ||
============ | ||
|
||
Having the override logging settings defined in yaml override files keeps | ||
changes to log configuration easier to view and manage, as they will be | ||
consistent with the rest of Django setting management. | ||
|
||
Having the actual override occur as part of `get_logging_config` means one does | ||
not need to worry about resetting the logging state, if called multiple times | ||
during setting extensions. | ||
|
||
The limitation demarkation of log configuration then lies in what the logging | ||
module actually offers. | ||
|
||
|
||
Alternatives Considered | ||
======================= | ||
|
||
Individual kwargs for specific overrides | ||
---------------------------------------- | ||
|
||
The argument here is to create limitiation by attempting to guess what specific | ||
logging customization might be required for future log formating. This would | ||
clutter the interface if more options are later discovered to be necessary. | ||
|
||
Moving the log configuration into the settings module | ||
----------------------------------------------------- | ||
|
||
The argument here is to make logging management the same as other configuration | ||
values, for ergonomics. There are advantages to this; the implementation does | ||
not need to handle dictionary merges, and the possibility of divergent logging | ||
implementations is less. But the initial comment to the implementation is worth | ||
reproducing in full here: | ||
|
||
Return the appropriate logging config dictionary. You should assign the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite understand what this means. If say, |
||
result of this to the LOGGING var in your settings. The reason it's done | ||
this way instead of registering directly is because I didn't want to worry | ||
about resetting the logging state if this is called multiple times when | ||
settings are extended. | ||
|
||
This seems like a good enough reason to continue to use `get_logging_config`. | ||
|
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you talking about setting a param to https://github.com/edx/edx-platform/blob/ac8b4f5a6dfdc4ccc6433aeae60d42f6aa341207/openedx/core/lib/logsettings.py#L13 that would allow us to replace the entire logger config dictionary? Thus allowing any operator to fully manage how logging is done on their deployment of edx-platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebberg checking back in on this. Trying to understand what piece you want to make overridable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general idea is to be able to support customizing the logging configuration by passing in configuration values. The specifics of the mechanism are still subject to debate and design work. For the time being we have written a plugin to override the
LOGGING
setting but it might be worth exploring how to support some measure of modification without having to resort to a plugin.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, I agree that making the logging settings more configurable is valuable. I think we can go a bit further though and explore also making it easier to understand and explain.
It sounds like the current suggestion is to have more config settings that can be passed to the
get_logger_config
function that will allow for more flexibility?