gocollector: Added options to Go Collector for changing the collections

[bwplotka]

Fixes #983

Also:

• Improved docs
• fixed TestMemStatsEquivalence, it was noop before (:
• Removed gc_cpu_fraction metric completely, since it's not working completely for Go1.17+

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

[bwplotka]

cc @mknyszek - we would love your eyes on this (:

[bwplotka]

Fun fact. This test was noop, because typ.Kind was always struct - we want field kind instead 🙈

[mknyszek]

oops! good catch.

[bwplotka]

After this is merged I will attempt to fix other issues:

#994
#995
#1026

I would also (in separate PR) propose to default to just goRuntimeMemStats for now instead of goRuntimeMemStats | goRuntimeMetrics set. I think migration to new metrics is important, but it has to be opt in for now IMO. We impacted a lot ppl by adding two digit more of metrics and I am kind of missing the reason (users don't know how to leverage them yet) 🤔 Not sure, yet, let's discuss. (:

[kakkoyun]

Overall looks good.

prometheus/collectors/go_collector.go file missing build flags to exclude versions older than <1.17, I guess, considering CI failures.

[bwplotka]

Fixed @kakkoyun

@mknyszek I don't know much about runtime/metrics.Read and nothing about "stop-the-world" problem was written into its documentation (go docs). I assume we can remove section about the consequences of stop-the-world logic from Go Collector, right? Could you confirm? (: I mean the following one:

// NewGoCollector returns a collector that exports metrics about the current Go // process. This includes memory stats. To collect those, runtime.ReadMemStats // is called. This requires to “stop the world”, which usually only happens for // garbage collection (GC). Take the following implications into account when // deciding whether to use the Go collector: // // 1. The performance impact of stopping the world is the more relevant the more // frequently metrics are collected. However, with Go1.9 or later the // stop-the-world time per metrics collection is very short (~25µs) so that the // performance impact will only matter in rare cases. However, with older Go // versions, the stop-the-world duration depends on the heap size and can be // quite significant (~1.7 ms/GiB as per // https://go-review.googlesource.com/c/go/+/34937). // // 2. During an ongoing GC, nothing else can stop the world. Therefore, if the // metrics collection happens to coincide with GC, it will only complete after // GC has finished. Usually, GC is fast enough to not cause problems. However, // with a very large heap, GC might take multiple seconds, which is enough to // cause scrape timeouts in common setups. To avoid this problem, the Go // collector will use the memstats from a previous collection if // runtime.ReadMemStats takes more than 1s. However, if there are no previously // collected memstats, or their collection is more than 5m ago, the collection // will block until runtime.ReadMemStats succeeds. 

[mknyszek]

Hey sorry for the delay. If it matters anymore, LGTM! Thanks, and sorry I never got around to this.

[mknyszek]

@mknyszek I don't know much about runtime/metrics.Read and nothing about "stop-the-world" problem was written into its documentation (go docs). I assume we can remove section about the consequences of stop-the-world logic from Go Collector, right? Could you confirm? (: I mean the following one:

As of Go 1.15 (golang/go#19812) I believe that problem is resolved for ReadMemStats. Before then, ReadMemStats would block until the GC mark phase was complete, meaning it could wait a while. Now it can stop-the-world at any time.

That's still less efficient than runtime/metrics.Read which never stops the world; it ensures metric consistency in a much more efficient way. Note that ReadMemStats still incurs a whole-program latency hit (usually not a problem in practice if you're not calling it very often) that runtime/metrics.Read does not.