add pprof config and start up logic #478
Conversation
|
|
||
| type ( | ||
| // PProfInitializer initialize the pprof based on config | ||
| PProfInitializer interface { |
There was a problem hiding this comment.
Do we need an interface for this? We will likely only ever have one implementation. We can just use the struct directly.
There was a problem hiding this comment.
i am keeping the interface since every else is using a interface / impl pattern.
and i agree that the interface is redundant......
common/service/config/pprof.go
Outdated
| } | ||
|
|
||
| // Initialize the pprof based on config | ||
| func (initializer *PProfInitializerImpl) Initialize() error { |
There was a problem hiding this comment.
nit: We often use "Start" for this and have that method on a lot of our structures. I would recommend naming this Start as well to keep it consistent. Will leave it to you to decide.
| ) | ||
|
|
||
| // NewInitializer create a new instance of PProf Initializer | ||
| func (cfg *PProf) NewInitializer(logger bark.Logger) *PProfInitializerImpl { |
There was a problem hiding this comment.
I would rather have a method NewInitializer(cfg *PProf, logger bark.Logger). That's how we do it everywhere and not much reason for this to be a member of PProf.
There was a problem hiding this comment.
I see why you did this now w.r.t other members in the config. The reason this one seems weird is PProf is a config struct. We use config to create a NewInitializer, seems to fit more as a parameter using which a new initializer is created.
There was a problem hiding this comment.
in common/service, rpc.go and ringpop.go are using the same pattern.....
seems i should follow the same pattern....
host/onebox.go
Outdated
| ports = append(ports, port) | ||
| } | ||
|
|
||
| c.logger.Infof("History pprof prots: %v", ports) |
There was a problem hiding this comment.
Do we want to log this? Other ones aren't being logged.
There was a problem hiding this comment.
the server, when starts, actually shows the address and port:
INFO[2017-12-21T15:23:09-08:00] Created RPC dispatcher for 'cadence-matching' and listening at '127.0.0.1:7935'
INFO[2017-12-21T15:23:09-08:00] Created RPC dispatcher for 'cadence-frontend' and listening at '127.0.0.1:7933'
INFO[2017-12-21T15:23:09-08:00] Created RPC dispatcher for 'cadence-history' and listening at '127.0.0.1:7934'
common/service/service.go
Outdated
| @@ -116,6 +119,10 @@ func (h *serviceImpl) Start() { | |||
| h.metricsScope.Counter(metrics.RestartCount).Inc(1) | |||
| h.runtimeMetricsReporter.Start() | |||
|
|
|||
| if err := h.pprofInitializer.Initialize(); err != nil { | |||
There was a problem hiding this comment.
You probably want to allow service startup if the pprof initializer is not set.
No description provided.