Allow configuring logger_class with statsd_host #1188

Merged
merged 3 commits into from Feb 19, 2016

Projects

None yet

2 participants

@bloodearnest
Contributor

Currently if you configure statsd_host, a configured logger_class will never be used.

I think this makes a user configured logger class always take priority.

(This is PoC change, I will come back and add tests/docs if it's worth pursuing)

@bloodearnest
Contributor

This is a proposed fix for #1187

@tilgovi
Collaborator
tilgovi commented Feb 2, 2016

Is it the case that a user should probably not specify both arguments together? Should we issue a warning when both are provided?

@bloodearnest
Contributor

On 2 February 2016 at 23:37, Randall Leeds notifications@github.com wrote:

Is it the case that a user should probably not specify both arguments
together? Should we issue a warning when both are provided

My use case is exactly that, though.

I did look at adding a warning about makeing sure that your custom logger
class inherited from the statsd logger, when both both are specified. But
since logging has not yet been initialised (by definition), I wasn't sure
how to do that. Is there a way to defer a warning until logging is
configured?

Thanks

Simon

Simon

@tilgovi
Collaborator
tilgovi commented Feb 11, 2016

I think this is fine. What do you think about maybe where you have the "N.B." in the docs for the statsd option to add a note that any custom logger class specified should be API-compatible with the statsd logger. That seems like a reasonable place to document this and easier than trying to issue a warning.

This is great, thank you.

@bloodearnest
Contributor

On 11 February 2016 at 21:26, Randall Leeds notifications@github.com
wrote:

I think this is fine. What do you think about maybe where you have the
"N.B." in the docs for the statsd option to add a note that any custom
logger class specified should be API-compatible with the statsd logger.
That seems like a reasonable place to document this and easier than trying
to issue a warning.

Good idea, done.

This is great, thank you.

Thank you for taking the time to look at it :)

Simon

bloodearnest added some commits Jan 22, 2016
@bloodearnest @bloodearnest bloodearnest Allow configuring logger_class with statsd_host
Currently if you configure statsd_host, a configured logger_class will never be used.

I think this makes a user configured logger class always take priority.

(This is PoC change, I will come back and add tests/docs if it's worth pursuing)
f68a043
@bloodearnest bloodearnest Always use the the user configured logger class.
Previously, configuring statsd_host would override the configured class
34b595e
@bloodearnest bloodearnest Update docs - more explicit info on logger_class b05286e
@bloodearnest
Contributor

Rebased on master, be great if you could land

@tilgovi
Collaborator
tilgovi commented Feb 19, 2016

Beautiful. Thanks for your effort and patience, @bloodearnest!

@tilgovi tilgovi merged commit 690b11e into benoitc:master Feb 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment