Issue/configurable logging #8471

wouterdb · 2024-12-12T20:53:51Z

Description

Add configurable logging.

closes #8306
closes #8307

includes

per-component log files via the config file

[logging]
server = server_log.yml
scheduler = scheduler.log.tmpl

tool to dump log file inmanta print_default_logging_config [compiler|scheduler|server]
moved resource action log to scheduler

Questions and results

do we want comments in the files outputted by the tool?
- comments are in the files in misc, not in the tool output
do we want to enforce a naming convention on the logger. (it is in the code, but I wouldn't, as we don't have a lot of feeling with this )
- no: we don't know logging well enogu
should -v work when using the config file?
- force in a cli logger at this level

Next steps

remove all resource action alike functions from the server
add a way to validate config files for specific tools (like scheduler) -> Logging: add a way to validate config files for specific tools (like scheduler) #8482
Some methods in the stable api of the logging are deprecated, do we drop them? -> logging: Some methods in the stable api of the logging are deprecated #8485
Hook on LSM Design way for extensions to contribute log config #8492
TTY forcing? -> logging Force TTY to be abled when generating default config #8484
load config from env var -> Add support to provide logging config via environment variables #8308
document logging config -> Dcoument configurable logging #8493

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

Attached issue to pull request
Changelog entry
Type annotations are present
Code is clear and sufficiently documented
No (preventable) type errors (check using make mypy or make mypy-diff)
Sufficient test cases (reproduces the bug/tests the requested feature)
Correct, in line with design
End user documentation is included or an issue is created for end-user documentation (add ref to issue here: ) -> Dcoument configurable logging #8493
If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

wouterdb · 2024-12-12T20:57:06Z

src/inmanta/app.py

@@ -85,7 +85,7 @@ def server_parser_config(parser: argparse.ArgumentParser, parent_parsers: abc.Se
    )


-@command("server", help_msg="Start the inmanta server", parser_config=server_parser_config)
+@command("server", help_msg="Start the inmanta server", parser_config=server_parser_config, component="server")


Adding the component here to load correct config

wouterdb · 2024-12-12T20:57:55Z

src/inmanta/app.py

+def default_logging_config(options: argparse.Namespace) -> None:
+    config_builder = LoggingConfigBuilder()
+
+    # Al possible context vars:


Handling of templates-in-templates below, somewhat nasty, but it works

It would be nice to have a docstring that explain how the templating works conceptually. Now you have to derive all that information from the code itself.

wouterdb · 2024-12-12T20:58:23Z

src/inmanta/app.py

+    log_context = {}
+    env = None
+    if hasattr(options, "environment"):
+        env = options.environment
+    if not env:
+        env = agent_config.environment.get()
+    if env:
+        log_context[LOG_CONTEXT_VAR_ENVIRONMENT] = env


We have so little config I just hardcoded it here

src/inmanta/logging.py

wouterdb · 2024-12-12T21:04:51Z

src/inmanta/server/agentmanager.py

-                "--log-file",
-                agent_log,


This is now part of the log config! (I'm a bit in doubt, but it simplifies things)

bartv · 2024-12-13T07:25:35Z

src/inmanta/app.py

+    )
+
+
+@command("default_logging_config", help_msg="Print the default log config", parser_config=default_log_config_parser)


I think generate_default_loging_config or some other prefix like print_, export_ or print_ would convey better what this does.

I would also extend the help message with: Print the default log config for the provided component

arnaudsjs · 2024-12-13T07:28:11Z

do we want comments in the files outputted by the tool? (this probably means either hardcode them as string with comments or some ruamel) . We could also provide documented versions in the docs

I think this is very much a must have, given that we have some very specific custom loggers. I am in favor of hard coding it. I guess this config is not going to change often, is it?

do we want to enforce a naming convention on the logger. (it is in the code, but I wouldn't, as we don't have a lot of feeling with this )

I think we already enforce some rules for formatters and handlers, defined by an extension, to prevent naming conflicts. I am not sure whether this is any useful for the loggers as well, because this is visible to the end-user. Our current convention is using the module names. I guess that's sufficiently unique.

should -v work when using the config file?

I think we will have to if we want to offer a decent user experience.

Some methods in the stable api of the logging are deprecated, do we drop them?

I think those methods are mainly used by the solutions team in many of their tooling. So the question is mainly whether they are ready for this. To be honest I am not even sure whether they are aware about the deprecation of these methods.

bartv · 2024-12-13T07:41:58Z

Some methods in the stable api of the logging are deprecated, do we drop them?

I think those methods are mainly used by the solutions team in many of their tooling. So the question is mainly whether they are ready for this. To be honest I am not even sure whether they are aware about the deprecation of these methods.

I think there has been discussion some time ago. iirc this is mostly used in the logger that is attached to requests in restbase. We can ask Guillaume, he probably knows from the top of his head.

arnaudsjs · 2024-12-13T08:43:11Z

src/inmanta/logging.py

@@ -84,6 +86,20 @@ def python_log_level_to_name(python_log_level: int) -> str:
 logging.addLevelName(3, "TRACE")


+class LogConfigDumper(Dumper):
+    """Ensure class variables are not shared, as representer config lives on the class"""


I don't understand this docstring.

arnaudsjs · 2024-12-13T08:54:24Z

src/inmanta/logging.py

+        if component == "scheduler":
+            # Override defaults
+            if LOG_CONTEXT_VAR_ENVIRONMENT not in context:
+                raise Exception("The scheduler expect an environment as context")


Suggested change

raise Exception("The scheduler expect an environment as context")

raise Exception("The scheduler expects an environment as context")

sanderr · 2024-12-13T10:53:36Z

I'm not sure about exposing the name scheduler to the end user, as mentioned before in the office. I think deploy or agent might be more intuitive. But I'm also not too heavily opposed to it, and I recognize that I'm bad at estimating what's intuitive when it comes to these types of things.

sanderr · 2024-12-13T10:54:13Z

Questions

I also think comments would be good.

sanderr · 2024-12-13T10:55:05Z

Questions

No opinion, I can get behind what Arnaud responded.

sanderr · 2024-12-13T10:56:03Z

Questions

As discussed in the office. I'm torn: either don't allow it, or have it set the root logger's level. Preference for the latter, but tough to make the call.

sanderr · 2024-12-13T10:56:19Z

Questions

Sounds good

sanderr

Approving PR description

wouterdb · 2024-12-16T15:31:55Z

I'm not sure about exposing the name scheduler to the end user, as mentioned before in the office. I think deploy or agent might be more intuitive. But I'm also not too heavily opposed to it, and I recognize that I'm bad at estimating what's intuitive when it comes to these types of things.

@sanderr I see you point, but I'm a bit torn. The main audience here is sysadmins. They will be interested in process listing, config files, log files,...

So they don't have the direct developer view. For them, the distinction agent/scheduler/executor is very relevant. I think we made step a forward from agent/agent(process)/agent(instance) in that they are now more distinct concepts.

But it would be nice if they wouldn't have to know. But I'm afraid they do have to know. (We can only afford them not to know if these things never fail, but I think venv's will keep acting up)

sanderr · 2024-12-16T15:34:30Z

Ok, I think that's a valid point. I don't know the user perspective sufficiently well to provide further input. I mainly wanted to expose this fact, but I'm totally fine leaving it as is if you think it would be best in the grand scheme of things.

tests/test_server.py

Hugo-Inmanta · 2024-12-18T14:40:37Z

tests/test_server.py

@@ -1340,10 +1339,6 @@ async def test_send_deploy_done(server, client, environment, agent, caplog, meth
            )
            assert result.code == 200, result.result

-    # Assert effect of send_deploy_done call


Not immediately clear at the moment why this is no longer logged

src/inmanta/logging.py

Hugo-Inmanta · 2024-12-18T14:59:21Z

src/inmanta/logging.py

        return config.logging_config.get()

    @stable_api
-    def apply_options(self, options: Options) -> None:
+    def apply_options(self, options: Options, component: str | None = None, context: Mapping[str, str] = {}) -> None:


missing param docstring from stable_api method

Hugo-Inmanta · 2024-12-18T15:13:16Z

src/inmanta/logging.py

@@ -550,6 +523,7 @@ def __init__(self, stream: TextIO = sys.stdout) -> None:
        self._handlers: abc.Sequence[logging.Handler] = self._apply_logging_config(log_config)
        self._options_applied: bool = False
        self._logging_configs_extensions: list[LoggingConfigExtension] = []


This is no longer used afaics. I don't remember what we discussed about the logging config for lsm, maybe it is still relevant to keep this around then.

Suggested change

self._logging_configs_extensions: list[LoggingConfigExtension] = []

src/inmanta/logging.py

Hugo-Inmanta · 2024-12-18T15:19:37Z

src/inmanta/logging.py

@@ -761,7 +766,6 @@ def __init__(
        super().__init__(fmt, log_colors=log_colors, reset=reset, no_color=no_color)
        self.fmt = fmt
        self._keep_logger_names = keep_logger_names
-        self._logger_mode_manager = LoggerModeManager.get_instance()


_wrap_record docstring should be updated: _logger_mode no longer exists

jptrindade

Do we already have tests to assert that -v

tests/test_logging.py

src/inmanta/config.py

src/inmanta/deploy/tasks.py

src/inmanta/agent/forking_executor.py

Co-authored-by: Hugo-Inmanta <[email protected]>

Co-authored-by: João Trindade <[email protected]>

…ta-core into issue/configurable_logging

jptrindade · 2024-12-18T16:20:45Z

src/inmanta/agent/forking_executor.py


    # Load config
    inmanta.config.Config.load_config_from_dict(config)

    # Make sure logfire is configured correctly
-    tracing.configure_logfire("agent.executor")
+    tracing.configure_logfire("inmanta.executor")


Suggested change

tracing.configure_logfire("inmanta.executor")

tracing.configure_logfire(LOGGER_NAME_EXECUTOR)

It is not about the logger name here (even if I'm not sure what it is)

src/inmanta/logging.py

Hugo-Inmanta · 2024-12-19T07:44:07Z

src/inmanta/logging.py

@@ -805,8 +811,6 @@ def format(self, record: logging.LogRecord) -> str:
    def _wrap_record(self, record: logging.LogRecord) -> logging.LogRecord:


We could even lift the logic from _get_logger_name_for:

if not self._keep_logger_names: ....

In here and then call into _get_logger_name_for only when required

src/inmanta/logging.py

tests/deploy/test_scheduler_agent.py

Co-authored-by: Hugo-Inmanta <[email protected]>

…logging

inmantaci · 2024-12-19T10:44:06Z

Processing this pull request

inmantaci · 2024-12-19T10:44:09Z

Merged into branches master in 00301d7

# Description Add configurable logging. closes #8306 closes #8307 closes #8356 ## includes 1. per-component log files via the config file ```ini [logging] server = server_log.yml scheduler = scheduler.log.tmpl ``` 2. tool to dump log file ` inmanta print_default_logging_config [compiler|scheduler|server]` 3. moved resource action log to scheduler ## Questions and results 1. [x] do we want comments in the files outputted by the tool? - comments are in the files in misc, not in the tool output 3. [x] do we want to enforce a naming convention on the logger. (it is in the code, but I wouldn't, as we don't have a lot of feeling with this ) - no: we don't know logging well enogu 5. [x] should `-v` work when using the config file? - force in a cli logger at this level ## Next steps 1. [ ] remove all resource action alike functions from the server 2. [ ] add a way to validate config files for specific tools (like scheduler) -> #8482 6. [ ] Some methods in the stable api of the logging are deprecated, do we drop them? -> #8485 7. [ ] Hook on LSM #8492 8. [ ] TTY forcing? -> #8484 9. [ ] load config from env var -> #8308 10. [ ] document logging config -> #8493 # Self Check: Strike through any lines that are not applicable (`~~line~~`) then check the box - [x] Attached issue to pull request - [x] Changelog entry - [x] Type annotations are present - [x] Code is clear and sufficiently documented - [x] No (preventable) type errors (check using make mypy or make mypy-diff) - [x] Sufficient test cases (reproduces the bug/tests the requested feature) - [x] Correct, in line with design - [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: ) -> #8493 - [ ] If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)

wouterdb added 2 commits December 11, 2024 15:26

temp

a298065

configurable logging

5cdb509

wouterdb requested review from bartv, arnaudsjs and sanderr December 12, 2024 20:53

wouterdb commented Dec 12, 2024

View reviewed changes

src/inmanta/logging.py Show resolved Hide resolved

wouterdb commented Dec 12, 2024

View reviewed changes

src/inmanta/logging.py Outdated Show resolved Hide resolved

wouterdb commented Dec 12, 2024

View reviewed changes

fix tests

5536037

bartv reviewed Dec 13, 2024

View reviewed changes

arnaudsjs reviewed Dec 13, 2024

View reviewed changes

sanderr approved these changes Dec 13, 2024

View reviewed changes

wouterdb added 2 commits December 13, 2024 13:38

Pass along config

a596882

Fix verbose option

ca3dec3

wouterdb added 4 commits December 16, 2024 20:36

reviewer comments

b687d95

Temp

5d5c848

More fixes

db9388a

Fix log merge

6768bfb

Hugo-Inmanta requested changes Dec 18, 2024

View reviewed changes

jptrindade requested changes Dec 18, 2024

View reviewed changes

wouterdb and others added 5 commits December 18, 2024 17:04

Update src/inmanta/logging.py

a2352c6

Co-authored-by: Hugo-Inmanta <[email protected]>

Update src/inmanta/deploy/tasks.py

0b3125b

Co-authored-by: João Trindade <[email protected]>

Apply suggestions from code review

d3fff9c

Co-authored-by: João Trindade <[email protected]>

Reviewer comments

b080d98

Merge branch 'issue/configurable_logging' of github.com:inmanta/inman…

7ee2011

…ta-core into issue/configurable_logging

wouterdb requested review from Hugo-Inmanta and jptrindade December 18, 2024 16:18

jptrindade approved these changes Dec 18, 2024

View reviewed changes

update component names

26bd4a2

Hugo-Inmanta requested changes Dec 19, 2024

View reviewed changes

Hugo-Inmanta reviewed Dec 19, 2024

View reviewed changes

tests/deploy/test_scheduler_agent.py Outdated Show resolved Hide resolved

wouterdb and others added 4 commits December 19, 2024 09:05

Apply suggestions from code review

a9fd53b

Co-authored-by: Hugo-Inmanta <[email protected]>

Update src/inmanta/logging.py

d651878

Co-authored-by: Hugo-Inmanta <[email protected]>

Move condition

69481f0

Merge remote-tracking branch 'origin/master' into issue/configurable_…

0c4219c

…logging

wouterdb requested a review from Hugo-Inmanta December 19, 2024 09:00

Fix

23ef621

Hugo-Inmanta approved these changes Dec 19, 2024

View reviewed changes

wouterdb added 4 commits December 19, 2024 10:39

more fixes

c8993de

Merge remote-tracking branch 'origin/master' into issue/configurable_…

e77f80a

…logging

fixed pep8

3e787bb

fixed merge

ab4697e

wouterdb added the merge-tool-ready This ticket is ready to be merged in label Dec 19, 2024

inmantaci closed this Dec 19, 2024

inmantaci deleted the issue/configurable_logging branch December 19, 2024 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/configurable logging #8471

Issue/configurable logging #8471

wouterdb commented Dec 12, 2024 •

edited

Loading

wouterdb Dec 12, 2024

wouterdb Dec 12, 2024

arnaudsjs Dec 13, 2024

wouterdb Dec 12, 2024

wouterdb Dec 12, 2024

bartv Dec 13, 2024

arnaudsjs commented Dec 13, 2024 •

edited

Loading

bartv commented Dec 13, 2024

arnaudsjs Dec 13, 2024

arnaudsjs Dec 13, 2024

sanderr commented Dec 13, 2024

sanderr commented Dec 13, 2024

sanderr commented Dec 13, 2024

sanderr commented Dec 13, 2024

sanderr commented Dec 13, 2024

sanderr left a comment

wouterdb commented Dec 16, 2024

sanderr commented Dec 16, 2024

Hugo-Inmanta Dec 18, 2024

Hugo-Inmanta Dec 18, 2024

Hugo-Inmanta Dec 18, 2024

Hugo-Inmanta Dec 18, 2024

jptrindade left a comment

jptrindade Dec 18, 2024

wouterdb Dec 18, 2024

Hugo-Inmanta Dec 19, 2024

wouterdb Dec 19, 2024

inmantaci commented Dec 19, 2024

inmantaci commented Dec 19, 2024

		)


		@command("default_logging_config", help_msg="Print the default log config", parser_config=default_log_config_parser)

	raise Exception("The scheduler expect an environment as context")
	raise Exception("The scheduler expects an environment as context")

	tracing.configure_logfire("inmanta.executor")
	tracing.configure_logfire(LOGGER_NAME_EXECUTOR)

		@@ -805,8 +811,6 @@ def format(self, record: logging.LogRecord) -> str:
		def _wrap_record(self, record: logging.LogRecord) -> logging.LogRecord:

Issue/configurable logging #8471

Issue/configurable logging #8471

Conversation

wouterdb commented Dec 12, 2024 • edited Loading

Description

includes

Questions and results

Next steps

Self Check:

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

arnaudsjs commented Dec 13, 2024 • edited Loading

bartv commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderr commented Dec 13, 2024

sanderr commented Dec 13, 2024

Questions

sanderr commented Dec 13, 2024

Questions

sanderr commented Dec 13, 2024

Questions

sanderr commented Dec 13, 2024

Questions

sanderr left a comment

Choose a reason for hiding this comment

wouterdb commented Dec 16, 2024

sanderr commented Dec 16, 2024

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

jptrindade 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

inmantaci commented Dec 19, 2024

inmantaci commented Dec 19, 2024

wouterdb commented Dec 12, 2024 •

edited

Loading

arnaudsjs commented Dec 13, 2024 •

edited

Loading