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

Set up a pprof server with optional mutex profiling in cmdutil #216

Merged
merged 11 commits into from
Aug 6, 2024
81 changes: 81 additions & 0 deletions cmdutil/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ package cmdutil

import (
"context"
"net/http"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put the pprof server in the debug package. It is used for debugging

"net/http/pprof"
"runtime"
"time"

"github.com/oklog/run"
)
Expand Down Expand Up @@ -93,3 +97,80 @@ func MultiServer(servers ...Server) Server {
StopFunc: s.Stop,
}
}

// NewPprofServer sets up a pprof server with optional mutex profiling.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have this commented out code? Can we just remove it?

// func NewPprofServer(addr string, enableMutexProfiling bool) Server {
// if addr == "" {
// addr = "127.0.0.1:9998" // Default port
// }

// return NewContextServer(func(ctx context.Context) error {
// if enableMutexProfiling {
// runtime.SetMutexProfileFraction(2)
// }

// pprofSrv := &http.Server{
// Addr: addr,
// Handler: http.HandlerFunc(pprof.Index),
// }

// go func() {
// <-ctx.Done()
// pprofSrv.Close()
// }()

// return pprofSrv.ListenAndServe()
// })
// }

// ProfileConfig holds the configuration for the pprof server.
type ProfileConfig struct {
Addr string
Profiles []string
}

// NewPprofServer sets up a pprof server with optional mutex profiling and configurable profiling types.
func NewPprofServer(config ProfileConfig) Server {
if config.Addr == "" {
config.Addr = "127.0.0.1:9998" // Default port
}

return NewContextServer(func(ctx context.Context) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should return a *Server type and then we can build a context server if needed. That follows the debug.Server pattern:
https://github.com/heroku/x/blob/master/cmdutil/debug/debug.go#L48-L52

mux := http.NewServeMux()

profileHandlers := map[string]http.HandlerFunc{
"index": pprof.Index,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the handlers should be configurable. We don't need all of these profile types most of the time.

"cmdline": pprof.Cmdline,
"profile": pprof.Profile,
"symbol": pprof.Symbol,
"heap": pprof.Handler("heap").ServeHTTP,
"goroutine": pprof.Handler("goroutine").ServeHTTP,
"threadcreate": pprof.Handler("threadcreate").ServeHTTP,
"block": pprof.Handler("block").ServeHTTP,
"trace": pprof.Trace,
"mutex": pprof.Handler("mutex").ServeHTTP,
}

for _, profile := range config.Profiles {
if handler, exists := profileHandlers[profile]; exists {
if profile == "mutex" {
runtime.SetMutexProfileFraction(2)
}
mux.HandleFunc("/debug/pprof/"+profile, handler)
}
}

pprofSrv := &http.Server{
Addr: config.Addr,
Handler: mux,
ReadHeaderTimeout: 5 * time.Second,
}

go func() {
<-ctx.Done()
pprofSrv.Close()
}()

return pprofSrv.ListenAndServe()
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have some logging when we start the server, including what port we are binding to: https://github.com/heroku/x/blob/master/cmdutil/debug/debug.go#L48-L52

})
}
Loading