Update metrics for managed keys #5354
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## feat/multikey_metrics #5354 +/- ##
=========================================================
- Coverage 80.31% 80.29% -0.02%
=========================================================
Files 697 697
Lines 90355 90222 -133
=========================================================
- Hits 72570 72447 -123
+ Misses 12608 12607 -1
+ Partials 5177 5168 -9
☔ View full report in Codecov by Sentry. |
process/block/metrics.go
Outdated
| isInConsensus := false | ||
| for index, publicKey := range pubKeys { | ||
| if index == 0 { | ||
| continue |
There was a problem hiding this comment.
Why do we skip the index 0?
There was a problem hiding this comment.
that's the leader index. I guess we can have a const like leaderIndex := 0 and use that in the if branch
There was a problem hiding this comment.
added constant
process/block/metrics.go
Outdated
| if bytes.Equal([]byte(publicKey), ownPublicKey) { | ||
| if index == 0 { | ||
| return | ||
| } | ||
|
|
||
| isInConsensus = true | ||
| break | ||
| appStatusHandler.Increment(common.MetricCountConsensusAcceptedBlocks) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| if !isInConsensus { | ||
| return | ||
| if managedPeersHolder.IsKeyManagedByCurrentNode([]byte(publicKey)) { | ||
| appStatusHandler.Increment(common.MetricCountConsensusAcceptedBlocks) | ||
| } |
There was a problem hiding this comment.
Can be rewritten as something like:
isMyKey := bytes.Equal([]byte(publicKey), ownPublicKey)
isManagedByCurrentNode := managedPeersHolder.IsKeyManagedByCurrentNode([]byte(publicKey))
if isMyKey || isManagedByCurrentNode {
appStatusHandler.Increment(common.MetricCountConsensusAcceptedBlocks)
}
There was a problem hiding this comment.
good suggestion, applied
| @@ -159,10 +159,12 @@ func (sr *subroundStartRound) initCurrentRound() bool { | |||
| msg = " (my turn in multi-key)" | |||
| } | |||
| if leader == sr.SelfPubKey() { | |||
| msg = " (my turn)" | |||
| } | |||
| if len(msg) != 0 { | |||
| @@ -181,10 +183,10 @@ func (sr *subroundStartRound) initCurrentRound() bool { | |||
| } | |||
| sr.AppStatusHandler().SetStringValue(common.MetricConsensusState, "not in consensus group") | |||
| } else { | |||
| if leader != sr.SelfPubKey() { | |||
| if leader != sr.SelfPubKey() && !sr.IsKeyManagedByCurrentNode([]byte(leader)) { | |||
process/block/metrics.go
Outdated
| isInConsensus := false | ||
| for index, publicKey := range pubKeys { | ||
| if index == 0 { | ||
| continue |
There was a problem hiding this comment.
that's the leader index. I guess we can have a const like leaderIndex := 0 and use that in the if branch
process/block/metrics.go
Outdated
| header.PrevRandSeed, | ||
| header.Round, | ||
| header.GetPrevRandSeed(), | ||
| header.GetRound(), | ||
| core.MetachainShardId, |
There was a problem hiding this comment.
is this ok? Shouldn't have been header.ShardID() ?
There was a problem hiding this comment.
right, messed it up while merging the methods
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?