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.

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

atls: only report attestation error if all validators fail #813

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

Freax13
Copy link
Contributor

@Freax13 Freax13 commented Aug 16, 2024

Previously we reported every single validator error even if there's one validator that passes. This lead to noisy logs and incorrect metrics.

@Freax13 Freax13 added the no changelog PRs not listed in the release notes label Aug 16, 2024
@Freax13 Freax13 requested a review from msanft August 16, 2024 12:46
@Freax13 Freax13 requested a review from katexochen as a code owner August 16, 2024 12:46
Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

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

I'm not sure if we really want to pass down a prometheus counter to the attestation functions directly, instead of adding some more generic mechanism to inject code into the validation process (like the callbacks)

As you can see in your change, this results in the CLI side not utilizing the counters at all, having a unused parameter effectively. If we want to pass the counter directly, we should probably have some kind of builder pattern here that allows for adding the counter subsequently, and only using if it it's present.

cli/cmd/recover.go Show resolved Hide resolved
Previously we reported every single validator error even if there's one
validator that passes. This lead to noisy logs and incorrect metrics.
@Freax13 Freax13 force-pushed the tom/validator-graceful-fail branch from 80ebf3f to 7272598 Compare August 16, 2024 13:29
@Freax13 Freax13 requested a review from msanft August 19, 2024 06:51
Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

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

Would prefer a dynamic integration of the counter into the aTLS code, but for now, this is fine to me.

@Freax13 Freax13 merged commit 3568fe4 into main Aug 19, 2024
10 checks passed
@Freax13 Freax13 deleted the tom/validator-graceful-fail branch August 19, 2024 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants