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

fix(libbeat): mitigate race condition in ratelimit processor #42966

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

mauri870
Copy link
Member

@mauri870 mauri870 commented Feb 28, 2025

Proposed commit message

This PR fixes race conditions on the ratelimit processor. The race conditions can only manifest with configurations that have a global ratelimit processor and multiple inputs. Fixed by adding a mutex to the bucket struct.

During benchmarking, I also noticed an unnecessary allocation occurring in the hot path. Removing this allocation resulted in a 25% performance improvement. While locks have an inherent cost, in this scenario, the performance gain offsets that cost.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Run filebeat with a global ratelimit processor under the race detector:

$ cd x-pack/filebeat; go build -race .
$ GORACE="halt_on_error=1" ./filebeat -c ratelimit.yml -d '*' -v -e
filebeat:
  inputs:
    - type: benchmark
      enabled: true
      message: "bench1"
      eps: 100
    - type: benchmark
      enabled: true
      message: "bench2"
      eps: 100
    - type: benchmark
      enabled: true
      message: "bench3"
      eps: 100
    - type: benchmark
      enabled: true
      message: "bench4"
      eps: 100
processors:
- rate_limit:
   limit: "1/s"
output.console:
  pretty: true
logging:
  level: debug
  selectors:
    - "*"
path.home: /tmp/filebeat

@mauri870 mauri870 added Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team bugfix backport-active-all Automated backport with mergify to all the active branches labels Feb 28, 2025
@mauri870 mauri870 self-assigned this Feb 28, 2025
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Feb 28, 2025
@mauri870 mauri870 marked this pull request as ready for review February 28, 2025 17:11
@mauri870 mauri870 requested a review from a team as a code owner February 28, 2025 17:11
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@mauri870 mauri870 force-pushed the ratelimit-data-race branch from 92dac6e to 31eb212 Compare February 28, 2025 17:15
@mauri870 mauri870 force-pushed the ratelimit-data-race branch from 31eb212 to 2a24573 Compare February 28, 2025 17:17
@mauri870 mauri870 force-pushed the ratelimit-data-race branch from 3f60d1d to d1f22f4 Compare March 5, 2025 12:00
@mauri870 mauri870 requested a review from belimawr March 5, 2025 12:21
@mauri870
Copy link
Member Author

mauri870 commented Mar 5, 2025

/test

1 similar comment
@mauri870
Copy link
Member Author

mauri870 commented Mar 5, 2025

/test

Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

LGTM, just waiting for CI to be green.

@mauri870
Copy link
Member Author

mauri870 commented Mar 6, 2025

/test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-active-all Automated backport with mergify to all the active branches bugfix 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