Skip to content

fix: do not use new metric instances every Go memstats collection #1792

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

Closed

Conversation

alespour
Copy link

@alespour alespour commented Apr 7, 2025

This PR changes how Go memstats are collected. Instead of creating new ConstMetric during every collection, it (re)uses instances of Gauge or Counter in order to reduce needless memory allocations.

@alespour alespour force-pushed the fix/go-memstats-reuse-metrics branch from b141a7a to 074bca8 Compare April 7, 2025 13:53
@alespour alespour closed this Apr 7, 2025
@alespour alespour reopened this Apr 7, 2025
@alespour alespour force-pushed the fix/go-memstats-reuse-metrics branch from 0c8f9cb to e21d4eb Compare April 7, 2025 14:11
@alespour alespour marked this pull request as ready for review April 7, 2025 14:30
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

This is looking good.

Could you please add some benchmarks? So that we can actually see if we have tangible improvements?

@alespour
Copy link
Author

Benchmark go test -v -bench=BenchmarkGoCollector -benchmem ./prometheus -run BenchmarkGoCollector -benchtime=10s

main:

BenchmarkGoCollector
BenchmarkGoCollector-4   	  346846	     37594 ns/op	   22871 B/op	     111 allocs/op
PASS

#1792

BenchmarkGoCollector
BenchmarkGoCollector-4   	  550576	     32366 ns/op	   18958 B/op	      27 allocs/op
PASS

metric Metric
}

func (m *memStatsMetric) update(memStats *runtime.MemStats) Metric {
Copy link
Member

Choose a reason for hiding this comment

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

This is not concurrency-safe. Metrics collection can happen concurrently, and then all kind of weirdness will happen here.

@beorn7
Copy link
Member

beorn7 commented Apr 10, 2025

Updating the metrics primitives that are meant for direct instrumentation just before a scrape is an anti-pattern. (The "reverse engineering" needed to calculate the counter increment is a good sign that you are working against the intention of the API here.)

Scrapes can happen at any time and in particular concurrently. This PR introduces races.

If you try to work around the concurrency problem by using mutexes, you might easily have a slower metrics collection than before this change.

I also doubt that the allocation savings here really make a dent in the big picture. Encoding the metrics later to be sent out via the network is probably an order of magnitude more effort anyway.

@alespour alespour closed this Apr 10, 2025
@alespour alespour deleted the fix/go-memstats-reuse-metrics branch April 10, 2025 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants