Run pprof (by default disabled) on a separate port than metrics #1758
Conversation
cla-bot
bot
added
the
s
cmd/nitro/nitro.go
Outdated
| @@ -122,6 +122,30 @@ func main() { | |||
| os.Exit(mainImpl()) | |||
| } | |||
|
|
|||
| // Runs metrics server at address:port specified by config if metrics flag is | |||
| // enabled. Additionally if pprof at specified pprof port when it's enabled. | |||
| func mustRunMetrics(cfg *NodeConfig) { | |||
There was a problem hiding this comment.
Why is this function called mustRunMetrics?
There was a problem hiding this comment.
MustXYZ is a convention in Go [0]. When helper stops the program (usually called from main) convention is to name it with mustXYZ.
[0] https://google.github.io/styleguide/go/decisions#must-functions
cmd/nitro/nitro.go
Outdated
| log.Error("--metrics must be enabled in order to use pprof with the metrics server") | ||
| return 1 | ||
| } | ||
| mustRunMetrics(nodeConfig) |
There was a problem hiding this comment.
After opening the blockchain I don't think we want to use log.Crit as it won't do a clean shutdown (it skips defers). One simple fix would be to move this call to before the openInitializeChainDb call.
There was a problem hiding this comment.
Good point, moved it up.
cmd/genericconf/server.go
Outdated
| UpdateInterval time.Duration `koanf:"update-interval"` | ||
| } | ||
|
|
||
| var MetricsServerConfigDefault = MetricsServerConfig{ | ||
| Addr: "127.0.0.1", | ||
| Port: 6070, | ||
| Pprof: false, | ||
| Pprof: true, | ||
| PprofPort: 6071, | ||
| UpdateInterval: 3 * time.Second, | ||
| } | ||
|
|
||
| func MetricsServerAddOptions(prefix string, f *flag.FlagSet) { | ||
| f.String(prefix+".addr", MetricsServerConfigDefault.Addr, "metrics server address") | ||
| f.Int(prefix+".port", MetricsServerConfigDefault.Port, "metrics server port") | ||
| f.Bool(prefix+".pprof", MetricsServerConfigDefault.Pprof, "enable profiling for Go") |
There was a problem hiding this comment.
I'm not sure which of these we should do, but we should either:
- Add a comment here explaining that pprof will only be exposed if metrics are enabled
- Make pprof independent of metrics, and have it default to disabled.
I think the latter approach is a bit better because metrics and pprof seem like separate features to me.
There was a problem hiding this comment.
Went with the second option.
joshuacolvin0
changed the title
Introduce a flag for pprof port (default 6071), and enable pprof by default.
Default handler for pprof will be: "http://127.0.0.1:6071/debug/pprof/" which exposes stacktrace as well at "http://127.0.0.1:6071/debug/pprof/goroutine".
Leaving pprof flag itself to have ability to disable it, at we may consider dropping that after a while.