-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
cmdutil/server.go
Outdated
config.Addr = "127.0.0.1:9998" // Default port | ||
} | ||
|
||
return NewContextServer(func(ctx context.Context) error { |
There was a problem hiding this comment.
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
cmdutil/server.go
Outdated
@@ -93,3 +97,80 @@ func MultiServer(servers ...Server) Server { | |||
StopFunc: s.Stop, | |||
} | |||
} | |||
|
|||
// NewPprofServer sets up a pprof server with optional mutex profiling. |
There was a problem hiding this comment.
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?
cmdutil/server.go
Outdated
@@ -4,6 +4,10 @@ package cmdutil | |||
|
|||
import ( | |||
"context" | |||
"net/http" |
There was a problem hiding this comment.
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
cmdutil/server.go
Outdated
mux := http.NewServeMux() | ||
|
||
profileHandlers := map[string]http.HandlerFunc{ | ||
"index": pprof.Index, |
There was a problem hiding this comment.
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.
cmdutil/server.go
Outdated
pprofSrv.Close() | ||
}() | ||
|
||
return pprofSrv.ListenAndServe() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd also like to see a unit test of this
cmdutil/debug/debug.go
Outdated
logger logrus.FieldLogger | ||
addr string | ||
done chan struct{} | ||
pprofServer *http.Server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see this as a separate server. So something like debu.PProfServer
.
cmdutil/debug/debug.go
Outdated
} | ||
|
||
l.WithFields(logrus.Fields{ | ||
"at": "binding", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Binding log should happen when Run()
is called. The server does not actually start until Run()
cmdutil/debug/debug.go
Outdated
for profile, handler := range config.ProfileHandlers { | ||
if handler != nil { | ||
if profile == "mutex" { | ||
runtime.SetMutexProfileFraction(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be configurable on the profile config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean something like this:
type PProfServerConfig struct {
MutextProfileFraction int
}
If set to zero and the mutex profile is enabled, we can use a reasonable default of 2.
cmdutil/debug/debug.go
Outdated
"service": "pprof", | ||
"profile": profile, | ||
}).Info("Added pprof profile handler") | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this else. If someone has passed in a nil-value handler, that is poor configuration we should not support. A nil-pointer exception and subsequent panic are acceptable
cmdutil/debug/debug.go
Outdated
"at": "adding", | ||
"service": "pprof", | ||
"profile": profile, | ||
}).Info("Added pprof profile handler") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think we need to log this. If we want, we can do a simple strings.Join()
on the profiles slice and then log profiles="heap,cpu,blah..."
when we log the port binding in Run()
.
cmdutil/debug/debug.go
Outdated
} | ||
|
||
// ProfileConfig holds the configuration for the pprof server. | ||
type ProfileConfig struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is used to configure a PProfServer
, let's call it PProfServerConfig
.
cmdutil/debug/debug.go
Outdated
for profile, handler := range config.ProfileHandlers { | ||
if handler != nil { | ||
if profile == "mutex" { | ||
runtime.SetMutexProfileFraction(2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean something like this:
type PProfServerConfig struct {
MutextProfileFraction int
}
If set to zero and the mutex profile is enabled, we can use a reasonable default of 2.
cmdutil/debug/debug.go
Outdated
if s.pprofServer != nil { | ||
go func() { | ||
if err := s.pprofServer.ListenAndServe(); err != nil && err != http.ErrServerClosed { | ||
s.logger.WithError(err).Error("pprof server error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want the error to be returned by Run()
cmdutil/debug/debug.go
Outdated
if profile == "mutex" { | ||
runtime.SetMutexProfileFraction(2) | ||
} | ||
mux.HandleFunc("/debug/pprof/"+profile, handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to raw construction of the handlers like this. The pprof.Handler
function can construct the handlers for us: https://pkg.go.dev/net/http/pprof#Handler
cmdutil/debug/debug.go
Outdated
} | ||
runtime.SetMutexProfileFraction(config.MutexProfileFraction) | ||
} | ||
mux.Handle("/debug/pprof/"+profile, pprof.Handler(profile)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I am thinking that we should just use pprof.Index
and generate a handler like so:
http.HandlerFunc(pprof.Index)
Then you can avoid using mux and having to pass in the list of profiles.
cmdutil/debug/debug.go
Outdated
for _, profile := range config.ProfileNames { | ||
if profile == "mutex" { | ||
if config.MutexProfileFraction == 0 { | ||
config.MutexProfileFraction = 2 // Use default value of 2 if not set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not modify the config. That is unexpected behavior. Instead, we can use a local variable:
mpf := defaultMutextProfileFraction // value is 2
if config.MutexProfileFraction != 0 {
mpf = config.MutextProfileFraction
}
runtime.SetMutextProfileFraction(mpf)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor things but this looks good!
cmdutil/debug/debug.go
Outdated
"addr": s.addr, | ||
}).Info() | ||
|
||
if s.pprofServer != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is nil, I think we can return an error.
cmdutil/debug/debug_test.go
Outdated
} | ||
runtime.SetMutexProfileFraction(tt.expectedMutexFraction) // Reset to the expected value | ||
|
||
urls := []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think verifying the paths is probably overkill since that is functionality within pprof
package.
Description
We want to configure the pprof listener that is capable of running mutex profiles on server set-up in
cmdutil/debug
. This PR created a newNewPprofServer
method to thecmdutil/debug
package, allowing for the setup of approf
server with configurable profiling types and optional mutex profiling.Changes
PProfServerConfig
struct: holds configuration for the pprof server.PProfServer
struct: wraps a pprof serverNewPProfServer
function: creates and configures the pprof server.Run
andStop
methods: theNewPProfServer
function includesRun
andStop
methods for managing the pprof server lifecycle.NewPProfServer
Metadata
W-16252388