8000 Introduce LOG_LEVEL_OVERRIDES config var by simonrw · Pull Request #10808 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 7 commits into from
May 29, 2025
Merged

Conversation

simonrw
Copy link
Contributor
@simonrw simonrw commented May 13, 2024

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.

@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label May 13, 2024
@simonrw simonrw self-assigned this May 13, 2024
Copy link
github-actions bot commented May 13, 2024

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   9m 4s ⏱️
495 tests 445 ✅  50 💤 0 ❌
990 runs  890 ✅ 100 💤 0 ❌

Results for commit 6ed2dd6.

♻️ This comment has been updated with latest results.

Copy link
github-actions bot commented May 13, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 44m 52s ⏱️ + 2m 42s
4 469 tests ±0  4 081 ✅ ±0  388 💤 ±0  0 ❌ ±0 
4 471 runs  ±0  4 081 ✅ ±0  390 💤 ±0  0 ❌ ±0 

Results for commit 6ed2dd6. ± Comparison against base commit e10e769.

♻️ This comment has been updated with latest results.

Copy link
Member
@dfangl dfangl left a 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.

@@ -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", "{}")
Copy link
Member

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 :)

Copy link
Contributor Author

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?

Copy link
Member

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 :)

@@ -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)
Copy link
Member

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.

Copy link
Contributor Author

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 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in db163ef

@@ -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:
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks!

@simonrw simonrw force-pushed the config/logging-level-overrides branch 2 times, most recently from 3dd89f1 to 2474a6d Compare May 13, 2024 15:05
@simonrw simonrw marked this pull request as ready for review May 13, 2024 16:02
Copy link
Member
@alexrashed alexrashed left a 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?

@simonrw simonrw force-pushed the config/logging-level-overrides branch from e098af2 to aa4269c Compare May 15, 2024 15:47
Copy link
Member
@thrau thrau left a 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

@joe4dev joe4dev added this to the Playground milestone Jun 10, 2024
@simonrw simonrw force-pushed the config/logging-level-overrides branch from aa4269c to 98e64e2 Compare March 2, 2025 06:25
@dfangl dfangl force-pushed the config/logging-level-overrides branch from 98e64e2 to b8c3b97 Compare April 2, 2025 09:10
@dfangl dfangl force-pushed the config/logging-level-overrides branch from b8c3b97 to 234a639 Compare May 7, 2025 15:00
@dfangl dfangl requested a review from alexrashed May 7, 2025 15:06
@dfangl dfangl changed the title Introduce LOGGING_OVERRIDE config var Introduce LOG_LEVEL_OVERRIDES config var May 7, 2025
Copy link
Member
@alexrashed alexrashed left a 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! 💯

@dfangl dfangl modified the milestones: Playground, 4.5 May 7, 2025
@simonrw simonrw added semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases and removed semver: patch Non-breaking changes which can be included in patch releases labels May 7, 2025
@localstack-bot
Copy link
Collaborator

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.

Copy link
Contributor Author
simonrw commented May 7, 2025

Marking as minor since it changes the "public" API (i.e. configuration) of LocalStack.

@simonrw simonrw force-pushed the config/logging-level-overrides branch from 76e51eb to 6ed2dd6 Compare May 29, 2025 08:49
Copy link

Test Results - Preflight, Unit

21 579 tests  ±0   19 927 ✅ ±0   6m 26s ⏱️ ±0s
     1 suites ±0    1 652 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 6ed2dd6. ± Comparison against base commit e10e769.

Copy link

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 7s ⏱️ ±0s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 6ed2dd6. ± Comparison against base commit e10e769.

Copy link

Test Results - Alternative Providers

597 tests   420 ✅  14m 36s ⏱️
  4 suites  177 💤
  4 files      0 ❌

Results for commit 6ed2dd6.

Copy link

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 21m 0s ⏱️
4 824 tests 4 283 ✅ 541 💤 0 ❌
4 830 runs  4 283 ✅ 547 💤 0 ❌

Results for commit 6ed2dd6.

@simonrw simonrw merged commit cec8fc5 into master May 29, 2025
64 checks passed
@simonrw simonrw deleted the config/logging-level-overrides branch May 29, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0