emmett-framework / granian

A Rust HTTP server for Python applications
BSD 3-Clause "New" or "Revised" License
2.86k stars 83 forks source link

Optional logging #122

Closed Aeron closed 1 year ago

Aeron commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Done and done 👍