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

reduce logging level for timeouts #131

Closed

Conversation

alexeyOnGitHub
Copy link
Contributor

this change is related to #67

the motivation for this change is that some users may have external
retriers around clients, like this Memcached one.
the library ideally should throw an exception on timeout and
let users deal with it however they see fit - it may be logging,
retrying or falling back to another solution.
when the library logs an error in this case, this makes logging
noisy, which may distort operational metrics in a
typical prod environment.

this change is related to spotify#67

the motivation for this change is that some users may have external
retriers around clients, like this Memcached one.
the library ideally should throw an exception on timeout and
let users deal with it however they see fit - it may be logging,
retrying or falling back to another solution.
when the library logs an error in this case, this makes logging
noisy, which may distort operational metrics in a
typical prod environment.
@coveralls
Copy link

coveralls commented May 21, 2019

Coverage Status

Coverage remained the same at ?% when pulling e461432 on alexeyOnGitHub:ask-reduce-logging into f69d98f on spotify:master.

@alexeyOnGitHub alexeyOnGitHub changed the title reduce logging for timeouts from error to warn reduce logging level for timeouts May 21, 2019
@spkrka
Copy link
Member

spkrka commented May 24, 2019

Thanks for the PR! I am not sure this is something that we want to do.

First of all - how noisy is this in practice? It should only log once if memcached becomes very slow (doesn't make forward progress at all during the period (default 3 seconds)).

Second of all, users of this could configure their logging to filter out this message if it is too annoying.
For logback, that would look something like:

<logger name="com.spotify.folsom.client.DefaultRawMemcacheClient" level="OFF"/>

Also, I think error better captures the semantic meaning of the log event compared to info.
(Though perhaps warn would still make sense)

@alexeyOnGitHub
Copy link
Contributor Author

we need to provide multiple 9s of availability together with pretty low latency,
so we use very aggressive timeouts for Memcached calls and then automatically fall back to another level of caching. this happens pretty often in our case, at least in highly loaded services.
I don't want to completely disable logging for this client. ideally I would want to be able to attach my logic (via a listener or something similar) to this condition and process it myself.
we forked the library and disabled extra logging for now, feel free to close this PR

@alexeyOnGitHub alexeyOnGitHub deleted the ask-reduce-logging branch November 29, 2019 04:49
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.

4 participants