Introduce LOGGING_OVERRIDE config var #10808

simonrw · 2024-05-13T11:56:42Z

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.

github-actions · 2024-05-13T12:04:21Z

S3 Image Test Results (AMD64 / ARM64)

2 files 2 suites 3m 25s ⏱️
398 tests 346 ✅ 52 💤 0 ❌
796 runs 692 ✅ 104 💤 0 ❌

Results for commit aa4269c.

♻️ This comment has been updated with latest results.

github-actions · 2024-05-13T12:51:28Z

LocalStack Community integration with Pro

2 files ±0 2 suites ±0 1h 36m 52s ⏱️ +2s
2 950 tests ±0 2 648 ✅ ±0 302 💤 ±0 0 ❌ ±0
2 952 runs ±0 2 648 ✅ ±0 304 💤 ±0 0 ❌ ±0

Results for commit aa4269c. ± Comparison against base commit 9d80628.

♻️ This comment has been updated with latest results.

dfangl

Thanks for implementing this, configuring specific components to log on a higher level will hopefully be useful in support cases.

dfangl · 2024-05-13T14:23:44Z

localstack/config.py

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


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

I think this name is misleading, since it sounds like it updates the entire config, rather than just allowing overrides. WDYT?

You are right, let's keep it to override for now :)

dfangl · 2024-05-13T14:24:53Z

localstack/logging/setup.py

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


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.

I think that's a good usability improvement 👍

Implemented in db163ef

dfangl · 2024-05-13T14:27:38Z

localstack/logging/setup.py

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


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.

Good point, thanks!

alexrashed

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?

alexrashed · 2024-05-14T07:54:56Z

localstack/logging/setup.py

+        except RuntimeError:
+            raise
+        except Exception as e:
+            raise RuntimeError(


q: Can this exception really only be raised in case the value is malformed?

I am not sure, so since we "raise ... from e" I am happy to remove the "malformed input" part of the error message as it only printed e anyway.

alexrashed · 2024-05-14T07:56:52Z

localstack/utils/collections.py

+        if len(items) != 2:
+            raise ValueError(f"invalid key/value pair: '{pair}'")


q: Since maxsplit=2 is set, this means that this check really only checks if the pair does not contain = right?
Maybe it would be better to remove the maxsplit to also catch cases like a=b=c?

You are right, thanks!

localstack/config.py

dominikschubert · 2024-05-15T07:42:22Z

localstack/utils/collections.py

@@ -533,3 +533,18 @@ def is_comma_delimited_list(string: str, item_regex: Optional[str] = None) -> bo
    if pattern.match(string) is None:
        return False
    return True
+
+
+def parse_key_value_pairs(raw_text: str) -> dict:


nit: could add a docstring here with for example the values from one of the test cases below to clearly demonstrate its purpose.

Thanks for the suggestion!

Co-Authored-By: Daniel Fangl <daniel.fangl@localstack.cloud>

Co-authored-by: Dominik Schubert <dominik.schubert91@gmail.com>

thrau

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

thrau · 2024-05-15T20:41:25Z

localstack/logging/setup.py

+                    raise RuntimeError(
+                        f"Failed to configure logging overrides ({raw_logging_override}): '{level_name}' is not a valid log level"
+                    )


nit: is there a specific reason to raise RuntimeError and not ValueError?

simonrw added the semver: patch Non-breaking changes which can be included in patch releases label May 13, 2024

simonrw self-assigned this May 13, 2024

simonrw mentioned this pull request May 13, 2024

Add trace logging for the DNS server #10809

Draft

dfangl reviewed May 13, 2024

View reviewed changes

simonrw force-pushed the config/logging-level-overrides branch 2 times, most recently from 3dd89f1 to 2474a6d Compare May 13, 2024 15:05

simonrw marked this pull request as ready for review May 13, 2024 16:02

simonrw requested review from alexrashed, dominikschubert and thrau as code owners May 13, 2024 16:02

alexrashed reviewed May 14, 2024

View reviewed changes

dominikschubert reviewed May 15, 2024

View reviewed changes

localstack/config.py Show resolved Hide resolved

dominikschubert reviewed May 15, 2024

View reviewed changes

simonrw and others added 5 commits May 15, 2024 16:40

Introduce LOGGING_OVERRIDE config var

f443788

Co-Authored-By: Daniel Fangl <daniel.fangl@localstack.cloud>

Do not use walrus operator for python version compatibility

7909941

Convert envar to key-value pairs

356ad77

Update localstack/config.py

cce4b2d

Co-authored-by: Dominik Schubert <dominik.schubert91@gmail.com>

Address review comments

aa4269c

simonrw force-pushed the config/logging-level-overrides branch from e098af2 to aa4269c Compare May 15, 2024 15:47

thrau approved these changes May 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce LOGGING_OVERRIDE config var #10808

Introduce LOGGING_OVERRIDE config var #10808

simonrw commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

dfangl left a comment

dfangl May 13, 2024

simonrw May 13, 2024

dfangl May 13, 2024

dfangl May 13, 2024

simonrw May 13, 2024

simonrw May 13, 2024

dfangl May 13, 2024

simonrw May 13, 2024

alexrashed left a comment

alexrashed May 14, 2024

simonrw May 15, 2024

alexrashed May 14, 2024

simonrw May 15, 2024

dominikschubert May 15, 2024

simonrw May 15, 2024

thrau left a comment

thrau May 15, 2024 •

edited

		if len(items) != 2:
		raise ValueError(f"invalid key/value pair: '{pair}'")

Introduce LOGGING_OVERRIDE config var #10808

Are you sure you want to change the base?

Introduce LOGGING_OVERRIDE config var #10808

Conversation

simonrw commented May 13, 2024 • edited

Motivation

Changes

github-actions bot commented May 13, 2024 • edited

S3 Image Test Results (AMD64 / ARM64)

github-actions bot commented May 13, 2024 • edited

LocalStack Community integration with Pro

dfangl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexrashed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thrau left a comment

Choose a reason for hiding this comment

thrau May 15, 2024 • edited

Choose a reason for hiding this comment

simonrw commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

github-actions bot commented May 13, 2024 •

edited

thrau May 15, 2024 •

edited