-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
fix: Reimplement timestamp computation #2947
fix: Reimplement timestamp computation #2947
Conversation
size-limit report
|
13d0a8c
to
a5212a2
Compare
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.
We don't need the react native workaround anymore since we always do what we did for react-native?
Yes (Notable Change nr. 3 in the PR description, and probably what makes the bundle sizes go a tiny bit down) |
packages/utils/src/time.ts
Outdated
if (!performance || !performance.now) { | ||
return performanceFallback; | ||
} | ||
let timestampSource = dateTimestampSource; |
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.
I'm pretty sure we can just move dateTimestampSource
down here and assign it conditionally.
/** | ||
* Returns a timestamp in seconds since the UNIX epoch using the Date API. | ||
*/ | ||
export const dateTimestampInSeconds = dateTimestampSource.nowSeconds.bind(dateTimestampSource); |
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.
I don't see any gains from using dateTimestampSource
instead of a simple named function. Unless we already know that we're going to add more methods there?
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.
Functions would of course work. I started this with documentation, and a common interface seemed like a reasonable way to explain that there are going to be multiple ways to generate a timestamp.
Date.now
and performance.now
are also methods, not global functions.
When exporting from utils
though, exporting a function keeps with the existing usage (timestampWithMs
) in call sites.
*/ | ||
function getReactNativePerformanceWrapper(): CrossPlatformPerformance { | ||
// Performance only available >= RN 0.63 | ||
export const browserPerformanceTimeOrigin = ((): number | undefined => { |
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.
I'm not sure how I feel about this tbh. Imports should imo be always deterministic, and here they are not.
WDY about writing getBrowserTimeOrigin
instead and caching the initiall result instead? This way we'll never end up with code:
import { something } from 'utils';
if (something) {
// wait, why it can be undefined? I imported it.
}
On the other hand, we'll end up with the code like:
import { getSometing } from 'utils';
const something = getSometing();
if (something) {
// wait, why it can be undefined? I imported it.
}
I'm not against or pro any of these options, so you can decide.
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.
Imports should imo be always deterministic
I agree.
Here, this value is only undefined
if performance
itself is undefined
.
This is essentially replacing our polyfill (setting a performance.timeOrigin
when it otherwise would not exist) with an explicit value we can use only within the SDK, without affecting other JS code.
The alternative was to repeat the fallback to performance.timing.navigationStart
everywhere the timeOrigin
is needed, which ends up being more than a one-liner as performance.timing
itself might be undefined (and we not using ?
... is that still the case?).
If you have yet another idea how to deal with this, I'm happy to update.
1af1f5e
to
5247320
Compare
This is a partial improvement to address inconsistencies in browsers and how they implement performance.now(). It mitigates observing timestamps in the past for pageload transactions, but gives no guarantees to navigation transactions. Depending on the platform, the clock used in performance.now() may stop when the computer goes to sleep, creating an ever increasing skew. This skew is more likely to manifest in navigation transactions. Notable Changes - Do not polyfill/patch performance.timeOrigin to avoid changing behavior of third parties that may depend on it. Instead, use an explicit browserPerformanceTimeOrigin where needed. - Use a timestamp based on the Date API for error events, breadcrumbs and envelope header. This should fully resolve the problem of ingesting events with timestamps in the past, as long as the client wall clock is reasonably in sync. - Apply an equivalent workaround that we used for React Native to all JavaScript environments, essentially resetting the timeOrigin used to compute monotonic timestamps when execution starts.
5247320
to
c112acb
Compare
This is a partial improvement to address inconsistencies in browsers and how they implement
performance.now()
.Related to #2590.
It mitigates observing timestamps in the past for
pageload
transactions, but gives no (new) guarantees tonavigation
transactions.Depending on the platform, the clock used in
performance.now()
may stop when the computer goes to sleep, creating an ever increasing skew. This skew is more likely to manifest innavigation
transactions, as they typically happen in Single Page Applications that stay open for days, weeks or even months.Notable Changes
Do not polyfill/patch
performance.timeOrigin
to avoid changing behavior of third parties that may depend on it. Instead, use an explicitbrowserPerformanceTimeOrigin
where needed.Use a timestamp based on the Date API for error events, breadcrumbs and envelope header. This should fully resolve the problem of ingesting events with timestamps in the past, as long as the client wall clock is reasonably in sync.
Apply an equivalent workaround that we used for React Native to all JavaScript environments, essentially resetting the timeOrigin used to compute monotonic timestamps when execution starts.
This doesn't intend to fully solve the problems investigated as part of #2590, but to give one step forward in the right direction addressing improvements that can be had today.
We've been discussing and designing internally a more well-rounded solution to ship to all SDKs as an implementation guideline, stay tuned.