-
Notifications
You must be signed in to change notification settings - Fork 97
Get Timestamp with process.hrtime and Date.now #261
Get Timestamp with process.hrtime and Date.now #261
Conversation
Codecov Report
@@ Coverage Diff @@
## master #261 +/- ##
=========================================
- Coverage 94.6% 94.6% -0.01%
=========================================
Files 105 103 -2
Lines 7397 7391 -6
Branches 691 691
=========================================
- Hits 6998 6992 -6
Misses 399 399
Continue to review full report at Codecov.
|
I think it would be nice to preserve high-resolution timestamps if we can. What if we measured |
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.
Just adding a review to agree with @draffensperger's comment above. Preserving high res timestamps would definitely be ideal and his approach should work.
Update: Also, this functionality seems very similar to the logic done inside the src/internal/clock.ts
class, which also utilizes the Date
for starting times and process.hrtime()
for high resolution duration. We should probably move the time logic in this PR to be colocated for simplicity.
@draffensperger and @justindsmith Thanks for the great suggestion. I managed to write function
|
@mayurkale22 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.
The function looks good. One minor suggestion would be to move the const hrtime = process.hrtime; const origin = hrtime();
code block right above const refTime = Date.now()
. That way there are no statements between when you measure hrtime
and when you measure now
so the accuracy could be very slightly better.
const nanos = (epochMilli: number): number => { | ||
const mos = epochMilli - seconds(epochMilli) * MILLIS_PER_SECOND; | ||
return Number(BigInt(mos).times(NANOS_PER_MILLI).toString()); | ||
}; |
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.
Optional nit: would it be cleaner to make these regular function
declarations rather than const
function expressions?
d5da3d6
to
4e58473
Compare
I have modified the code to use above mentioned logic, PTAL @draffensperger @justindsmith |
export function getTimestampWithProcessHRTime(): Timestamp { | ||
const [offsetSecs, offsetNanos] = hrtime(origin); // [seconds, nanoseconds] | ||
|
||
// determine drfit in seconds and nanoseconds |
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.
Nit: "drift" spelling
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.
Done.
const seconds = startSecs + offsetSecs; | ||
const nanos = startNanos + offsetNanos; | ||
|
||
// if nanos excess NANOS_PER_SECOND value. |
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.
Is this comment needed, it seems to express pretty closely what the if statement says?
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.
Make sense, removed.
const [points] = timeseries.points; | ||
assert.deepStrictEqual(timeseries.labelValues, LABEL_VALUES_200); | ||
assert.equal(points.value, 2); | ||
assert.equal(Math.floor(now / 1e3), points.timestamp.seconds); |
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.
What would you think about mocking Date.now
and process.hrtime
for the test (setting them and unsetting them before/after)? That way if someone sets a breakpoint in the test and some time elapses it will still pass and you can also assert exact values for nanoseconds.
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.
Initially I was thinking about it, but then realized Date.now
and process.hrtime
are initialized once in common/time-util.ts
. I think it is hard to mock them in before/after tests. I tried exporting const TEST_ONLY_ORIGIN_HRTIME = origin;
and const TEST_ONLY_REF_TIME = refTime;
but, couldn't manage to assert exact values for nanoseconds. Let me know if you have a better approach to mock these.
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.
What if you had something like this:
let hrtime = process.hrtime;
let hrtimeOrigin: [number, number] = [0, 0];
let hrtimeRefSeconds = 0;
let hrtimeRefNanos = 0;
/** This is used to enable tests to mock process.hrtime while still allow us to cache it. */
function resetHrtimeFunctionCache() {
hrtime = process.hrtime;
}
function setHrtimeReference() {
resetHrtimeFunctionCache();
hrtimeOrigin = hrtime();
const refTime = Date.now();
hrtimeRefSeconds = ...
hrtimeRefNanos = ...
}
setHrtimeReference();
export TEST_ONLY = {setHrtimeReference};
.....
// In the test
import {TEST_ONLY, ...} from '../common/time-utils.ts';
...
let realHrtimeFn = process.hrtime;
let realNowFn = Date.now;
beforeEach(() => {
process.hrtime = () => [55, 22];
Date.now = () => 14500000;
// Force the clock to recalibrate the time offset with the mocked time
TEST_ONLY.setHrtimeReference();
// Simulate that now some time passed ...
process.hrtime = () => [66, 33];
TEST_ONLY.resetHrtimeFunctionCache();
});
afterEach(() => {
process.hrtime = realHrtimeFn;
Date.now = realDateNow;
// Reset the hrtime reference so that it uses a real clock again.
TEST_ONLY.setHrtimeReference();
});
If this feels too involved, I'm OK with skipping it but wanted to put it there as an option. If you go this route, it could be helpful to export these as some kind of time testing helpers to easily set/unset the mocked time.
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.
@draffensperger Awesome, thanks for the help. I have modified the tests, PTAL.
@@ -95,6 +89,7 @@ describe('addInt64Gauge', () => { | |||
const pointEntry = int64Gauge.getOrCreateTimeSeries(LABEL_VALUES_200); | |||
pointEntry.add(100); | |||
|
|||
now = Date.now(); |
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.
Similar comment here about mocking out the time vs. measuring it in the test
@@ -281,6 +267,7 @@ describe('addDerivedInt64Gauge', () => { | |||
derivedInt64Gauge.createTimeSeries(LABEL_VALUES_200, map); | |||
map.set('key1', 'value1'); | |||
|
|||
now = Date.now(); |
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.
For these tests and others below too
import {getTimestampWithProcessHRTime} from '../src/common/time-util'; | ||
|
||
describe('getTimestampWithProcessHRTime()', () => { | ||
it('compare now with getTimestampWithProcessHRTime', () => { |
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.
What would you think about using time mocking here and adding tests that cover the cases where the nanoseconds both do and don't overflow?
What you could do is move the initialization logic in common/time-util.ts
above into a function say setupHrtimeDrift
or similar, then you would add a new exported variable export TEST_ONLY = {setupHrtimeDrift}
with a comment that is exported to enable testing the drift configuration (and of course you call the setupHrtimeDrift
in the time-util.ts
file).
Then you import the TEST_ONLY
symbol here in the test and call setupHrtimeDrift
after you have mocked process.hrtime
and Date.now
. Then you redefine them again before calling getTimestampWithProcessHRTime
for the two cases to test.
If this seems too involved, I'm open to leaving this as-is though.
Although process.hrtime() provides a high-resolution timer, it is from an unknown relative time, not epoch i.e. The time is relative to an arbitrary time in the past (not related to the time of day) and therefore not subject to clock drifts. The whole purpose of hrtime is to gather information on intervals or durations. That's why it doesn't really matter what the point in time was. Also, it is impossible to construct the ISO timestamp from hrtime, which is required when we export metrics data using stackdriver exporter. I am proposing to use Date.now function, as it returns the number of milliseconds elapsed since 1 January 1970 00:00:00 UTC. I have written timestampFromMillis function to construct Timestamp object from the given milliseconds.
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, thanks for changing this code to be testable with mocked time.
@justindsmith please take another look. Thanks. |
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!
Although
process.hrtime()
provides a high-resolution timer, it is from an unknown relative time, not epoch i.e. The time is relative to an arbitrary time in the past (not related to the time of day) and therefore not subject to clock drifts. The whole purpose ofhrtime
is to gather information on intervals or durations. That's why it doesn't really matter what the point in time was. Also, it is impossible to construct the ISO timestamp fromhrtime
, which is required when we export metrics data using stackdriver monitoring exporter.I raised this concern in #196, but I failed to justify reasoning that time. While converting OC metrics data to stackdriver exporter I faced the issue with converting
process.hrtime()
to a timestamp in RFC3339 UTC "Zulu" format something like this https://github.com/census-instrumentation/opencensus-node/blob/master/packages/opencensus-exporter-stackdriver/src/stackdriver-monitoring.ts#L183.I am proposing to use
Date.now
function, as it returns the number of milliseconds elapsed since 1 January 1970 00:00:00 UTC. I have writtentimestampFromMillis
function to construct Timestamp object from the given milliseconds. Although this is not accurate to nanoseconds, But I think this is the best we can get in nodejs. Correct me if I am wrong and let me know your views.