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.

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

Jump to bottom

proposal: net/http/pprof: function to register all profiles to a mux #71213

Open
DerekTBrown opened this issue Jan 9, 2025 · 8 comments · May be fixed by #71214
Open

proposal: net/http/pprof: function to register all profiles to a mux #71213

DerekTBrown opened this issue Jan 9, 2025 · 8 comments · May be fixed by #71214
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool Proposal
Milestone
Proposal

Comments

@DerekTBrown
Copy link

User Story

  • I have an application that has mux instances.
  • I want to enable net/http/pprof on a specific mux instance.
  • net/http/pprof does not provide an ergonomic interface for attaching the handlers to a specific mux.

Current Options / Alternatives Considered

The net/http/pprof package init function is the recommended path for enabling the pprof handler.1 This method uses the DefaultServeMux:

go/src/net/http/pprof/pprof.go

Lines 95 to 105 in 46b576b

func init() {
prefix := ""
if godebug.New("httpmuxgo121").Value() != "1" {
prefix = "GET "
}
http.HandleFunc(prefix+"/debug/pprof/", Index)
http.HandleFunc(prefix+"/debug/pprof/cmdline", Cmdline)
http.HandleFunc(prefix+"/debug/pprof/profile", Profile)
http.HandleFunc(prefix+"/debug/pprof/symbol", Symbol)
http.HandleFunc(prefix+"/debug/pprof/trace", Trace)
}

If the user wants to mount the pprof handlers using a non-default mux, they must do this by manually enumerating all of the available profilers2. For example:

	mux := http.NewServeMux()
	mux.HandleFunc("/debug/pprof/", pprof.Index)
	mux.HandleFunc("/debug/pprof/cmdline/", pprof.Cmdline)
	mux.HandleFunc("/debug/pprof/profile/", pprof.Profile)
	mux.HandleFunc("/debug/pprof/symbol/", pprof.Symbol)
	mux.HandleFunc("/debug/pprof/trace/", pprof.Trace)

Proposal

This experience could be made better for users by moving the logic in the init function into a separate method (with the mux as an argument), then invoking this within the default package init function.

Footnotes

  1. https://pkg.go.dev/net/http/pprof#:~:text=To%20use%20pprof%2C%20link%20this%20package%20into%20your%20program%3A ↩

  2. https://pkg.go.dev/net/http/pprof#:~:text=If%20you%20are%20not%20using%20DefaultServeMux%2C%20you%20will%20have%20to%20register%20handlers%20with%20the%20mux%20you%20are%20using. ↩
@DerekTBrown
Copy link
Author

Here is an example of where a re-implementation of this handler caused issues:

argoproj/argo-rollouts#4038

@DerekTBrown DerekTBrown linked a pull request Jan 10, 2025 that will close this issue
Open
@gabyhelp
Copy link

Related Issues

Related Code Changes

Related Discussions

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@ianlancetaylor
Copy link
Member

Closely related to #42834.

@seankhliao seankhliao changed the title [feat] Extend net/http/pprof to support enabling on non-default ServeMux proposal: net/http/pprof: function to register all profiles to a mux Jan 10, 2025
@gopherbot gopherbot added this to the Proposal milestone Jan 10, 2025
@tmthrgd
Copy link
Contributor

tmthrgd commented Jan 10, 2025

A relatively straight forward way to do this currently is:

import _ "net/http/pprof"

...

mux.Handle("/debug/pprof/", http.DefaultServerMux)

and then rely on the init registration against DefaultServerMux.

@gabyhelp gabyhelp added the LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool label Jan 10, 2025
@seankhliao
Copy link
Member

seankhliao commented Jan 12, 2025 •
edited
Loading

CL 641858 proposes

func AttachHandlers(prefix string, mux *http.ServeMux)

But I don't think it's necessary to expose prefix as:

  1. it's an implementation detail for optional method scoping that can't effectively be used outside of the standard library
  2. using any other non-method prefix will likely break the index page.

#42834 (comment) proposes a simpler:

func RegisterHandlers(mux *http.ServeMux)

@DerekTBrown
Copy link
Author

Closely related to #42834.

IIUC, this issue (#71213) is somewhat of a pre-requisite to taking action on #42834. We need an ergonomic way to initialize a non-default mux before we can stop using the default mux.

@DerekTBrown
Copy link
Author

CL 641858 proposes

func AttachHandlers(prefix string, mux *http.ServeMux)
But I don't think it's necessary to expose prefix as:

  1. it's an implementation detail for optional method scoping that can't effectively be used outside of the standard library
  2. using any other non-method prefix will likely break the index page.

#42834 (comment) proposes a simpler:

func RegisterHandlers(mux *http.ServeMux)

Makes sense. I revised the CL to not expose prefix.

@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Jan 15, 2025
@ianlancetaylor
Copy link
Member

CC @neild

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
LibraryProposal Issues describing a requested change to the Go standard library or x/ libraries, but not to a tool Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
6 participants
@tmthrgd @ianlancetaylor @DerekTBrown @gopherbot @seankhliao @gabyhelp