Fix incorrect time measurement by using monotonic time #70
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`Statsd#time` uses the `Time` class to measure the difference between elapsed time. This is incorrect because `Time.now` uses realtime, which is meant to be used for system clock. `clock_gettime` from `time.h` supports *monotonic time*. Let me quote the man page of `clock_gettime` man to demonstrate the difference between them: > CLOCK_REALTIME > the system's real time (i.e. wall time) clock, expressed as the > amount of time since the Epoch. > CLOCK_MONOTONIC > clock that increments monotonically, tracking the time since an > arbitrary point, and will continue to increment while the system is > asleep. Since we measure elapsed time, we should be using monotonic time, since it more accurate and meant to be used for that reason.
|
The errors in the build seem to be unrelated to the changes here (the specs pass locally for me). |
|
Thank you! |
bilbof
pushed a commit
to alphagov/publishing-api
that referenced
this pull request
Jan 7, 2021
This test mocks the internals of Statsd (time sending a message to timing) Statsd no longer uses Time.now to determine the time, instead using Process.clock_gettime in order to compute monotonic time. More details: reinh/statsd#70. We therefore must update our test to mock the new call and also stub the constant Process::CLOCK_MONOTONIC. This is the same dependency on some Statsd internal behaviour, it just _looks_ more brittle. Timecop may support this in future: travisjeffery/timecop#220 (comment)
bilbof
pushed a commit
to alphagov/publishing-api
that referenced
this pull request
Jan 7, 2021
This test mocks the internals of Statsd (time sending a message to timing) Statsd no longer uses Time.now to determine the time, instead using Process.clock_gettime in order to compute monotonic time. More details: reinh/statsd#70. We therefore must update our test to mock the new call and also stub the constant Process::CLOCK_MONOTONIC. This is the same dependency on some Statsd internal behaviour, it just _looks_ more brittle. Timecop may support this in future: travisjeffery/timecop#220 (comment)
bilbof
pushed a commit
to alphagov/publishing-api
that referenced
this pull request
Jan 7, 2021
This test mocks the internals of Statsd (time sending a message to timing) Statsd no longer uses Time.now to determine the time, instead using Process.clock_gettime in order to compute monotonic time. More details: reinh/statsd#70. We therefore must update our test to mock the new call and also stub the constant Process::CLOCK_MONOTONIC. This is the same dependency on some Statsd internal behaviour, it just _looks_ more brittle. Timecop may support this in future: travisjeffery/timecop#220 (comment)
bilbof
pushed a commit
to alphagov/publishing-api
that referenced
this pull request
Jan 7, 2021
This test mocks the internals of Statsd (time sending a message to timing) Statsd no longer uses Time.now to determine the time, instead using Process.clock_gettime in order to compute monotonic time. More details: reinh/statsd#70. I initially updated our test to mock the new call and also stub the constant Process::CLOCK_MONOTONIC. But, this is the same dependency on some Statsd internal behaviour, it just _looked_ more brittle. Timecop may support this in future: travisjeffery/timecop#220 (comment) Instead, sleeping for 1 second doesn't also test rely on testing the internals of the Statsd gem.
fredericfran-gds
pushed a commit
to alphagov/publishing-api
that referenced
this pull request
Apr 29, 2021
This test mocks the internals of Statsd (time sending a message to timing) Statsd no longer uses Time.now to determine the time, instead using Process.clock_gettime in order to compute monotonic time. More details: reinh/statsd#70. We therefore must update our test to mock the new call and also stub the constant Process::CLOCK_MONOTONIC. This is the same dependency on some Statsd internal behaviour, it just _looks_ more brittle. Timecop may support this in future: travisjeffery/timecop#220 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Statsd#timeuses theTimeclass to measure the difference betweenelapsed time. This is incorrect because
Time.nowuses realtime,which is meant to be used for system clock.
clock_gettimefromtime.hsupports monotonic time. Let me quotethe man page of
clock_gettimeman to demonstrate the differencebetween them:
Since we measure elapsed time, we should be using monotonic time,
since it more accurate and meant to be used for that reason.