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

[vesari]

In this PR I modify the gen_go_collector_set.go file so that the default runtime metrics list gets autogenerated (like a few test helpers functions already did) so that it can vary automatically according to the Go versions.
This is tied to the #1575 issue.

[vesari]

Once I'll have fixed the linters' complaints, I might still do some refactoring in gen_go_collector_set.go where probably the function computeMetricsList() can be changed to reduce repetitions happening in that file after I modified it.

[ArthurSens]

Does it make sense to add a Github Action workflow that runs make generate-go-collector-test-files and asserts that there's no git diff?

[ArthurSens]

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

[vesari]

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".

[vesari]

Does it make sense to add a Github Action workflow that runs make generate-go-collector-test-files and asserts that there's no git diff?

I haven't looked into what GitHub Actions does right now, but if as things stand there's no such check, then I guess it wouldn't hurt. I wonder if that would be for another PR though. On a side note, one thing I noticed is that before I merged the main branch into mine, the autogeneration included the test file for go1.20, but afterwards it wouldn't anymore, as it appears to be excluded from that process. So I wonder, are we foreseeing to discontinue support for go1.20 anytime soon? If not, was there any reason for that exclusion? @bwplotka and @kakkoyun asking you as well :D

[bwplotka]

LGTM, I am happy to solve 2 other problems (2 generative scripts instead of one and CI job for no manual changes) in other PRs.

However I want double check if we can get rid of this complex withDefaultRuntimeMetrics function... 🤔 Proposed something

[bwplotka]

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? 🤔

[vesari]

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?

[vesari]

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

[bwplotka]

. 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?

[vesari]

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?

[vesari]

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.

[ArthurSens]

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

[bwplotka]

And thanks for helping 💪🏽

[vesari]

I could simplify the function a bit, but it's still the same not completely dynamic approach. Maybe I can think of something more future proof.

[vesari]

Ok, I implemented another approach. Now the default runtime metrics subsets (needed to be included in or excluded from the final selection of metrics) are also generated. This makes withDefaultRuntimeMetrics func cleaner and more resilient. I'm still leaving it in the go_collector_latest_test.go for better visibility, rather than moving it into gen_go_collector_set.go

[bwplotka]

Thanks! Let me propose what I meant (or realize there is no better approach) in a different PR, this is already a lot better thank You!

[vesari]

Thanks! Let me propose what I meant (or realize there is no better approach) in a different PR, this is already a lot better thank You!

Thank you! I wouldn't be surprised if there were better approaches and I'd be very happy if you could come up with one 👯 . I know it's not ideal to have so many similar-looking variables (outside of functions, on top of that); I mainly tried to keep what's in the Go templates to a minimum, as not everybody finds them super readable 😝