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

Rate limiting ignored #258

Closed
deepxg opened this issue May 22, 2018 · 8 comments
Closed

Rate limiting ignored #258

deepxg opened this issue May 22, 2018 · 8 comments
Assignees

Comments

@deepxg
Copy link

deepxg commented May 22, 2018

I have an appender that sends stuff to Slack, and I had hoped that I'd rate limited it properly, but it occasionally goes bonkers. Is there any obvious issue with this config?

.
.
.
:appenders {
                    :slack
                    {:enabled? true
                     :async? false
                     :min-level :warn
                     :rate-limit [[1 1000]]
                     :output-fn :inherit}
.
.
.

I'd assumed that meant it'd send no more than 1 message a second, but that's not the case.

@ptaoussanis
Copy link
Member

Hi there :-)

Your config looks okay in principle.

Could you please provide some context? E.g. what you mean by "goes bonkers", if this only happens with Slack, if you have any reproducible example, etc.

@deepxg
Copy link
Author

deepxg commented Nov 6, 2018

I think my issue here was I was expecting rate limiting to occur at the appender level (i.e. stop flooding our Slack channel), but instead it's at the error source level. So it was working, just wasn't really doing what I expected.

@boechat107
Copy link

@deepxg, I think I had the same doubt about the rate limit configuration. After reading the source code and executing some tests, I concluded the rate limit is message-dependent.

We need to think of the rate limit for each possible logging message. If all logging messages are different, no rate limit kicks in.

Is my observation correct, @ptaoussanis?

@boechat107
Copy link

The first time I tried to test the rate limit my logging messages had the value of a counter, and no limit was ever imposed. I saw the limits kicking in when I logged only two different messages in a loop.

@ptaoussanis
Copy link
Member

ptaoussanis commented Jan 7, 2022

Hi there! Timbre uses this heuristic for identifying logging calls for the purposes of rate-limiting, etc.

I.e. each unique combination of the following identifies an individual rate-limiter:

  1. Each individual Timbre macro form (i.e. callsite, e.g. (timbre/infof ...)).
  2. When applicable, the particular message format string (only applicable for format-style logging, otherwise nil).
  3. An optional manually-provided hash argument (an advanced + currently undocumented feature) [see R1] or (common-case) the combination of all other arguments provided to the logging call.

I.e. you can think of the default limiter behaviour as per (logging callsite and argument content).
I.e. the main purpose of the default behaviour is to prevent the exact same repeated logical event from logging over and over.

If you'd prefer alternative behaviour, you can either:

  • Provide the optional manual hash argument in (3) above [see R1].
  • Or use your own rate limiter - manually or via middleware. Timbre just uses taoensso.encore/limiter under-the-covers. This is a high performance and very flexible general-use rate limiter.

Hope that helps! Cheers :-)

[R1] e.g. (timbre/infof ^:meta {:hash "my-logging-hash"} "User %s logged in" user-name).

@boechat107
Copy link

Thanks for the quick response, @ptaoussanis!

My tests confirm everything you said. I was able to find the hash composition here after your comment.

Wouldn't be worth it to add a link to your comment in *config* docstring?

@ptaoussanis
Copy link
Member

Will add some documentation on the next release 👍

@ptaoussanis ptaoussanis reopened this Jan 8, 2022
@ptaoussanis ptaoussanis self-assigned this Jan 8, 2022
@ptaoussanis
Copy link
Member

ptaoussanis commented Mar 22, 2022

Rate limiting behaviour will be explained in more detail in forthcoming release, thanks Andre!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants