-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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 flake on TestRecordOperation unit test #105809
Conversation
/assign @pacoxu @logicalhan |
9819c31
to
cf75599
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.
/priority important-longterm
/triage accepted
/lgtm
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
/assign @mrunalp
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aojea, pacoxu The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
legacyregistry.MustRegister(metrics.RuntimeOperationsErrors) | ||
}, | ||
) | ||
defer legacyregistry.Reset() |
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.
This looks problematic to me, it assumes that there are no concurrent tests using the legacyregistry.
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.
how can we deal with it then?
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.
/cc @logicalhan
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.
Let's first be explicit about whether there is a problem.
Are there test scenarios that have concurrent test func invocations in one process?
If so then the implication is that any given test func X can not assume that a registered metric is being affected only by the current invocation of X.
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.
These tests are executed one by one. They don't overlap with each other.
I printed begin time and end time for each test in
https://gist.github.com/CatherineF-dev/7557f0abdabe5e13ba1b37c7fa15523c
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.
The sample_and_watermark_test.go updated in #105886 could make and use a private registry in each test func invocation.
Same for the metrics_test.go in that same PR.
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.
I'm a bit busy these, anyone feels that can take over this PR?
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.
I can take it. Since #105886 is a similar issue.
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.
During testing, I found one advantage for using the global registry (legacyregistry). It could help detecting duplicated metrics name.
pkg1: metrics_a
pkg2: metrics_a
Both pkg1 and pkg2 emit metrics_a. It will be detected by the global registry, which is pretty useful.
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.
Duplicate metric names from registrations from non-test code will be detected by other unit, integration, and end-to-end tests.
/assign @CatherineF-dev |
@aojea: GitHub didn't allow me to assign the following users: CatherineF-dev. Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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. |
The below is solved.Still in trying replacing legacyregistry with local registry for mux.Handle("/metrics", legacyregistry.Handler()). I tried the following one, but failed
legacyregistry.Handler() is defined in registry.go |
Hi @aojea, I uploaded Not sure whether I have the permission to push to |
create your own PR , you are the one doing the effort :) |
@aojea: PR needs rebase. 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. |
duplicate |
/kind flake
Fixes #104940