Skip to content
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

Optional logging #122

Merged
merged 6 commits into from
Oct 17, 2023
Merged

Conversation

Aeron
Copy link
Contributor

@Aeron Aeron commented Sep 6, 2023

This PR adds the --log/--no-log option to CLI and the same argument to the Granian server class. The default value is enabled/True, so there are no backward compatibility changes. The option/argument simply allows to opt-out from logging to be configured and emit anything.

Thus, it resolves #101.

Does it look suitable? Don’t hesitate to give any feedback.

@gi0baro
Copy link
Member

gi0baro commented Sep 7, 2023

@Aeron reading again #101, I am actually a bit confused on the aim of this.

If the aim is to prevent Granian to log, then this won't change that. If the aim is to prevent Granian to alter the root config of the logging level, then this is correct, but I would change the naming, as --no-log to me sounds more like "Granian won't log anything" rather than "Granian won't configure the logging module".

WDYT?

@Aeron
Copy link
Contributor Author

Aeron commented Sep 7, 2023

In my case, I’m happy when the worker is not emitting log records, especially in stdin/stderr. Yet I can introduce logging.disable(logging.CRITICAL) for this to be considered an actual disabling on the Python level.

As far as I can see, all relevant logging from Rust code goes through pyo3-log, so it should be enough.

Will it be a suitable solution? Or did I misinterpret what you meant?

@gi0baro
Copy link
Member

gi0baro commented Sep 7, 2023

Actually pyo3-log uses the root logger, so if the application code configures it, even with this PR Granian will log stuff, and thus the parameter --no-log might became confusing.

As far as I remember, pyo3-log didn't allow to pick a specific logger. It might be worth to check again, maybe we can just move stuff to a granian logger and make this viable in every condition.

@Aeron
Copy link
Contributor Author

Aeron commented Sep 7, 2023

Yeah, I clearly was too fast to suggest the logging.disable solution 😅 We don’t want to turn off all Python logging. Sorry about that.

As for pyo3-log, it vaguely looks like the Logger.filter_target() method could help us here. I’ll give it a try then.

@Aeron
Copy link
Contributor Author

Aeron commented Sep 19, 2023

Okay, the filter_target does not help, in a way I hoped, but it gave a clue that pyo3-log loggers have proper names, module-related names. It’s not just the root logger.

So, Granian’s inner logger names start with _granian: _granian.rsgi.serve, etc. Yet they cannot be found among logging.root.manager.loggerDict, for example.

Therefore, the simplest way to turn off logging for all _granian.* loggers at once is to lower the logging level beyond critical. Other options are more quirky. Yet this way, it still can be re-enabled programmatically at any point, and the root logger is unaffected.

@gi0baro what do you think?

@gi0baro
Copy link
Member

gi0baro commented Sep 25, 2023

Therefore, the simplest way to turn off logging for all _granian.* loggers at once is to lower the logging level beyond critical. Other options are more quirky. Yet this way, it still can be re-enabled programmatically at any point, and the root logger is unaffected.

@gi0baro what do you think?

Makes sense to me.
I'm just thinking about the name of the parameter: while I find --log/--no-log in CLI very appropriate, probably I would prefer something more explicit in server.py, like log_enabled or logging_enabled. WDYT?

@Aeron
Copy link
Contributor Author

Aeron commented Oct 7, 2023

Done and done 👍

granian/log.py Outdated Show resolved Hide resolved
@gi0baro gi0baro added this to the 0.7 milestone Oct 16, 2023
Aeron and others added 5 commits October 17, 2023 12:39
Adds `--log`/`--no-log` option to CLI. Adds the relevant argument to the Granian server class and its spawn methods. Updates README with the relevant CLI option.
Updates logging configuration functionality with an argument to disable logging (by lowering the logger logging level beyond critical). Reverts server module related changes.
Alters server’s logging related argument names for the sake of more consistent naming.
Adds help description for logging enable option. Fixes single-quote formatting.
@gi0baro gi0baro force-pushed the feature/optional-logging branch from 2bbc07e to a8ba938 Compare October 17, 2023 10:43
@gi0baro gi0baro force-pushed the feature/optional-logging branch from a8ba938 to 6410a22 Compare October 17, 2023 10:44
@gi0baro gi0baro merged commit f54d807 into emmett-framework:master Oct 17, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flag to disable the logging
2 participants