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

Add periodic/Doer for frequency-limited action execution #220

Merged

Conversation

AndersonQ
Copy link
Member

@AndersonQ AndersonQ commented Aug 1, 2024

What does this PR do?

Add periodic/Doer for frequency-limited action execution

Why is it important?

The Doer limits the execution of an action to at most once within a specified period. It's designed for managing frequent events by providing a summary with the number of occurrences within the given period. If no event happen, the action is not executed.

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

Related issues


// RateLimitedLogger is a logger that limits log messages to once within a
// specified period.
// It is intended for logging events that occur frequently, providing a summary
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a custom implementation of the token bucket algorithm in https://github.com/elastic/beats/blob/main/libbeat/processors/ratelimit/token_bucket.go but I'd honestly prefer we replace that the one in x/time/rate as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well apart from the fact I missed there is a [Every](https://pkg.go.dev/golang.org/x/time/rate#Every), the work to accumulate the number of events to then log the summary would be the same. Also using the ticker it's a rather straight forward implementation. Thus I don't find it necessary to use https://pkg.go.dev/golang.org/x/time/rate

@AndersonQ AndersonQ force-pushed the 40157-rate-limit-cannot-index-logs branch 2 times, most recently from 010244f to b4dcc44 Compare August 6, 2024 15:02
@AndersonQ AndersonQ self-assigned this Aug 6, 2024
@AndersonQ AndersonQ force-pushed the 40157-rate-limit-cannot-index-logs branch from 86dc65c to e391dbc Compare August 6, 2024 15:47
@AndersonQ AndersonQ added enhancement New feature or request Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Aug 6, 2024
@AndersonQ AndersonQ marked this pull request as ready for review August 7, 2024 07:11
@AndersonQ AndersonQ requested a review from a team as a code owner August 7, 2024 07:11
@AndersonQ AndersonQ requested review from rdner and leehinman and removed request for a team August 7, 2024 07:11
@pierrehilbert
Copy link

@AndersonQ shouldn't we also change the PR title?

@AndersonQ AndersonQ requested a review from cmacknz August 7, 2024 07:44
@AndersonQ AndersonQ changed the title wip: rate limited logger Add RateLimitedLogger for frequency-limited logging Aug 7, 2024
@AndersonQ
Copy link
Member Author

@AndersonQ shouldn't we also change the PR title?

I did, but didn't click on 'save' ahah. Now it's fixed, thanks

The Doer limits the execution of an action to at most once within a specified period. It's designed for managing frequent events by providing a summary with the number of occurrences within the given period. If no event happen, the action is not executed.
@AndersonQ AndersonQ force-pushed the 40157-rate-limit-cannot-index-logs branch from b2fd3b3 to 456505c Compare August 8, 2024 09:00
@AndersonQ AndersonQ changed the title Add RateLimitedLogger for frequency-limited logging Add periodic/Doer for frequency-limited action execution Aug 8, 2024
Copy link
Member

@rdner rdner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not thread-safe.

periodic/ratelimited.go Outdated Show resolved Hide resolved
periodic/ratelimited.go Outdated Show resolved Hide resolved
periodic/ratelimited.go Show resolved Hide resolved
periodic/ratelimited.go Outdated Show resolved Hide resolved
periodic/ratelimited.go Outdated Show resolved Hide resolved
@AndersonQ AndersonQ requested a review from rdner August 8, 2024 14:57
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @AndersonQ

@AndersonQ AndersonQ merged commit 1cf0584 into elastic:main Aug 9, 2024
6 checks passed
@AndersonQ AndersonQ deleted the 40157-rate-limit-cannot-index-logs branch August 9, 2024 15:22
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.

5 participants