-
Notifications
You must be signed in to change notification settings - Fork 105
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.
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
fix(tss): remove keygen/sign metrics to avoid metrics bloat #1317
Conversation
x/tss/abci.go
Outdated
// sign participation | ||
telemetry.SetGaugeWithLabels([]string{types.ModuleName, "sign", "participation"}, | ||
float32(len(k.GetSignParticipants(ctx, info.SigID))), []metrics.Label{telemetry.NewLabel("keyID", string(info.KeyID))}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this still going to bloat based on the key ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, but keygen participation was also using keyID as a label, yet apparently wasn't causing any major issue. So I decided to preserve keyID for sign participation, and remove keyID from keygen participation. Nonetheless, if we find that sign participation metrics still result in bloat, we can completely remove them in an upcoming patch without having a consensus-breaking change (since those metrics are published via the end blocker and do nto modify the state).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not remove it now? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it seems unanimous that these should be removed, I did just that
…r-core into abridged_tss_metrics
Description
Closes #1298
Todos
Steps to Test
Expected Behaviour
Other Notes