open-telemetry / opentelemetry-python

OpenTelemetry Python API and SDK

https://opentelemetry.io

Apache License 2.0

1.66k stars 568 forks source link

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

Open martin-schulze-e2m opened 2 weeks ago

martin-schulze-e2m commented 2 weeks ago

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

linux-foundation-easycla[bot] commented 2 weeks ago

:x: The email address for the commit (14e1bd873f28f11a556e887736c8ed5278db5de7) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.
:x: - login: @martin-schulze-e2m . The commit (f22e9221bb43ce331e2f1d3af4dc5cbe86dfd3b8) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

xrmx commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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

martin-schulze-e2m commented 1 week ago

@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 commented 1 week ago

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