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 support to configure min GC intervals for soft and hard limits #12450

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

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Feb 21, 2025

When servers are in a high memory usage mode, the amount of times GC is called creates a high CPU usage which combined with a high ingest rate limits capability to offload existing queued data.

Configuring the minimum interval even for hard limit, allows the system to spend some CPU cycles between GCs to offload old data from queues/batch processor. The most amount of data I've seen accumulated are blocked in the batch processor queue (incoming channel) not in the exporter queue.

Copy link

codecov bot commented Feb 21, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.19%. Comparing base (f024b0d) to head (960c67d).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
internal/memorylimiter/config.go 42.85% 4 Missing ⚠️
internal/memorylimiter/memorylimiter.go 97.22% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (88.88%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #12450    +/-   ##
========================================
  Coverage   92.19%   92.19%            
========================================
  Files         466      465     -1     
  Lines       25325    25187   -138     
========================================
- Hits        23348    23221   -127     
+ Misses       1577     1570     -7     
+ Partials      400      396     -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +194 to +199
if time.Since(ml.lastGCDone) > ml.minGCIntervalWhenHardLimited {
ml.logger.Warn("Memory usage is above hard limit. Forcing a GC.", memstatToZapField(ms))
ms = ml.doGCandReadMemStats()
// Check the limit again to see if GC helped.
aboveSoftLimit = ml.usageChecker.aboveSoftLimit(ms)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be nice to see this factored into a function or closure that is called twice inside section of code.

Copy link
Member Author

Choose a reason for hiding this comment

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

2 out of 4 lines of code have different arguments (the if statement and the log message). I though it will not help that much. If you feel strong, I will do it.

@bogdandrutu bogdandrutu requested a review from jmacd February 22, 2025 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants