Introduce LOG_LEVEL_OVERRIDES config var #10808
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. |
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.
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.
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.
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.
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.
I think that's a good usability improvement 👍
There was a problem hiding this comment.
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.
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.
Good point, thanks!
3dd89f1 to
2474a6d
Compare
There was a problem hiding this comment.
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.
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
|
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.