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: disable Go names decoration #2976

Merged
merged 2 commits into from
Jan 31, 2024
Merged

Conversation

kolesnikovae
Copy link
Collaborator

@kolesnikovae kolesnikovae commented Jan 31, 2024

Starting from Go 1.21.0, type parameters are part of the function name in pprof, which often makes them unreadable

image

The change reverts #2343 (replaces Go type parameters with ellipses [...]) as it makes it impossible to find the selected node in the source profile in backend (needed for #2228, and #2727 and https://github.com/grafana/pyroscope-app-plugin/issues/207).

The presentation aspects are to be managed on the frontend. Ideally, there should be an option for user to disable all the decorations. Note that a function name change usually causes changes in the tree/flamegraph structure, which we used to fix. I believe we don't have to do this.

@kolesnikovae kolesnikovae marked this pull request as ready for review January 31, 2024 03:31
@kolesnikovae kolesnikovae requested a review from a team as a code owner January 31, 2024 03:31
@cyriltovena
Copy link
Contributor

cc @aocenas We're going to need some sort of function sanitizer on the frontend but still able to access to original function for programatic need.

@cyriltovena cyriltovena merged commit 369ca83 into main Jan 31, 2024
19 checks passed
@cyriltovena cyriltovena deleted the fix/disable-go-names-decoration branch January 31, 2024 07:52
@aocenas
Copy link
Member

aocenas commented Jan 31, 2024

@cyriltovena I assume we could just port this function https://github.com/grafana/pyroscope/pull/2343/files#diff-e7b8e2da222a25feb1fdc03174dd0d9783102c532048d0bd2945ab9ef6c87c8bR10 to the frontend right? Is it safe to apply to any code (non Go code)? It seems like @kolesnikovae suggests as much in the comment but wonder if there is some caveat or not.

@cyriltovena
Copy link
Contributor

I would not do it for any other language just in case I suggest we found this [go.shape......] and replace it with [...] that should be good enough and only appear in go.

Again the original must still be accessible on click event.

@cyriltovena
Copy link
Contributor

connectrpc.com/connect.NewUnaryHandler[go.shape.struct { github.com/grafana/pyroscope/api/gen/proto/go/types/v1.state google.golang.org/protobuf/internal/impl.MessageState; github.com/grafana/pyroscope/api/gen/proto/go/types/v1.sizeCache int32; github.com/grafana/pyroscope/api/gen/proto/go/types/v1.unknownFields []uint8; Matchers []string "protobuf:\"bytes,1,rep,name=matchers,proto3\" json:\"matchers,omitempty\""; Start int64 "protobuf:\"varint,2,opt,name=start,proto3\" json:\"start,omitempty\""; End int64 "protobuf:\"varint,3,opt,name=end,proto3\" json:\"end,omitempty\"" },go.shape.struct { github.com/grafana/pyroscope/api/gen/proto/go/types/v1.state google.golang.org/protobuf/internal/impl.MessageState; github.com/grafana/pyroscope/api/gen/proto/go/types/v1.sizeCache int32; github.com/grafana/pyroscope/api/gen/proto/go/types/v1.unknownFields []uint8; Names []string "protobuf:\"bytes,1,rep,name=names,proto3\" json:\"names,omitempty\"" }].func2

@aocenas
Copy link
Member

aocenas commented Jan 31, 2024

I would not do it for any other language just in case

Yeah but we still don't have any information about the language from the backend do we?

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.

4 participants