Gunicorn: Consistently use Python's warnings module and remove `utils.warn`

[Code0x58]

There is a util.warn function which prints to stderr with some fanfare which is only used to say that two pasterapp functions are deprecated in favour on gunicorn --paste $config. These should use a DeprecationWarning.

$ ag "warn\(" gunicorn
gunicorn/argparse_compat.py
1571:            warnings.warn(
2306:        warnings.warn(
2332:        warnings.warn(

gunicorn/util.py
206:        warnings.warn('tests may fail, delete still pending for ' + pathname,
524:def warn(msg):

gunicorn/app/pasterapp.py
178:    util.warn("""This command is deprecated.
202:    util.warn("""This command is deprecated

[berkerpeksag]

DeprecationWarnings are silent by default and since most users don't use Gunicorn for local development and don't set -Wd on production, I prefer our more explicit util.warn helper.

[tilgovi]

Even better might be to get the warning into the Gunicorn log, wherever that is.

We could change the two places where we do this in the pasterapp code to set app = Paster...() and then call app.logger.warn() before app.run().

[Code0x58]

I was thinking that warnings on stderr would usually just go nowhere, or if done well then into a log file somewhere if the service's (error) output is written anywhere; gunicorn.error/stdout are used for other things that just seem like noise that won't get looked at unless something is already going very wrong, so not a good place for something that you'd use proactively,

I think this would be an improvement for people who use logging.captureWarnings(True) along with logging.simplefilter("default") and specifying a _py.warnings_ handler.

I'm not a huge fan of it going to gunicorn.error either, although it is simple enough to set the parent of py.warnings to be gunicorn.error so that it propagates to it.

I think it would be better practice to configure py.warnings for the user so that it goes into it's own handler/log file (maybe requiring a flag to configure it). A lot of libraries/frameworks use it (including code gunicorn uses), and I think the world would be a better place if people used it or were even aware of it more. Something like --warning-log=$log_file sounds like a good idea to me - I do this kind kind of thing with self rolled logging config, which probably explains why I didn't put the flag in to begin with.

[tilgovi]

All very interesting. I do not know yet what the best approach is. This is the first time I've learned about the warnings module, myself.

I like using a standard mechanism for warnings. I dislike adding more CLI options.

As a server, Gunicorn can be opinionated about these issues. What we do right now is a strong opinion, to always log to stderr.

I think @berkerpeksag comment is correct. However, users run gunicorn not python -w. Therefore, Gunicorn gets to decide how to set up warnings by default. We could decide to set the default filter, as you suggest. If the user does not like it, they can write a custom application or modify the filters in a server hook.

Our glogging module uses disable_existing_loggers=False everywhere. Maybe one idea is to set up logging.captureWarnings(True) like you say, and set up a default py.warnings stderr handler. The behavior would be the same as today, but with log formatting. Users would start to see more warnings on stderr that might have been filtered before, but otherwise there would be no change in behavior.

We could update any relevant examples to add py.warnings handlers, too, if we want to push it further.

What do you all think?

[Code0x58]

I think the kind of warnings that are put out through the warnings module (e.g. [pending]deprecation) are a separate concern from those that usually go through the logging module (kind of dev vs. ops)

The opinion I have is that people should take advantage of the warnings module for notifying/being notified about development (rather than operational) concerns. When used by developers on both sides, they are _really_ useful - easier to maintain/filter than in depth changelogs, and again easier for broadcasting/picking up on development directions. At very least good deprecation warnings are brilliant when it comes to updating dependencies; I send them to their own log for this.

I think having another logging option like --access-logfile (not logged by default) isn't that bad, and would help promote use of warnings by end developers. At work I configure all logging (including _py.warnings_) in Python, not using the command line options (although have to hack around them), but still think other users would really benefit from it.

[benoitc]

IMO the current behaviour is the correct one if you consider it under the angle "gunicorn is a server".

At this level we are more interested by the log since you don't wan't to raise anything just log that deprecation when the gunicorn daemon is upgraded (HUP/USR2) and let the op know/prepare the next upgrade that will require a full restart of gunicorn with new options.

For those who embed Gunicorn inside their service the case may be different, but I would prefer to keep the current behaviour which is a common pattern among other servers too.

[tilgovi]

• I like the idea of using the warnings module because that's what it's there for.
• I want our warnings to go to stderr. I don't feel strongly about making them go anywhere else, even as an option.
• I like keeping the CLI options to a minimum.
• I like letting developers who know about the warnings module to do what they want with them.

For all these reasons, I propose we:

1. Subclass UserWarning so that we have a warning type of our own that isn't filtered by default.
2. Issue deprecation warnings using this warning.

Falcon does something similar: https://github.com/falconry/falcon/blob/master/falcon/util/misc.py#L53

[berkerpeksag]

util.warn is used in two places at the moment and they are going to be removed in Gunicorn 20. While I don't think we have an immediate need to have a proper warning mechanism, I agree with @tilgovi's comments above.

I also like Falcon's deprecated decorator. However, we probably need to adapt it (or support both use cases) to our Setting class since our only documented public API is gunicorn CLI:

class ConfigFile(Setting):
    deprecated_options = {
        'msg': 'The ``spam:PATH`` format is deprecated.',
        'hint': 'Please use ``python:PATH`` instead.',
        'will_be_removed': '20.0',
        'id': 'config.W001',
    }
    name = 'config'
    section = 'Config File'
    ...

I borrowed this idea from https://docs.djangoproject.com/en/1.11/topics/migrations/#considerations-when-removing-model-fields

[tilgovi]

Marking this for R20 and let's consider it low priority / nice to have. I think we all agree our current warning to stderr is okay for now.

I second @berkerpeksag comment that having a way to easily mark deprecation in the code would be great for the future.

[berkerpeksag]

@Code0x58 can we close PR #1591 now or would you like to update it to implement the ideas we've discussed in this issue?

[Code0x58]

I suppose one way that keeps current behaviour and adds python warnings (although with non ideal _filename/funcName/lineno/module/pathname_ in the resulting log record) is to use a wrapper that calls util.warn and also warnings.warn. I updated #1591 to do that, and also to state that those two functions will be removed in 20.

I would prefer it if warnings.warn was used exclusively, just for the improved log record information, but keeping the messages going to stderr means touching more of the python environment and so imposing things on the underlying app.

[benoitc]

@tilgovi @berkerpeksag what your current position about it? i'm temped to close it and make and finish #1482 for the next milestone . Thoughts?

[tilgovi]

After #1957 the only use will be for gaiohttp, which is also deprecated. We should remove gaiohttp module and remove util.warn.

[tilgovi]

Proposal:

• Close #1591 or update it to have a deprecation decorator and support for deprecating settings
• Close this ticket when we remove gaiohttp and pasterapp deprecations.

[benoitc]

@tilgovi i think the last thing we should now do is to remove gaiohttp, right?

[benoitc]

@tilgovi i made the changes removing gaiohttp in #1971 .