Add default Go runtime metrics for /gc/gogc:percent, /gc/gomemlimit:bytes, /sched/gomaxprocs:threads #1559

vesari · 2024-07-12T07:44:27Z

This PR adds 3 of the additional Go runtime metrics to the Prometheus default Go metrics, as recommended in this proposal #67120, namely /gc/gogc:percent, /gc/gomemlimit:bytes and /sched/gomaxprocs:threads. As agreed with this repo's maintainers, /sched/latencies:seconds will be dealt with in a separate PR.

…o runtime metrics Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

github-actions · 2024-07-12T07:44:42Z

PR body does not contain a valid type of change. Please refer to CONTRIBUTING.md for more information.

prometheus/go_collector.go

bwplotka

Looking good! Some comments 💪🏽

prometheus/go_collector.go

go.mod

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

…ult-go-runtime-metrics

bwplotka

Amazing work! It's not easy flow and you managed to get this working! However, there is a bit simpler (lower complexity) way of doing this (unless I missed sth), mentioned in the comment.

prometheus/collectors/go_collector_latest.go

bwplotka · 2024-07-29T10:13:12Z

prometheus/collectors/go_collector_latest_test.go

@@ -117,6 +118,7 @@ func TestGoCollectorAllowList(t *testing.T) {
 			reg.MustRegister(NewGoCollector(
 				WithGoCollectorMemStatsMetricsDisabled(),
 				WithGoCollectorRuntimeMetrics(test.rules...),
+				WithGoCollectorRuntimeEnvVarsMetricsDisabled(),


Can we test somewhere how it looks with those enabled?

bwplotka · 2024-07-29T10:22:21Z

prometheus/go_collector.go

@@ -279,3 +279,8 @@ type memStatsMetrics []struct {
 	eval    func(*runtime.MemStats) float64
 	valType ValueType
 }
+
+type runtimeEnvVarsMetrics []struct { // I couldn't come up with a better name. Any suggestions?


Nice! I see where you go with this. This way would totally work, but it's extra structures in many places. There is a way to simplify this by reusing generic flow, so by default adding appropriate regexes to RuntimeMetricRules in defaultGoCollectorOptions (also you can remove the weird dead code (comment)) -

client_golang/prometheus/go_collector_latest.go

Line 162 in 6478f56

func defaultGoCollectorOptions() internal.GoCollectorOptions {

I think we can add that by default, then fix tests. We could even test if WithoutGoCollectorRuntimeMetrics can turn those off 🤗 WDYT?

vesari · 2024-07-29T12:44:43Z

Thank you for all your points, I'll make changes accordingly

bwplotka · 2024-07-31T10:10:13Z

Another thing, I would use our auto-name conversion and add new metrics as runtime metrics would do so:

		"go_gc_gogc_percent",
		"go_gc_gomemlimit_bytes",
"go_sched_gomaxprocs_threads",

WDYT?

vesari · 2024-07-31T11:00:53Z

Another thing, I would use our auto-name conversion and add new metrics as runtime metrics would do so:
		"go_gc_gogc_percent",
		"go_gc_gomemlimit_bytes",
"go_sched_gomaxprocs_threads",
WDYT?

Yes, it makes sense, thanks for noticing the discrepancy

…ult-go-runtime-metrics

…sts accordingly Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

vesari · 2024-08-06T16:21:35Z

About withDefaultRuntimeMetrics function, since it's just for tests, I didn't bother encapsulating the bool arguments in an options struct.

ArthurSens

Hello hello, getting back from PTO now :)

@vesari @bwplotka, are you two aware that we generate the go runtime metrics from a script?

https://github.com/prometheus/client_golang/blob/main/prometheus/collectors/gen_go_collector_set.go

I'm confused that we don't see any failures in the CI though 🤔

vesari · 2024-08-07T07:09:03Z

Hello hello, getting back from PTO now :)

@vesari @bwplotka, are you two aware that we generate the go runtime metrics from a script?

https://github.com/prometheus/client_golang/blob/main/prometheus/collectors/gen_go_collector_set.go

I'm confused that we don't see any failures in the CI though 🤔

@ArthurSens hello and welcome back! Yes, I'm aware of that. This PR doesn't change or influence that. May I ask what failures you were expecting? Thanks! :)

bwplotka

Nice, small nits around testing only, otherwise good!

bwplotka · 2024-08-07T07:11:34Z

prometheus/go_collector_latest.go

@@ -153,7 +155,9 @@ func defaultGoCollectorOptions() internal.GoCollectorOptions {
 			"/gc/heap/frees-by-size:bytes":  goGCHeapFreesBytes,
 		},
 		RuntimeMetricRules: []internal.GoCollectorRule{
-			//{Matcher: regexp.MustCompile("")},
+			{
+				Matcher: regexp.MustCompile(`\/gc\/gogc:percent|\/gc\/gomemlimit:bytes|\/sched\/gomaxprocs:threads`),


hm, do we really need to escape /? It's not sed, it's RE2 language 🤔

prometheus/go_collector_latest_go120_test.go

bwplotka · 2024-08-07T07:13:23Z

prometheus/go_collector_latest_test.go

-			// Default, only MemStats.
-			expectedFQNameSet: addExpectedRuntimeMemStats(expectedBaseMetrics()),
+			// Default, only MemStats and default Runtime metrics.
+			opts: internal.GoCollectorOptions{


let's use default function then?

Do you mind using our new internal.GoCollector default etc here?

Signed-off-by: bwplotka <bwplotka@gmail.com>

Addressed comments

bwplotka

One tiny thing to do, then LGTM! thanks!

#1559 (comment)

prometheus/collectors/go_collector_go120_test.go

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

ArthurSens · 2024-08-09T14:45:14Z

I'll add some work to myself: Understand why CI didn't complain that we changed generated files without the appropriate changes to prometheus/gen_go_collector_metrics_set.go!

vesari · 2024-08-09T15:12:58Z

I'll add some work to myself: Understand why CI didn't complain that we changed generated files without the appropriate changes to prometheus/gen_go_collector_metrics_set.go!

Thank you for looking into it! I don't know if that makes any difference, but technically I didn't change any autogenerated files. I just added 2 separate files :)

bwplotka · 2024-08-09T19:26:24Z

Yup, we checked. The CI is NOT running this generative script. We only check if metrics given by runtime does is what we expect in those generated _test files and then complain with instruction to potentially rerun that script.

Let's discuss if that CI step is needed in the ticket you added #1575

bwplotka

Thanks! Let's go!!! 💪🏽

Add go_gomaxprocs, go_gogc_percent and go_gomemlimit to the default G…

cd8445b

…o runtime metrics Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

bwplotka reviewed Jul 12, 2024

View reviewed changes

prometheus/go_collector.go Outdated Show resolved Hide resolved

bwplotka reviewed Jul 12, 2024

View reviewed changes

prometheus/go_collector.go Outdated Show resolved Hide resolved

prometheus/go_collector.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

vesari added 8 commits July 19, 2024 15:14

Move newly added metrics out of base metrics and into goCollector

5d64fb7

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Rethink struct for newly added metrics, adapt and add tests

17b3065

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Simplify new metrics reading

5d9b680

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Correct loop, add debugging lines

4490f8d

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Make goRuntimeEnvVarsMetrics function Go version dependent

fb0afbe

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Fix go mod

0b3a6f0

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Remove debuggin line

6478f56

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Merge remote-tracking branch 'prometheus/main' into add-three-to-defa…

e4a5ade

…ult-go-runtime-metrics

bwplotka requested changes Jul 29, 2024

View reviewed changes

vesari changed the title ~~WIP: add default Go runtime metrics for /gc/gogc:percent, /gc/gomemlimit:bytes, /sched/gomaxprocs:threads~~ Add default Go runtime metrics for /gc/gogc:percent, /gc/gomemlimit:bytes, /sched/gomaxprocs:threads Jul 29, 2024

vesari marked this pull request as ready for review July 29, 2024 12:36

vesari added 3 commits August 3, 2024 16:10

Merge remote-tracking branch 'prometheus/main' into add-three-to-defa…

a04c891

…ult-go-runtime-metrics

Move default runtime metrics into the runtime metrics flow, change te…

20ebd6c

…sts accordingly Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

Go version expected default runtime metrics map for tests

ed4053d

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

vesari force-pushed the add-three-to-default-go-runtime-metrics branch from ab9d801 to ed4053d Compare August 6, 2024 15:56

vesari requested a review from bwplotka August 6, 2024 16:00

ArthurSens reviewed Aug 6, 2024

View reviewed changes

bwplotka approved these changes Aug 7, 2024

View reviewed changes

bwplotka and others added 3 commits August 9, 2024 11:01

1.21 update.

b8a9eb7

Signed-off-by: bwplotka <bwplotka@gmail.com>

Addressed comments on Arianna's PR.

23879d6

Signed-off-by: bwplotka <bwplotka@gmail.com>

Merge pull request #1 from prometheus/newmetrics

cd5a2a6

Addressed comments

Merge branch 'main' into add-three-to-default-go-runtime-metrics

5eb377e

bwplotka approved these changes Aug 9, 2024

View reviewed changes

bwplotka reviewed Aug 9, 2024

View reviewed changes

prometheus/collectors/go_collector_go120_test.go Show resolved Hide resolved

Use default GoCollector func in test

4a2cb15

Signed-off-by: Arianna Vespri <arianna.vespri@yahoo.it>

ArthurSens mentioned this pull request Aug 9, 2024

gocollector tests: Adjust CI to tell us when generated files were manually changed + refactor? #1575

Open

bwplotka approved these changes Aug 9, 2024

View reviewed changes

bwplotka merged commit 3ad2722 into prometheus:main Aug 9, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default Go runtime metrics for /gc/gogc:percent, /gc/gomemlimit:bytes, /sched/gomaxprocs:threads #1559

Add default Go runtime metrics for /gc/gogc:percent, /gc/gomemlimit:bytes, /sched/gomaxprocs:threads #1559

vesari commented Jul 12, 2024 •

edited

Loading

github-actions bot commented Jul 12, 2024

bwplotka left a comment

bwplotka left a comment

bwplotka Jul 29, 2024

bwplotka Jul 29, 2024

vesari commented Jul 29, 2024

bwplotka commented Jul 31, 2024

vesari commented Jul 31, 2024

vesari commented Aug 6, 2024

ArthurSens left a comment

vesari commented Aug 7, 2024

bwplotka left a comment

bwplotka Aug 7, 2024

bwplotka Aug 7, 2024

bwplotka Aug 9, 2024

bwplotka left a comment •

edited

Loading

ArthurSens commented Aug 9, 2024

vesari commented Aug 9, 2024

bwplotka commented Aug 9, 2024

bwplotka left a comment

Add default Go runtime metrics for /gc/gogc:percent, /gc/gomemlimit:bytes, /sched/gomaxprocs:threads #1559

Add default Go runtime metrics for /gc/gogc:percent, /gc/gomemlimit:bytes, /sched/gomaxprocs:threads #1559

Conversation

vesari commented Jul 12, 2024 • edited Loading

github-actions bot commented Jul 12, 2024

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Jul 29, 2024

Choose a reason for hiding this comment

bwplotka Jul 29, 2024

Choose a reason for hiding this comment

vesari commented Jul 29, 2024

bwplotka commented Jul 31, 2024

vesari commented Jul 31, 2024

vesari commented Aug 6, 2024

ArthurSens left a comment

Choose a reason for hiding this comment

vesari commented Aug 7, 2024

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Aug 7, 2024

Choose a reason for hiding this comment

bwplotka Aug 7, 2024

Choose a reason for hiding this comment

bwplotka Aug 9, 2024

Choose a reason for hiding this comment

bwplotka left a comment • edited Loading

Choose a reason for hiding this comment

ArthurSens commented Aug 9, 2024

vesari commented Aug 9, 2024

bwplotka commented Aug 9, 2024

bwplotka left a comment

Choose a reason for hiding this comment

vesari commented Jul 12, 2024 •

edited

Loading

bwplotka left a comment •

edited

Loading