Skip to content

use timedelta for any durations #1488

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Mar 29, 2022
Merged

use timedelta for any durations #1488

merged 13 commits into from
Mar 29, 2022

Conversation

beniwohli
Copy link
Contributor

@beniwohli beniwohli commented Mar 15, 2022 •
edited
Loading

timedelta only allows comparisons, additions and substractions
with other objects of type timedelta, not with unit-less floats or ints.

This should ensure that we only ever compare durations of the same
unit, not e.g. milliseconds to seconds.
@beniwohli
use an explicit Seconds type for any durations
ab29f90 
The Seconds type only allows comparisons, additions and substractions with other objects of type Seconds, not with unit-less floats or ints. This should ensure that we only ever compare durations of the same type, not e.g. milliseconds to seconds.
@ghost
Copy link

ghost commented Mar 15, 2022 •
edited by ghost
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-03-29T16:31:13.278+0000

  • Duration: 21 min 12 sec

Test stats 🧪

Test Results
Failed 0
Passed 4768
Skipped 3209
Total 7977

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /test linters : Run the Python linters only.

  • /test full : Run the full matrix of tests.

  • /test benchmark : Run the APM Agent Python benchmarks tests.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)
basepi
basepi reviewed Mar 15, 2022
View reviewed changes
Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

Looking good so far. I'll do an in-depth review for accuracy once tests are passing. But overall I think it's a good direction. We definitely need to standardize so this doesn't bite us again.
@beniwohli
Copy link
Contributor Author

/test full
@beniwohli
use timedelta instead of custom Seconds object
989d8ef 
also, introduce a _DurationConfigValue that handles everything
@beniwohli beniwohli changed the title use an explicit Seconds type for any durations use timedelta for any durations Mar 21, 2022
@beniwohli
Copy link
Contributor Author

/test full
basepi
basepi reviewed Mar 22, 2022
View reviewed changes
Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

Looks solid! Let me know if I can help with the integration test failures.
@beniwohli
don't use timedelta in metrics
6e9035e 
it's impossible to know which unit the sum value in Prometheus summaries has, so we can't properly convert it to a timedelta.
@basepi
Copy link
Contributor

basepi commented Mar 24, 2022

/test full
@basepi basepi mentioned this pull request Mar 24, 2022
Merged
@beniwohli beniwohli marked this pull request as ready for review March 28, 2022 07:15
@beniwohli beniwohli requested a review from basepi March 29, 2022 11:32
basepi
basepi approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

So much goodness here. Quite a few places where I noted "oh, that's much clearer" while reviewing. 👍
@basepi basepi merged commit 8ad296a into elastic:main Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-python
2 participants
@beniwohli @basepi