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

meshapi: follow best practice for metric names #722

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Conversation

katexochen
Copy link
Member

Counters should have a _total suffix, so we are renaming
contrast_meshapi_attestation_failures -> contrast_meshapi_attestation_failures_total.

This will also be enforced by #721 in the future.

@katexochen katexochen added the breaking change A user-affecting breaking change label Jul 12, 2024
@katexochen katexochen requested a review from Freax13 July 12, 2024 11:36
Copy link

github-actions bot commented Jul 12, 2024

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-07-12 13:17 UTC

Counters should have a _total suffix.

Signed-off-by: Paul Meyer <[email protected]>
Name: "attestation_failures",
Name: "attestation_failures_total",
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered creating a constant containing this value? We could reuse the constant in the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Might be a good idea if we want to use this in more places in the future, or must reference more label names elsewhere. But I think in general building up tests on metrics as insight isn't that good of a design.

@katexochen katexochen merged commit 40cd503 into main Jul 12, 2024
10 checks passed
@katexochen katexochen deleted the p/prom-best-prac branch July 12, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A user-affecting breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants