Skip to content

Refactor default runtime metrics tests for Go collector so that default runtime metric set autogenerates #1631

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Sep 23, 2024
Merged

Refactor default runtime metrics tests for Go collector so that default runtime metric set autogenerates #1631

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
13b607a
Enable autogeneration for default runtime metrics list in collectors …
vesari Sep 19, 2024
154e9da
Adapt withDefaultRuntimeMetrics function to work regardless of the Go…
vesari Sep 19, 2024
0a62ed8
Merge remote-tracking branch 'prometheus/main' into refactor-default-…
vesari Sep 19, 2024
3cdd683
Autogenerate go collector test for go1.23
vesari Sep 19, 2024
dcb8936
Modify gen_go_collector_set.go to please linter and regenerate files
vesari Sep 20, 2024
fe0487b
Simplify gen_go_collector_set.go logic by modifying func computeMetri…
vesari Sep 20, 2024
2158a2f
Slight simplification of withDefaultRuntimeMetrics func
vesari Sep 21, 2024
70b1a8b
Refactor withDefaultRuntimeMetrics with generated default runtime met…
vesari Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions prometheus/collectors/gen_go_collector_set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,35 @@ func main() {
descriptions := computeMetricsList()
groupedMetrics := groupMetrics(descriptions)

allDesc := metrics.All()

// Find default metrics.
var defaultRuntimeDesc []metrics.Description
for _, d := range allDesc {
if !internal.GoCollectorDefaultRuntimeMetrics.MatchString(d.Name) {
continue
}
defaultRuntimeDesc = append(defaultRuntimeDesc, d)
}

var defaultRuntimeMetricsList []string

for _, d := range defaultRuntimeDesc {
if trans := rm2prom(d); trans != "" {
Copy link
Member

Choose a reason for hiding this comment

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

What does trans means? Not sure I understand the variable name 🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I was still simplifying a bit as announced ;), so I condensed the piece of code you are referring to. Nonetheless, to answer your question. I maintained the naming of that trans variable to be consistent with similar pre-existing usages in this file and in gen_go_collector_metrics_set.go. Under the hood, the function rm2prom calls internal.RuntimeMetricsToProm and there I could assume that trans stands for "translation".

defaultRuntimeMetricsList = append(defaultRuntimeMetricsList, trans)
}
}

// Generate code.
var buf bytes.Buffer
err = testFile.Execute(&buf, struct {
GoVersion goVersion
Groups []metricGroup
GoVersion goVersion
Groups []metricGroup
DefaultRuntimeMetricsList []string
}{
GoVersion: v,
Groups: groupedMetrics,
GoVersion: v,
Groups: groupedMetrics,
DefaultRuntimeMetricsList: defaultRuntimeMetricsList,
})
if err != nil {
log.Fatalf("executing template: %v", err)
Expand Down Expand Up @@ -186,4 +207,10 @@ func {{ .Name }}() []string {
})
}
{{ end }}

var defaultRuntimeMetrics = []string{
{{- range $metric := .DefaultRuntimeMetricsList }}
{{ $metric | printf "%q"}},
{{- end }}
}
`))
13 changes: 0 additions & 13 deletions prometheus/collectors/go_collector_go120_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package collectors

import "sort"

func withAllMetrics() []string {
return withBaseMetrics([]string{
"go_cgo_go_to_c_calls_calls_total",
Expand Down Expand Up @@ -122,14 +120,3 @@ func withDebugMetrics() []string {
var defaultRuntimeMetrics = []string{
"go_sched_gomaxprocs_threads",
}

func withDefaultRuntimeMetrics(metricNames []string, withoutGC, withoutSched bool) []string {
// If withoutSched is true, exclude "go_sched_gomaxprocs_threads".
if withoutSched {
return metricNames
}
metricNames = append(metricNames, defaultRuntimeMetrics...)
// sorting is required
sort.Strings(metricNames)
return metricNames
}
21 changes: 0 additions & 21 deletions prometheus/collectors/go_collector_go121_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package collectors

import "sort"

func withAllMetrics() []string {
return withBaseMetrics([]string{
"go_cgo_go_to_c_calls_calls_total",
Expand Down Expand Up @@ -177,22 +175,3 @@ var defaultRuntimeMetrics = []string{
"go_gc_gomemlimit_bytes",
"go_sched_gomaxprocs_threads",
}

func withDefaultRuntimeMetrics(metricNames []string, withoutGC, withoutSched bool) []string {
if withoutGC && withoutSched {
// If both flags are true, return the metricNames as is.
return metricNames
} else if withoutGC && !withoutSched {
// If only withoutGC is true, include "go_sched_gomaxprocs_threads" only.
metricNames = append(metricNames, []string{"go_sched_gomaxprocs_threads"}...)
} else if withoutSched && !withoutGC {
// If only withoutSched is true, exclude "go_sched_gomaxprocs_threads".
metricNames = append(metricNames, []string{"go_gc_gogc_percent", "go_gc_gomemlimit_bytes"}...)
} else {
// If neither flag is true, use the default metrics.
metricNames = append(metricNames, defaultRuntimeMetrics...)
}
// sorting is required
sort.Strings(metricNames)
return metricNames
}
21 changes: 0 additions & 21 deletions prometheus/collectors/go_collector_go122_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package collectors

import "sort"

func withAllMetrics() []string {
return withBaseMetrics([]string{
"go_cgo_go_to_c_calls_calls_total",
Expand Down Expand Up @@ -199,22 +197,3 @@ var defaultRuntimeMetrics = []string{
"go_gc_gomemlimit_bytes",
"go_sched_gomaxprocs_threads",
}

func withDefaultRuntimeMetrics(metricNames []string, withoutGC, withoutSched bool) []string {
if withoutGC && withoutSched {
// If both flags are true, return the metricNames as is.
return metricNames
} else if withoutGC && !withoutSched {
// If only withoutGC is true, include "go_sched_gomaxprocs_threads" only.
metricNames = append(metricNames, []string{"go_sched_gomaxprocs_threads"}...)
} else if withoutSched && !withoutGC {
// If only withoutSched is true, exclude "go_sched_gomaxprocs_threads".
metricNames = append(metricNames, []string{"go_gc_gogc_percent", "go_gc_gomemlimit_bytes"}...)
} else {
// If neither flag is true, use the default metrics.
metricNames = append(metricNames, defaultRuntimeMetrics...)
}
// sorting is required
sort.Strings(metricNames)
return metricNames
}
21 changes: 0 additions & 21 deletions prometheus/collectors/go_collector_go123_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

package collectors

import "sort"

func withAllMetrics() []string {
return withBaseMetrics([]string{
"go_cgo_go_to_c_calls_calls_total",
Expand Down Expand Up @@ -211,22 +209,3 @@ var defaultRuntimeMetrics = []string{
"go_gc_gomemlimit_bytes",
"go_sched_gomaxprocs_threads",
}

func withDefaultRuntimeMetrics(metricNames []string, withoutGC, withoutSched bool) []string {
if withoutGC && withoutSched {
// If both flags are true, return the metricNames as is.
return metricNames
} else if withoutGC && !withoutSched {
// If only withoutGC is true, include "go_sched_gomaxprocs_threads" only.
metricNames = append(metricNames, []string{"go_sched_gomaxprocs_threads"}...)
} else if withoutSched && !withoutGC {
// If only withoutSched is true, exclude "go_sched_gomaxprocs_threads".
metricNames = append(metricNames, []string{"go_gc_gogc_percent", "go_gc_gomemlimit_bytes"}...)
} else {
// If neither flag is true, use the default metrics.
metricNames = append(metricNames, defaultRuntimeMetrics...)
}
// sorting is required
sort.Strings(metricNames)
return metricNames
}
22 changes: 22 additions & 0 deletions prometheus/collectors/go_collector_latest_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,28 @@ var memstatMetrics = []string{
"go_memstats_sys_bytes",
}

func withDefaultRuntimeMetrics(metricNames []string, withoutGC, withoutSched bool) []string {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, can you remind me why we need this?

It looks we use it here:

{
			name:     "allow GC",
			rules:    []GoRuntimeMetricsRule{MetricsGC},
			expected: withDefaultRuntimeMetrics(withGCMetrics(), true, false),
		},
		{
			name:     "allow Memory",
			rules:    []GoRuntimeMetricsRule{MetricsMemory},
			expected: withDefaultRuntimeMetrics(withMemoryMetrics(), false, false),
		},
		{
			name:     "allow Scheduler",
			rules:    []GoRuntimeMetricsRule{MetricsScheduler},
			expected: withDefaultRuntimeMetrics(withSchedulerMetrics(), false, true),
		},
		{
			name:     "allow debug",
			rules:    []GoRuntimeMetricsRule{MetricsDebug},
			expected: withDefaultRuntimeMetrics(withDebugMetrics(), false, false),
		},

But it all those cases we expect default metrics, so why not adding defaultRuntimeMetrics always? Is it about duplications? Then let's have dedupSort function for name? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. I also would like for it to be simpler, and maybe there are ways to simplify it that I'm not seeing right now... but in any case it's not only about deduplicating and sorting; it's also about not including metrics (especially when they don't exist in go1.20) according to the different allow/deny list scenarios. In fact it's also used once in TestGoCollectorDenyList. Shall I change the name to dedupSort all the same? Or make any other change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And thanks for taking care of the other 2 problems! <3

Copy link
Member

Choose a reason for hiding this comment

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

. but in any case it's not only about deduplicating and sorting; it's also about not including metrics (especially when they don't exist in go1.20)

but that is solved with defaultRuntimeMetrics generation, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The defaultRuntimeMetrics generation solves the problem of defining what the set of default runtime metrics looks like for a given Go version; in practice this allows adding the right set of default runtime metrics as is, when there are no deny lists at all OR when no deny list matches either all gc or all sched metrics.

But in this function (which lives in a file that has to work with all the supported Go versions indifferently) I chose the approach where in practice I append the non-denied default metrics to the rest of the other requested metrics in a hard-coded way. That is to say, I append a subset of those metrics; a subset that is not dynamically generated, unlike the whole set.

So when it comes to cases when only some of the default runtime metrics have to be filtered out because of deny lists matching gc OR sched (not both: both is a non-problem), the subset to be appended may be different according to the version but I cannot dynamically/automatically reflect those differences in that function: hence all the many conditionals. Shall I think of a different, more dynamic solution?

Copy link
Contributor Author

@vesari vesari Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Incidentally: Arthur just wrote in Slack that we shouldn’t be worrying about supporting go1.20. If that means that the tests for go1.20 can be deleted, then at least half of this problem is solved, as the default runtime metrics list looks exactly the same starting from go 1.21. But maybe there are reasons we should still be keeping them around for a while.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with deleting 1.20 test files, we don't even run them in CI anymore

if withoutGC && withoutSched {
// If both flags are true, return the metricNames as is.
return metricNames
} else if withoutGC && !withoutSched && defaultRuntimeMetrics != nil {
// If only withoutGC is true and the Go version is at least 1.20, include "go_sched_gomaxprocs_threads" only.
metricNames = append(metricNames, []string{"go_sched_gomaxprocs_threads"}...)
} else if withoutSched && !withoutGC && len(defaultRuntimeMetrics) > 1 {
// If only withoutSched is true and the Go version is higher than 1.20, exclude "go_sched_gomaxprocs_threads".
metricNames = append(metricNames, []string{"go_gc_gogc_percent", "go_gc_gomemlimit_bytes"}...)
} else if withoutSched && len(defaultRuntimeMetrics) == 1 {
// If only withoutSched is true and the Go version is 1.20, return the metricNames as is.
return metricNames
} else {
// In any other case, use the default metrics.
metricNames = append(metricNames, defaultRuntimeMetrics...)
}
// sorting is required
sort.Strings(metricNames)
return metricNames
}

func TestGoCollectorMarshalling(t *testing.T) {
reg := prometheus.NewPedanticRegistry()
reg.MustRegister(NewGoCollector(
Expand Down