-
Notifications
You must be signed in to change notification settings - Fork 1.2k
✨ Define metrics.Registry as an interface #713
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
✨ Define metrics.Registry as an interface #713
Conversation
Welcome @xrstf! |
Hi @xrstf. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
/lgtm
/ok-to-test
/assign @DirectXMan12
5d47d26
to
9b6f3d1
Compare
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.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DirectXMan12, xrstf The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest o_O looks like a flake? (asked the author of the PR that added that test to take a look) |
Kustomize and Snapshot Go Links
It's currently not possible to replace the predefined Registry with a custom implementation.
I need this because I re-create controllers with the same name at runtime and this causes the default registry to rightfully complain that the metrics for this label combination are already registered. When calling
controller.New()
I have no influence over the metrics creation and errors are always logged with a logger I can also not control.With this PR I can register my own Registry implementation that silently drops the AlreadyRegistered errors.
We could have used a custom subset of the two predefined Registerer/Gatherer interfaces, but both are so short and only contain what we actually need, so it seemed not worth the effort to replicate the method definitions.
I can see that there are some long-standing intentions to migrate to both non-global metrics definitions and the use of OpenTelemetry, but in the meantime this would make life easier [for me]. :-)