Use simpler locking in the Go 1.17 collector by mknyszek · Pull Request #975 · prometheus/client_golang (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation5 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

mknyszek

A previous PR made it so that the Go 1.17 collector locked only around
uses of rmSampleBuf, but really that means that Metric values may be
sent over the channel containing some values from future metrics.Read
calls. While generally-speaking this isn't a problem, we lose any
consistency guarantees provided by the runtime/metrics package.

Also, that optimization to not just lock around all of Collect was
premature. Truthfully, Collect is called relatively infrequently, and
its critical path is fairly fast (10s of µs). To prove it, this test
also adds a benchmark.

Signed-off-by: Michael Anthony Knyszek mknyszek@google.com

@mknyszek

@kakkoyun

@mknyszek Could you attach a benchmark comparison if it's possible? For documentation purposes.

kakkoyun

Choose a reason for hiding this comment

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

LGTM. I'll wait for @beorn7's comments since he has more context and elven eyes :)

beorn7

Choose a reason for hiding this comment

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

Thank you very much.

@mknyszek

A previous PR made it so that the Go 1.17 collector locked only around uses of rmSampleBuf, but really that means that Metric values may be sent over the channel containing some values from future metrics.Read calls. While generally-speaking this isn't a problem, we lose any consistency guarantees provided by the runtime/metrics package.

Also, that optimization to not just lock around all of Collect was premature. Truthfully, Collect is called relatively infrequently, and its critical path is fairly fast (10s of µs). To prove it, this change also adds a benchmark.

name old time/op new time/op delta GoCollector-16 43.7µs ± 2% 43.2µs ± 2% ~ (p=0.190 n=9+9)

Note that because the benchmark is single-threaded it actually looks like it might be getting slightly faster, because all those Collect calls for the Metrics are direct calls instead of interface calls.

Signed-off-by: Michael Anthony Knyszek mknyszek@google.com

@mknyszek

kakkoyun pushed a commit that referenced this pull request

May 13, 2022

@mknyszek @kakkoyun

A previous PR made it so that the Go 1.17 collector locked only around uses of rmSampleBuf, but really that means that Metric values may be sent over the channel containing some values from future metrics.Read calls. While generally-speaking this isn't a problem, we lose any consistency guarantees provided by the runtime/metrics package.

Also, that optimization to not just lock around all of Collect was premature. Truthfully, Collect is called relatively infrequently, and its critical path is fairly fast (10s of µs). To prove it, this change also adds a benchmark.

name old time/op new time/op delta GoCollector-16 43.7µs ± 2% 43.2µs ± 2% ~ (p=0.190 n=9+9)

Note that because the benchmark is single-threaded it actually looks like it might be getting slightly faster, because all those Collect calls for the Metrics are direct calls instead of interface calls.

Signed-off-by: Michael Anthony Knyszek mknyszek@google.com

kakkoyun added a commit that referenced this pull request

Jul 6, 2022

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

A previous PR made it so that the Go 1.17 collector locked only around uses of rmSampleBuf, but really that means that Metric values may be sent over the channel containing some values from future metrics.Read calls. While generally-speaking this isn't a problem, we lose any consistency guarantees provided by the runtime/metrics package.

Also, that optimization to not just lock around all of Collect was premature. Truthfully, Collect is called relatively infrequently, and its critical path is fairly fast (10s of µs). To prove it, this change also adds a benchmark.

name old time/op new time/op delta GoCollector-16 43.7µs ± 2% 43.2µs ± 2% ~ (p=0.190 n=9+9)

Note that because the benchmark is single-threaded it actually looks like it might be getting slightly faster, because all those Collect calls for the Metrics are direct calls instead of interface calls.

Signed-off-by: Michael Anthony Knyszek mknyszek@google.com

Replace io.ReadAll with bytes.Buffer.ReadFrom. Both need to resize a buffer until they have finished reading; the former increases by 1.25x each time while the latter uses 2x.

Also added a benchmark to demonstrate the benefit: name old time/op new time/op delta Client/4KB-8 35.9µs ± 4% 35.3µs ± 3% ~ (p=0.310 n=5+5) Client/50KB-8 83.1µs ± 8% 69.5µs ± 1% -16.37% (p=0.008 n=5+5) Client/1000KB-8 891µs ± 6% 750µs ± 0% -15.83% (p=0.016 n=5+4) Client/2000KB-8 1.74ms ± 2% 1.35ms ± 1% -22.72% (p=0.008 n=5+5)

name old alloc/op new alloc/op delta Client/4KB-8 20.2kB ± 0% 20.4kB ± 0% +1.26% (p=0.008 n=5+5) Client/50KB-8 218kB ± 0% 136kB ± 0% -37.65% (p=0.008 n=5+5) Client/1000KB-8 5.88MB ± 0% 2.11MB ± 0% -64.10% (p=0.008 n=5+5) Client/2000KB-8 11.7MB ± 0% 4.2MB ± 0% -63.93% (p=0.008 n=5+5)

name old allocs/op new allocs/op delta Client/4KB-8 75.0 ± 0% 72.0 ± 0% -4.00% (p=0.008 n=5+5) Client/50KB-8 109 ± 0% 98 ± 0% -10.09% (p=0.079 n=4+5) Client/1000KB-8 617 ± 0% 593 ± 0% -3.89% (p=0.008 n=5+5) Client/2000KB-8 1.13k ± 0% 1.09k ± 0% -3.27% (p=0.008 n=5+5)

Signed-off-by: Bryan Boreham bjboreham@gmail.com

The Go runtime/metrics package currently exports extremely granular histograms. Exponentially bucket any histogram with unit "seconds" or "bytes" instead to dramatically reduce the number of buckets, and thus the number of metrics.

This change also adds a test to check for expected cardinality to prevent cardinality surprises in the future.

Signed-off-by: Michael Anthony Knyszek mknyszek@google.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Update examples/random/main.go: prometheus.NewBuildInfoCollector is deprecated. Use collectors.NewBuildInfoCollector instead.

Signed-off-by: alissa-tung alissa-tung@outlook.com

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

Fixes #983

Also:

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

Fixes #967

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Co-authored-by: Bartlomiej Plotka bwplotka@gmail.com Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Co-authored-by: Bartlomiej Plotka bwplotka@gmail.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Co-authored-by: Bartlomiej Plotka bwplotka@gmail.com Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

Co-authored-by: Bartlomiej Plotka bwplotka@gmail.com

Co-authored-by: Kemal Akkoyun kakkoyun@gmail.com Co-authored-by: Michael Knyszek mknyszek@google.com Co-authored-by: Bryan Boreham bjboreham@gmail.com Co-authored-by: Kemal Akkoyun kakkoyun@users.noreply.github.com Co-authored-by: alissa-tung alissa-tung@outlook.com Co-authored-by: Bartlomiej Plotka bwplotka@gmail.com