made epoch start metrics persistent and/or fetch from epoch bootstrapper #1872
Conversation
| @@ -863,6 +863,7 @@ func startNode(ctx *cli.Context, log logger.Logger, version string) error { | |||
| Rounder: rounder, | |||
| AddressPubkeyConverter: addressPubkeyConverter, | |||
| LatestStorageDataProvider: latestStorageDataProvider, | |||
| StatusHandler: coreComponents.StatusHandler, | |||
There was a problem hiding this comment.
please check if this is the correct instance. We have an issue regarding coreComponents.StatusHandler. It is instantiated twice (one mock, and then the "real" one). ⚔️
There was a problem hiding this comment.
it is ok. on main.go, line 738 we see a :
coreComponents.StatusHandler = statusHandlersInfo.StatusHandler
and the bootstrapper args are written on line 839
There was a problem hiding this comment.
the nil implementation of status handler is used when returning the Core Components but the value is filled after status handlers are created
| @@ -88,6 +89,9 @@ func checkArguments(args ArgsEpochStartBootstrap) error { | |||
| if args.GeneralConfig.EpochStartConfig.MinNumConnectedPeersToStart < minNumConnectedPeers { | |||
| return fmt.Errorf("%s: %w", baseErrorMessage, epochStart.ErrNotEnoughNumConnectedPeers) | |||
| } | |||
| if check.IfNil(args.StatusHandler) { | |||
There was a problem hiding this comment.
this will hide errors, since we injected the status handler on the constructor. Suggestion to return error here and fix all integration tests.
There was a problem hiding this comment.
also added nil checks when using the status handler inside bootstrapper, as it is only an additional component.
epochStart/bootstrap/process.go
Outdated
| @@ -878,6 +881,11 @@ func (e *epochStartBootstrap) createRequestHandler() error { | |||
| return err | |||
| } | |||
|
|
|||
| func (e *epochStartBootstrap) setEpochStartMetrics() { | |||
| e.statusHandler.SetUInt64Value(core.MetricNonceAtEpochStart, e.epochStartMeta.Nonce) | |||
There was a problem hiding this comment.
shouldn't we also rewrite the metrics for erd_nonces_passed_in_current_epoch and erd_rounds_passed_in_current_epoch ?
There was a problem hiding this comment.
No, those are calculated when calling the API endpoint. So it is ok
iulianpascalau
dismissed
SebastianMarian’s stale review
The base branch was changed.
epochStart/bootstrap/process.go
Outdated
| @@ -891,8 +892,10 @@ func (e *epochStartBootstrap) createRequestHandler() error { | |||
| } | |||
|
|
|||
| func (e *epochStartBootstrap) setEpochStartMetrics() { | |||
| e.statusHandler.SetUInt64Value(core.MetricNonceAtEpochStart, e.epochStartMeta.Nonce) | |||
| e.statusHandler.SetUInt64Value(core.MetricRoundAtEpochStart, e.epochStartMeta.Round) | |||
| if !check.IfNil(e.statusHandler) && e.epochStartMeta != nil { | |||
There was a problem hiding this comment.
do we still need the check !check.IfNil(e.statusHandler) ?
iulianpascalau
dismissed stale reviews from SebastianMarian and themself
via
d938d58
bogdan-rosianu
dismissed stale reviews from SebastianMarian and iulianpascalau
via
cafcba8
Made metrics related to nonce and round at epoch start persistent