fix: Use monotonic clock to compute durations #2441
Conversation
packages/apm/src/span.ts
Outdated
| @@ -261,7 +272,9 @@ export class Span implements SpanInterface, SpanContext { | |||
| return undefined; | |||
| } | |||
|
|
|||
| this.timestamp = timestampWithMs(); | |||
| // TODO: Fallback to timestampWithMs() when performance.now is unavailable. | |||
There was a problem hiding this comment.
Suggestions for how to do this welcome.
Could use a polyfill for performance.now or conditionally set _startTimestampMonotonic to some sentinel value and check it here.
There was a problem hiding this comment.
It looks like the policy for the JS SDK is to not polyfill anything older than Edge (i.e. IE 11 and older): #2003
So, I think we should be safe in not worrying about this 👍
| * Works with mostly any browser version released since 2012. | ||
| * https://caniuse.com/#search=performance.now | ||
| * | ||
| * Works with Node.js v8.5.0 or higher. | ||
| * https://nodejs.org/api/perf_hooks.html#perf_hooks_performance_now |
There was a problem hiding this comment.
Couldn't find what platform versions we support. What are we targeting?
I see a Travis job with Node 8, namely v8.17.0.
I also see Node 6, v6.17.1. Likely no performance.now there.
What about browser support?
|
Related: getsentry/sentry-python#631 |
|
I'm not opposed to this 👍 |
|
@rhcarvalho you probably need the following to address the travis build failures: import { isNodeEnv } from '@sentry/utils';
const { performance } = isNodeEnv() ? require('perf_hooks') : window.performance; |
Thanks @dashed! That helps :) I missed this step for node. I read that |
|
@rhcarvalho I think so. But I'm not familiar with WebWorkers unfortunately. |
rhcarvalho
force-pushed
the
rhcarvalho/span-monotonic
branch
from
c149180
to
5a236a8
Compare
A monotonic clock is monotonically increasing and not subject to system clock adjustments or system clock skew. The difference between any two chronologically recorded time values returned from the Performance.now() method MUST never be negative if the two time values have the same time origin. The same guarantee above does not exist for the difference between two calls to `new Date().getTime()` as used by `timestampWithMs()`. Resources: https://stackoverflow.com/questions/7272395/monotonically-increasing-time-in-javascript https://caniuse.com/#search=performance.now https://www.w3.org/TR/hr-time/#sec-monotonic-clock
rhcarvalho
force-pushed
the
rhcarvalho/span-monotonic
branch
from
5a236a8
to
1996a24
Compare
|
|
For Node.js we need For this to move forward we need either a hack to use a different timer or drop Node 6 support #2455. |
|
@rhcarvalho I'm highly confident that Node.js 6 support can be safely dropped as per https://github.com/nodejs/Release#end-of-life-releases |
A monotonic clock is monotonically increasing and not subject to system
clock adjustments or system clock skew. The difference between any two
chronologically recorded time values returned from the Performance.now()
method MUST never be negative if the two time values have the same time
origin.
The same guarantee above does not exist for the difference between two
calls to
new Date().getTime()as used bytimestampWithMs().Resources:
https://stackoverflow.com/questions/7272395/monotonically-increasing-time-in-javascript
https://caniuse.com/#search=performance.now
https://www.w3.org/TR/hr-time/
https://www.w3.org/TR/hr-time-2/
https://nodejs.org/api/perf_hooks.html
Know limitation
At present, we only address timestamp computation within a single span.
A more complete patch should compute all durations in a transaction using a monotonic clock and set all timestamps relative to a single reading of a wall clock time source.