-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Introduce LOG_LEVEL_OVERRIDES config var #10808
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
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 9m 4s ⏱️ Results for commit 6ed2dd6. ♻️ This comment has been updated with latest results. |
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.
Thanks for implementing this, configuring specific components to log on a higher level will hopefully be useful in support cases.
localstack/config.py
Outdated
@@ -418,6 +418,9 @@ def in_docker(): | |||
LS_LOG = eval_log_type("LS_LOG") | |||
DEBUG = is_env_true("DEBUG") or LS_LOG in TRACE_LOG_LEVELS | |||
|
|||
# allow setting custom log levels for individual loggers | |||
LOGGING_OVERRIDE = os.environ.get("LOGGING_OVERRIDE", "{}") |
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.
Not sure about the name, maybe LOG_LEVEL_CONFIG
or LOGGING_CONFIG
? Since this is for now an undocumented change, it might not be that important, but those changes sometimes stay :)
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.
I think this name is misleading, since it sounds like it updates the entire config, rather than just allowing overrides. WDYT?
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.
You are right, let's keep it to override for now :)
localstack/logging/setup.py
Outdated
@@ -78,6 +79,23 @@ def setup_logging_from_config(): | |||
for name, level in trace_internal_log_levels.items(): | |||
logging.getLogger(name).setLevel(level) | |||
|
|||
if raw_value := config.LOGGING_OVERRIDE: | |||
try: | |||
logging_overrides = json.loads(raw_value) |
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.
WDYT json vs key-value pairs? I think the latter might be easier to write, and we do not need nested structures. I do not have a strong opinion though.
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.
I think that's a good usability improvement 👍
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.
Implemented in db163ef
localstack/logging/setup.py
Outdated
@@ -78,6 +79,23 @@ def setup_logging_from_config(): | |||
for name, level in trace_internal_log_levels.items(): | |||
logging.getLogger(name).setLevel(level) | |||
|
|||
if raw_value := config.LOGGING_OVERRIDE: |
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.
Not sure if we can use this syntax here - this file is imported by the cli if the debug flag is set. Our tests pass, but that flag might break.
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.
Good point, thanks!
3dd89f1
to
2474a6d
Compare
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.
Thanks for challenging the logging config and testing new approaches.
I only added 2 questions to the code, but I am not 100% sure about the reasoning behind it.
We already have the distinction between default_log_levels
, trace_log_levels
, and trace_internal_log_levels
. Are the logs really so noisy that they are even too verbose for trace_internal_log_levels
?
e098af2
to
aa4269c
Compare
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.
really nice addition! thanks for going through the review iterations, i think the reviews improved the already great PR a lot!
My only nit would be that I'm also not 100% sold on the name. If we're overriding log levels, then i believe those three words should appear in the config variable, LOG_LEVEL_OVERRIDES
for instance?
- LOG_LEVEL to indicate what it is you're configuring
- OVERRIDES (plural) to indicate that you're overriding multiple log level configs
aa4269c
to
98e64e2
Compare
98e64e2
to
b8c3b97
Compare
b8c3b97
to
234a639
Compare
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.
Nice cleanup, thanks for tackling all the comments, the result is super small and clean! 💯
Currently, only patch changes are allowed on master. Your PR labels (semver: minor) indicate that it cannot be merged into the master at this time. |
Marking as |
Co-Authored-By: Daniel Fangl <daniel.fangl@localstack.cloud>
76e51eb
to
6ed2dd6
Compare
Test Results - Alternative Providers597 tests 420 ✅ 14m 36s ⏱️ Results for commit 6ed2dd6. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 21m 0s ⏱️ Results for commit 6ed2dd6. |
Motivation
While debugging DNS issues, we found the need to add additional logging to help debugging, but only for a specific module. The DNS resolution process is too noisy to use current debug logs.
See #10809 for an application of this new config feature.
Changes
@dfangl suggested we support a per-logger override for the log level. This would mean that if we are interested in a specific logger (or can disable some logs), we can specify this in a structured environment variable.
Note: we are planning on re-working the logging overall, so this functionality should only be used in specific cases, to enable more detailed logs for specific aspects of LocalStack's internals, and not documented.