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

Run pprof (by default disabled) on a separate port than metrics #1758

Merged
merged 7 commits into from
Jul 17, 2023
Merged

Run pprof (by default disabled) on a separate port than metrics #1758

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3eb0340
Run pprof on a separate port than metrics
anodar Jul 13, 2023
c001949
Separate PProf flag from metrics flag, disable by default
anodar Jul 13, 2023
741a9c8
Rename mustRunMetrics to startMetrics and return error instead
anodar Jul 14, 2023
77ad191
Merge branch 'master' into pprof-metrics
anodar Jul 14, 2023
38f9b32
Add comment explaining where pprof handlers are registered
anodar Jul 17, 2023
836c52d
Merge branch 'pprof-metrics' of github.com:OffchainLabs/nitro into pp…
anodar Jul 17, 2023
42d7842
Merge branch 'master' into pprof-metrics
anodar Jul 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cmd/genericconf/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,19 +190,22 @@ type MetricsServerConfig struct {
Addr string `koanf:"addr"`
Port int `koanf:"port"`
Pprof bool `koanf:"pprof"`
PprofPort int `koanf:"pprof-port"`
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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with the second option.

f.Int(prefix+".pprof-port", MetricsServerConfigDefault.PprofPort, "pprof server port")
f.Duration(prefix+".update-interval", MetricsServerConfigDefault.UpdateInterval, "metrics server update interval")
}
41 changes: 25 additions & 16 deletions cmd/nitro/nitro.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this function called mustRunMetrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if !cfg.Metrics {
if cfg.MetricsServer.Pprof {
log.Warn("Metrics must be enabled in order to use pprof with the metrics server")
}
log.Debug("Metrics are disabled")
return
}
if cfg.MetricsServer.Addr == "" {
log.Crit("Metrics are enabled but server address is not specified")
}
go metrics.CollectProcessMetrics(cfg.MetricsServer.UpdateInterval)

if cfg.MetricsServer.Pprof {
if cfg.MetricsServer.Port == cfg.MetricsServer.PprofPort {
log.Crit("Cannot use same port for metrics and pprof servers", "port", cfg.MetricsServer.Port)
}
genericconf.StartPprof(fmt.Sprintf("%v:%v", cfg.MetricsServer.Addr, cfg.MetricsServer.PprofPort))
}
exp.Setup(fmt.Sprintf("%v:%v", cfg.MetricsServer.Addr, cfg.MetricsServer.Port))
}

// Returns the exit code
func mainImpl() int {
ctx, cancelFunc := context.WithCancel(context.Background())
Expand Down Expand Up @@ -379,22 +403,7 @@ func mainImpl() int {
return 1
}

if nodeConfig.Metrics {
go metrics.CollectProcessMetrics(nodeConfig.MetricsServer.UpdateInterval)

if nodeConfig.MetricsServer.Addr != "" {
address := fmt.Sprintf("%v:%v", nodeConfig.MetricsServer.Addr, nodeConfig.MetricsServer.Port)
if nodeConfig.MetricsServer.Pprof {
genericconf.StartPprof(address)
} else {
exp.Setup(address)
}
}
} else if nodeConfig.MetricsServer.Pprof {
flag.Usage()
log.Error("--metrics must be enabled in order to use pprof with the metrics server")
return 1
}
mustRunMetrics(nodeConfig)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, moved it up.


fatalErrChan := make(chan error, 10)

Expand Down