Skip to content
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.

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

fix: drop type params from Go function names #3010

Merged
merged 4 commits into from
Feb 21, 2024

Conversation

kolesnikovae
Copy link
Collaborator

@kolesnikovae kolesnikovae commented Feb 19, 2024

This change involves the removal of type parameters from Go function names during the ingestion process.

The primary rationale behind this modification is that, in Go, function type parameters within profiles are often encountered in unexpected code paths. Allegedly, the Go profiler does not consider the call site when constructing the function name and may include parameters from irrelevant function instantiations. This behavior significantly increases the cardinality of stack traces. For more details, please refer to #2241, specifically this comment.

The elimination of type parameters results in a reduction of cardinality by up to 50% (ingester's heap, 1 hour):

	fix_test.go:24:  * Sample:     6785 -> 3797
	fix_test.go:25:  * Location:   4848 -> 4680
	fix_test.go:26:  * Function:   2801 -> 2724
	fix_test.go:27:  * Strings:    3536 -> 3458

Regarding the impact on PGO: given that we cannot rely on the parameters, no negative impact is expected.

We probably should be more specific about the Go version, but we do not have this information yet. Theoretically, we only have to do this for go < 1.22 / 1.21.6.

Related:

Go:

@kolesnikovae kolesnikovae marked this pull request as ready for review February 19, 2024 07:13
@kolesnikovae kolesnikovae requested a review from a team as a code owner February 19, 2024 07:13
pkg/pprof/fix.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

Can't wait to try this out in our biggest cluster. Do you know if there's a report of that bug somewhere in the go community ?

@simonswine
Copy link
Contributor

@cyriltovena it is linked in the top comment under Go:

@cyriltovena
Copy link
Contributor

I'm blind today ! sorry.

@kolesnikovae kolesnikovae merged commit 6a47d01 into main Feb 21, 2024
19 checks passed
@kolesnikovae kolesnikovae deleted the fix/drop-go-type-params branch February 21, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants