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

logp: add rate limited logger #256

Closed
wants to merge 8 commits into from

Conversation

mauri870
Copy link
Member

What does this PR do?

This commit adds support for rate limiting to logp.Logger. This is useful
when you have a lot of log messages and you want to avoid flooding the
destination.

The logger was augmented with the following methods:
Throttled will only log once every period.
Sampled will only log the nth message.
Limited will only log the first n messages.

The implementation is based on x/time/rate.Sometimes.

For all these limiters any message that is not logged will be dropped.

These also synergize with one another, for example:

log.Sampled(10).
    Throttled(time.Second).
    Info("")`

will log one message every second, and only one out of ten messages.

Likewise,

log.Limited(10).
    Throttled(time.Second).
    Info("")

will log the first ten messages, and then one message every second.

Why is it important?

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

Related issues

This commit adds support for rate limiting to logp.Logger. This is useful
when you have a lot of log messages and you want to avoid flooding the
destination.

The logger was augmented with the following methods:
    `Throttled` will only log once every period.
    `Sampled` will only log the nth message.
    `Limited` will only log the first n messages.

The implementation is based on x/time/rate.Sometimes.

For all these limiters any message that is not logged will be dropped.

These also synergize with one another, for example:

    log.Sampled(10).
        Throttled(time.Second).
        Info("")`

will log one message every second, and only one out of ten messages.

Likewise,

    log.Limited(10).
        Throttled(time.Second).
        Info("")

will log the first ten messages, and then one message every second.
@mauri870 mauri870 added the enhancement New feature or request label Nov 29, 2024
@mauri870 mauri870 self-assigned this Nov 29, 2024
@mauri870 mauri870 marked this pull request as ready for review November 29, 2024 16:10
@mauri870 mauri870 requested a review from a team as a code owner November 29, 2024 16:10
@mauri870 mauri870 requested review from faec and VihasMakwana and removed request for a team November 29, 2024 16:10
@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Nov 29, 2024
@mauri870 mauri870 marked this pull request as draft December 4, 2024 11:08
@mauri870
Copy link
Member Author

I really like the idea of a rate limited logger, but I think the cost is too high, at least using the approach implemented here.

For a single threaded execution the cost is tolerable, but for mutli-threaded scenarios the contention is too high for a hot path:

go test -bench='Benchmark.*Logger' -benchmem -run=^$ ./logp | column -t
goos: linux
goarch: amd64
pkg: github.com/elastic/elastic-agent-libs/logp
cpu: Intel(R) Xeon(R) CPU E5-2697A v4 @ 2.60GHz
BenchmarkLogger/default-32                                                                        100000000                                   10.33      ns/op  0         B/op  0  allocs/op
BenchmarkLogger/sampled-32                                                                        17537918                                    69.40      ns/op  0         B/op  0  allocs/op
BenchmarkLogger/throttled-32                                                                      12967281                                    87.35      ns/op  0         B/op  0  allocs/op
BenchmarkLogger/limited-32                                                                        18614289                                    63.58      ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerSynthetic/default-32                                                     1000000000                                  0.0001241  ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerSynthetic/sampled-32                                                     277615                                      13437      ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerSynthetic/throttled-32                                                   236827                                      18124      ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerSynthetic/limited-32                                                     329881                                      9944       ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerRealistic/default-32                                                     1000000000                                  0.0000182  ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerRealistic/sampled-every-4-32                                             347101                                      11292      ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerRealistic/throttled-1-per-second-32                                      544308                                      12081      ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerRealistic/limited-first-10-32                                            1051113                                     5290       ns/op  0         B/op  0  allocs/op
BenchmarkConcurrentLoggerRealistic/sampled-1-out-of-2-limited-first-10-throttled-1-per-second-32  290791                                      20145      ns/op  0         B/op  0  allocs/op
PASS                                                                                                                                                           
ok  

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 21, 2025

💔 Build Failed

Failed CI Steps

History

cc @mauri870

@mauri870
Copy link
Member Author

Closing this since the solution is not really what we are looking for right now. We'll continue discussing in elastic/beats#41993.

@mauri870 mauri870 closed this Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants