Update default Go metrics #564
Conversation
SuperQ
requested review from
fishy,
kylelemons and
pacejackson
and removed request for
a team
Re-register the Go metrics collector with new baseplate defaults that include Go `runtime/metrics` scheduler metrics. Signed-off-by: SuperQ <[email protected]>
SuperQ
force-pushed
the
superq/more_go_metrics
branch
from
5a11774
to
af6c6fc
Compare
SuperQ
requested review from
pacejackson
and removed request for
fishy and
kylelemons
|
Yes, somehow when I hit the "re-request review", github removed @kylelemons and @fishy. 😕 |
| // Unregister the default GoCollector. | ||
| prometheus.Unregister(collectors.NewGoCollector()) |
There was a problem hiding this comment.
🔕 why do we have to do this :sigh:, why isn't that the default?
There was a problem hiding this comment.
Because the default registry includes some default settings for Go metrics to make it easy for end users.
Customizing the default registry is a bit awkward. We could avoid this by completely eliminating use of the default registry in baseplate.go. But, that means we would need to have user switch to promauto.With(reg)....
So, we do a little bit of dance to get around it.
There was a problem hiding this comment.
we are actually already using promauto.With(reg) because of f68284f
There was a problem hiding this comment.
Yea, I'm not sure that was the right thing to do. :-/
| "go_memstats_stack_inuse_bytes", | ||
| "go_memstats_stack_sys_bytes", | ||
| "go_memstats_sys_bytes", | ||
| "go_sched_gomaxprocs_threads", |
There was a problem hiding this comment.
🔕 this one seems to be the diff between 1.18 and 1.19?
There was a problem hiding this comment.
Yup, new feature in Go 1.19.
There was a problem hiding this comment.
I think I would skip the build tags and just do
expectedMetrics := ...
if !strings.HasPrefix(runtime.Version(), "go1.18") {
expectedMetrics = append(...)
}
Or you could use https://pkg.go.dev/golang.org/x/mod/semver if you want to be a bit more precise.
There was a problem hiding this comment.
Sure, I can do that.
There was a problem hiding this comment.
So, I thought about this a bit.
- Go doesn't actually use semver, so
semver.Compare()is going to be messy. - The build tags match Go version >=, so its a bit future-proof .
I think we need to keep this testing the way it is. (This is how upstream Prometheus client_golang does testing)
| "go_memstats_stack_inuse_bytes", | ||
| "go_memstats_stack_sys_bytes", | ||
| "go_memstats_sys_bytes", | ||
| "go_sched_gomaxprocs_threads", |
There was a problem hiding this comment.
I think I would skip the build tags and just do
expectedMetrics := ...
if !strings.HasPrefix(runtime.Version(), "go1.18") {
expectedMetrics = append(...)
}
Or you could use https://pkg.go.dev/golang.org/x/mod/semver if you want to be a bit more precise.
|
Should we merge this? |
Closes #
💸 TL;DR
Re-register the Go metrics collector with new baseplate defaults that include Go
runtime/metricsscheduler metrics.📜 Details
Design Doc
Jira
🧪 Testing Steps / Validation
✅ Checks