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

Issue 1087: support dictConfig logging configuration #1110

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@MatMoore
Copy link
Contributor

MatMoore commented Sep 3, 2015

Hi,

This PR is a proof of concept for issue #1087.

So far I have only done a brief test, and I am not too familiar with the existing config implementation, but I will happily look into this in more detail and clean it up if I can get some feedback on the approach taken.

As pointed out in the linked issue, dictConfig enables certain functionality that is not available through the older fileConfig interface.

I've kept the new option separate from --config for backwards compatibility, but dictConfig takes precedence. I don't know whether --config should be entirely deprecated though.

@roboslone @berkerpeksag what are your thoughts?

@berkerpeksag

This comment has been minimized.

Copy link
Collaborator

berkerpeksag commented Sep 3, 2015

Thanks for the PR, @MatMoore :) This approach looks good to me. I'll review it more closely soon.

I don't think we should touch --config, but I'm in favor of deprecating --logconfig. Having a separate and outdated setting(also a separate file) for logging configuration is not a good a thing for users :)

I also would like to disable passing --dictconfig since it makes no sense and it confuses users. Is there a simple way to do this, @benoitc?

@@ -199,7 +199,9 @@ def setup(self, cfg):
self.access_log, cfg, self.syslog_fmt, "access"
)

if cfg.logconfig:
if cfg.logconfigdict:
dictConfig(cfg.logconfigdict)

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 3, 2015

Collaborator

Haven't played with the patch yet, but perhaps we should update CONFIG_DEFAULTS first and then pass it to dictConfig?

This comment has been minimized.

@MatMoore

MatMoore Sep 3, 2015

Author Contributor

Yep, using dictConfig for the default case seems sensible, at least if fileConfig will be eventually removed. I'll take a closer look at this.

This comment has been minimized.

@MatMoore

MatMoore Sep 5, 2015

Author Contributor

I misunderstood your comment earlier, but I've implemented this now (i.e. the defaults will be applied regardless of whether dictConfig or fileConfig is used). As far as I can tell the format is the same for both.

@benoitc

This comment has been minimized.

Copy link
Owner

benoitc commented Sep 3, 2015

Thanks for the patch!

Do we really need another config setting? It seems that you can only pass the dictConfig via the config file. So we can probably only check it instead and accept it.

Also in case you didn't notice it the CI is broken with the current patch:

    def validate_dict(val):
        if not isinstance(val, dict):
>           raise TypeError("Value is not a dictionary: %s " % val)
E           TypeError: Value is not a dictionary: None
@MatMoore

This comment has been minimized.

Copy link
Contributor Author

MatMoore commented Sep 3, 2015

Doh, sorry about that - this was very rushed, but I'll try to spend some more time on it this weekend (and I'll make the tests pass this time!)

@benoitc Could you clarify what you mean by only check it and accept it? With the new settings object I was only planning on using it for basic validation and documentation, not cli options, so I can do it differently if there's a simpler way?

I suppose we could also reuse logconfig for the dictConfig version, while still supporting the deprecated file values, but it doesn't seem like the setting metaclass was designed for this usage.

@berkerpeksag Oops, --logconfig was the one I meant :) Happy to go down the deprecation route if that's the consensus.

Mat Moore and others added some commits Sep 3, 2015

@MatMoore

This comment has been minimized.

Copy link
Contributor Author

MatMoore commented Sep 5, 2015

Something I didn't realise earlier is that dictConfig wasn't added until 2.7, so I've modified my changes to ignore the new setting on older versions. Do you want me to still put a deprecation notice for --logconfig?

@@ -199,7 +204,11 @@ def setup(self, cfg):
self.access_log, cfg, self.syslog_fmt, "access"
)

if cfg.logconfig:
if dictConfig and cfg.logconfig_dict:

This comment has been minimized.

@berkerpeksag

berkerpeksag Sep 26, 2015

Collaborator

I think we can print a user friendly message if dictConfig is None instead of silently ignore the problem. I will review this more carefully soon. Sorry for my late response!

This comment has been minimized.

@MatMoore

MatMoore Sep 26, 2015

Author Contributor

No problem :) I think making dictConfig an error on 2.6 and below would probably be best, but I'll wait until you're done with the review before changing anything.

@benoitc

This comment has been minimized.

Copy link
Owner

benoitc commented Feb 6, 2017

bump ? What is needed to close that PR?

@berkerpeksag

This comment has been minimized.

Copy link
Collaborator

berkerpeksag commented Feb 6, 2017

I need some time to review this. I can't make any promise for 19.7, but I'll review it eventually :)

@benoitc

This comment has been minimized.

Copy link
Owner

benoitc commented Feb 6, 2017

@berkerpeksag thanks :)

@MatMoore

This comment has been minimized.

Copy link
Contributor Author

MatMoore commented Mar 3, 2017

Hi, I'm going to close the PR as I don't personally need the feature any more and I'm not particularly engaged with making sure it gets into gunicorn. In the meantime it just clutters my open PRs list.

@benoitc if the change is still useful feel free to raise it again as a new PR. Thanks for following up on this.

@MatMoore MatMoore closed this Mar 3, 2017

@benoitc benoitc referenced this pull request Sep 7, 2017

Open

generic logging options #1572

@benoitc

This comment has been minimized.

Copy link
Owner

benoitc commented Sep 7, 2017

@berkerpeksag reopened after #1572 discussion. What do you think we should do there to apply it?

@benoitc benoitc reopened this Sep 7, 2017

@aconrad

This comment has been minimized.

Copy link
Contributor

aconrad commented Sep 16, 2017

Would anyone mind to update this PR with the latest master to fix the coverage report?

@tilgovi

This comment has been minimized.

Copy link
Collaborator

tilgovi commented Sep 18, 2017

Rebased. See #1602.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.