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

Update validator reporting logs and metrics for Altair #9589

Merged
merged 41 commits into from
Sep 29, 2021
Merged

Update validator reporting logs and metrics for Altair #9589

merged 41 commits into from
Sep 29, 2021

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Sep 14, 2021 •
edited
Loading

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Inclusion slot and delay are key metrics that users read. This PR is an attempt to replace those metrics with something equally as meaningful for Altair.

Which issues(s) does this PR fix?

Fixes #9545

Other notes for review

@alrevuelta
Copy link

Perhaps add a note in IndividualVotesRespond stating that inclusion_slot and inclusion_distance are not usable after Altair fork?

@terencechain terencechain added this to the v2.0.0 release milestone Sep 27, 2021
@prestonvanloon prestonvanloon marked this pull request as ready for review September 27, 2021 16:47
@prestonvanloon prestonvanloon requested a review from a team as a code owner September 27, 2021 16:47
@prestonvanloon prestonvanloon requested review from kasey, jmozah and rauljordan and removed request for a team September 27, 2021 16:47
terencechain
terencechain previously approved these changes Sep 27, 2021
View reviewed changes
potuz
potuz reviewed Sep 27, 2021
View reviewed changes
validator/client/metrics.go
@@ -258,23 +269,29 @@ func (v *validator) LogValidatorGainsAndLosses(ctx context.Context, slot types.S
startBalance := float64(v.startBalances[pubKeyBytes]) / gweiPerEth
percentNet := (newBalance - prevBalance) / prevBalance
percentSinceStart := (newBalance - startBalance) / startBalance
log.WithFields(logrus.Fields{

previousEpochSummaryFields := logrus.Fields{
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding here Altair's Inactivity scores

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I totally meant to do that!

@potuz
Copy link
Contributor

potuz commented Sep 27, 2021

Another general comment is that perhaps you want to update the description of the correctXXX flags both for logs and metrics. The thing is that after Altair a user might honestly not know if a YES say for correctlyVotedSource means that he voted the source or that he got the timely flag for source. I suggest we include the individual bits separately as it would make debugging easier and since this is the only thing that a user would want to see: namely if you got the source right, and within 5 slots, then report yes, and if you got the source right, and got included at 6 slots, then report No.

@potuz potuz mentioned this pull request Sep 27, 2021
Closed
@prestonvanloon
Copy link
Member Author

This is failing e2e consistently. I'll look into that today.

@prestonvanloon prestonvanloon marked this pull request as draft September 29, 2021 15:04
@prestonvanloon
Copy link
Member Author 

panic: runtime error: index out of range [0] with length 0

goroutine 6554 [running]:
github.com/prysmaticlabs/prysm/validator/client.(*validator).LogValidatorGainsAndLosses(0xc000223040, 0x13f2800, 0xc002381e00, 0x29, 0x7ffe98769e57, 0x3)
        validator/client/metrics.go:267 +0x1ba5
github.com/prysmaticlabs/prysm/validator/client.run.func2(0xc002014930, 0x1406258, 0xc000223040, 0xc00228f460, 0x29, 0xc001dfd960, 0xc00228f490)
        validator/client/runner.go:217 +0x9f
created by github.com/prysmaticlabs/prysm/validator/client.run
        validator/client/runner.go:213 +0x1cd9

@prestonvanloon prestonvanloon marked this pull request as ready for review September 29, 2021 20:25
@prestonvanloon
Copy link
Member Author

Ready for another look. I discovered a bug in the previous release that would lead to a panic if the server returned a response where the slices were not the same length, i.e. when Altair fork occurs and some fields are no longer returned.

terencechain
terencechain reviewed Sep 29, 2021
View reviewed changes
validator/client/metrics.go
@@ -408,6 +457,15 @@ func (v *validator) UpdateLogAggregateStats(resp *ethpb.ValidatorPerformanceResp
totalPrevBal += v.prevBalance[i]
}

if totalStartBal == 0 ||
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@prylabs-bulldozer prylabs-bulldozer bot merged commit 520bc9d into develop Sep 29, 2021
@delete-merged-branch delete-merged-branch bot deleted the deprecate-fields-for-altair branch September 29, 2021 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@potuz potuz potuz left review comments

@terencechain terencechain terencechain approved these changes

@kasey kasey Awaiting requested review from kasey

@jmozah jmozah Awaiting requested review from jmozah

@rauljordan rauljordan Awaiting requested review from rauljordan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.0 release
Development

Successfully merging this pull request may close these issues.

GetValidatorPerformance returns averageInclusionDistance of 0 in v2
4 participants
@prestonvanloon @alrevuelta @potuz @terencechain