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

Fix the dkg_state and reshare_state metrics #966

Merged
merged 7 commits into from
Apr 21, 2022
Merged

Fix the dkg_state and reshare_state metrics #966

merged 7 commits into from
Apr 21, 2022

Conversation

mcamou
Copy link
Contributor

@mcamou mcamou commented Apr 20, 2022

No description provided.

@mcamou mcamou self-assigned this Apr 20, 2022
@mcamou mcamou added the monitoring Area: Monitoring label Apr 20, 2022
@mcamou mcamou added this to the Ceremony dashboard milestone Apr 20, 2022
willscott
willscott reviewed Apr 20, 2022
View reviewed changes
Copy link
Member

@willscott willscott left a comment

Choose a reason for hiding this comment

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

These look like fine changes.

Are we reasonably convinced that we are appropriately reporting metrics in all the places we should be - like when a leader starts a dkg - and it was just the aggregation in the dashboard that was making the transitions not always appear as expected?

@mcamou
Copy link
Contributor Author

mcamou commented Apr 20, 2022 •
edited
Loading

These look like fine changes.

Are we reasonably convinced that we are appropriately reporting metrics in all the places we should be - like when a leader starts a dkg - and it was just the aggregation in the dashboard that was making the transitions not always appear as expected?

I am not sure of anything ;)

Looking for usages of metrics.DKGStateChange and metrics.ReshareStateChange will show all the places where the metrics are emitted, I'd really like someone who is actually familiar with the logic flow to do a sanity check. Any takers?

@mcamou
Copy link
Contributor Author

mcamou commented Apr 20, 2022

@willscott @AnomalRoil After some testing I realized that I am missing some state changes. I'll push some changes tomorrow to fix that. Meanwhile I'll mark this PR as draft so that it doesn't get accidentally merged.

@mcamou mcamou marked this pull request as draft April 20, 2022 19:08
@mcamou mcamou marked this pull request as ready for review April 21, 2022 10:53
AnomalRoil
AnomalRoil requested changes Apr 21, 2022
View reviewed changes
core/drand_beacon.go Outdated Show resolved Hide resolved
core/drand_beacon_control.go
@@ -698,7 +707,7 @@ func (bp *BeaconProcess) setupAutomaticResharing(_ context.Context, oldGroup *ke
Metadata: &metadata,
}

metrics.ReshareStateChange(metrics.ReshareWaiting, beaconID, false)
metrics.ReshareStateChange(metrics.ReshareWaiting, beaconID, in.GetInfo().GetLeader())
Copy link
Member

Choose a reason for hiding this comment

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

I think the leader never calls the setupAutomaticResharing function. But yeah, it cannot hurt to have it be dynamically tested.

metrics/metrics.go Show resolved Hide resolved
metrics/metrics.go Show resolved Hide resolved
@AnomalRoil AnomalRoil merged commit 814b9e6 into master Apr 21, 2022
@AnomalRoil AnomalRoil deleted the fix_state_metrics branch April 21, 2022 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willscott willscott willscott approved these changes

@AnomalRoil AnomalRoil AnomalRoil approved these changes

Assignees

@mcamou mcamou

Labels
monitoring Area: Monitoring
Projects
None yet
Milestone
Ceremony dashboard
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mcamou @willscott @AnomalRoil