Skip to content
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: Try to use a better performance API #3356

Merged
merged 12 commits into from Mar 31, 2021

Conversation

wmak
Copy link
Member

@wmak wmak commented Mar 30, 2021

  • Trying to address the inconsistencies between browsers
  • Always try to use the timeOrigin first, as long as its consistent with the current time
  • Otherwise, use the navigationStart if its consistent with the current time
  • And still fallback to date if all of the above fails
  • Remember what source is used for the time origin, so we can tag events and analyze later
  • An attempt to fix Timing issues using Performance API #2590

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests. -- Manually tested with Safari, Chrome + Firefox
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

- Trying to address the inconsistencies between browsers
- Always try to use the timeOrigin first, as long as its consistent with
  either the navigationStart or the current time
- Otherwise, use the navigationStart if its consistent with the current
  time
- And still fallback to date if all else fails
@wmak wmak requested a review from kamilogorek as a code owner March 30, 2021 15:11
@wmak wmak requested a review from rhcarvalho March 30, 2021 15:22
@github-actions
Copy link
Contributor

github-actions bot commented Mar 30, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.51 KB (+0.19% 🔺)
@sentry/browser - Webpack 21.37 KB (+0.26% 🔺)
@sentry/react - Webpack 21.41 KB (+0.26% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 27.6 KB (+0.14% 🔺)

@dashed
Copy link
Member

dashed commented Mar 30, 2021

@wmak were you able to test this change in Safari, Chrome, and Firefox?

@wmak
Copy link
Member Author

wmak commented Mar 30, 2021

@dashed
Yup!
In firefox when timeOrigin was incorrect, this changes to timing.navigationStart, and if timeOrigin was correct it would continue using it.
In chrome where timeOrigin is usually correct this continues to use it
in Safari which only has timing.navigationStart this continues to use that.

packages/utils/src/time.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

I like this, it can likely mitigate the problems we're seeing on FF.

Thanks @wmak <3

packages/utils/src/time.ts Outdated Show resolved Hide resolved
packages/utils/src/time.ts Outdated Show resolved Hide resolved
packages/utils/src/time.ts Show resolved Hide resolved
packages/utils/src/time.ts Outdated Show resolved Hide resolved
wmak and others added 2 commits March 30, 2021 13:01
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
@wmak
Copy link
Member Author

wmak commented Mar 30, 2021

Also, starting with Threshold == 1 hour, but this will likely mean we start seeing inaccurate durations up to an hour, if this solution works out we could drop Threshold down to a few seconds instead, just want to start with something safer for now

packages/utils/src/time.ts Outdated Show resolved Hide resolved
Co-authored-by: Alberto Leal <mail4alberto@gmail.com>
Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

I'd love to see this in prod.

@wmak I had one idea since yesterday -- as a way to debug and quantify how this is working, we could set some internal variable with a string depending on what time origin is being used (timeOrigin, navigationStart or dateNow) and then in Sentry we can use this + event processor to set a tag on every transaction, so we can filter and see what fraction of users use which method, see if there is a clear browser correlation etc.
We could also mark/tag whenever we decided that timeOrigin or navigationStart is reliable or not.

@kamilogorek would you have some suggestion on how to implement the "internal global value"? E.g. how exactly to bind to the global sentry object?

@rhcarvalho
Copy link
Contributor

Getting this in so we can test across more browsers in the wild.

@rhcarvalho rhcarvalho merged commit dece89a into master Mar 31, 2021
@rhcarvalho rhcarvalho deleted the wmak/fix/performance-time-origin branch March 31, 2021 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Timing issues using Performance API
3 participants