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

Fix: Don't pass ns for ms in timeout during metrics collection #3963

Closed

Conversation

martin-schulze-e2m
Copy link

Description

When nearing the collection deadline, the remaining time should be accurately passed is timeout to the callbacks.
Once the deadline is closer than 10s, the remaining time in nanoseconds is passed to timeout_millis, which is expected to be in milliseconds, thus giving a timeout that is 10e6 times too large.

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

CI?

Does This PR Require a Contrib Repo Change?

No. (As far as I can tell.)

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@martin-schulze-e2m martin-schulze-e2m requested a review from a team June 10, 2024 10:55
Copy link

linux-foundation-easycla bot commented Jun 10, 2024

CLA Missing ID CLA Not Signed

@xrmx
Copy link
Contributor

xrmx commented Jun 10, 2024

Good catch, care to sign the CLA and add a test? asserting that a mocked CallbackOptions that is called with the right value should be enough.

@martin-schulze-e2m
Copy link
Author

Good catch, care to sign the CLA and add a test? asserting that a mocked CallbackOptions that is called with the right value should be enough.

I contacted our legal department regarding the CLA but cannot give an ETA when this will be resolved. After that I will have a look at the tests.

@martin-schulze-e2m
Copy link
Author

I updated an existing test that had a wrong test expectation.

We are still working on getting the CLA signed but legal approved already.

I won't be available in the next few weeks so if more changes are required, you might need to be patient.

@xrmx
Copy link
Contributor

xrmx commented Jun 20, 2024

@martin-schulze-e2m please use only the account that will have the CLA signed for every commit

@martin-schulze-e2m
Copy link
Author

martin-schulze-e2m commented Jun 21, 2024

@xrmx I started this PR via the web ui and edited the second commit locally. I don't know how to consolidate that to only the "web account".

I tried to rebase and sign both with my GPG Key. Is that enough for EasyCLA?

@xrmx
Copy link
Contributor

xrmx commented Jun 21, 2024

@martin-schulze-e2m We'll sort it out once we have a green tick from the CLA check

@martin-schulze-e2m
Copy link
Author

I just wanted to let you know that this will take a while longer. Our designated CLA manager was not able to get the required signature before leaving until September.

@xrmx
Copy link
Contributor

xrmx commented Jul 23, 2024

@martin-schulze-e2m would it be a problem for you if we will do a re-implementation since this is open since a few already?

@martin-schulze-e2m
Copy link
Author

I just wanted to get this resolved and thought this was the fastest route. It turns out it was not.

Would it be a problem for you if we will do a re-implementation

I cannot speak for my employer, I am fine with that and would have had no problem with contributing this privately...

I hope we will eventually get the CLA sorted out to have a more streamlined experience the next time. :)

@xrmx
Copy link
Contributor

xrmx commented Jul 23, 2024

@martin-schulze-e2m if you want to redo the contribution on your own time and personal account please do

@martin-schulze-e2m
Copy link
Author

martin-schulze-e2m commented Jul 23, 2024

I am severely time constrained right now and meant: if I had known in advance that this would be an issue, I would have contributed under a different account.

Anyways I doubt that I can legally do a "clean room" design that will shield your org. I think a rewrite must come from someone else now.

I am sorry about the hassle.

@xrmx
Copy link
Contributor

xrmx commented Jul 23, 2024

@martin-schulze-e2m don't worry

@xrmx
Copy link
Contributor

xrmx commented Jul 24, 2024

Superseded by #4074, thanks for spotting the issue and the initial attempt!

@xrmx xrmx closed this Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants