Skip to content

feat(ember): Add more render instrumentation to @sentry/ember #2902

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

Merged
merged 12 commits into from
Sep 21, 2020

Conversation

k-fish
Copy link
Member

@k-fish k-fish commented Sep 15, 2020

Summary

This PR will fill out the rest of the instrumentation for Ember to provide meaningful spans across most of the transaction.

It adds instrumentation for:

  • initial load (which uses the build tools to inject a small script into head to measure script evaluation, which includes initial render)
  • the runloop, to capture queues that take a long time, this can primarily be used to capture long render queues when someone is using the new glimmer components as they don't have instrumentation.
  • components (non-glimmer) render times

Additionally it fixes a couple bugs with routes, especially when nesting, such as beforeModel not consistently getting instrumented.

Screenshots

Example of transaction from the dummy application

Screen Shot 2020-09-14 at 9 06 18 PM

Dummy app updated to include nested routes

Screen Shot 2020-09-14 at 9 23 56 PM

This PR will fill out the rest of the instrumentation for Ember to provide meaningful spans across most of the transaction.

It adds instrumentation for:
- initial load (which issues the build tools to inject a small script into head to measure script evaluation, which includes initial render)
- the runloop, to capture queues that take a long time, this can primarily be used to capture long render queues when someone is using the new glimmer components as they don't have instrumentation.
- components (non-glimmer) render times

Additionally it fixes a couple bugs with routes, especially when nesting, such as beforeModel not consistently getting instrumented.
@k-fish k-fish requested a review from kamilogorek as a code owner September 15, 2020 04:26
@github-actions
Copy link
Contributor

github-actions bot commented Sep 15, 2020

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 17.96 KB (0%)
@sentry/browser - Webpack 18.8 KB (0%)
@sentry/react - Webpack 18.8 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 23.76 KB (+0.01% 🔺)

@k-fish k-fish requested a review from a team September 16, 2020 17:10
Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

Just a few general questions, but otherwise looks good!

);
}

export default instrumentRoutePerformance(SlowLoadingRoute);
Copy link
Contributor

Choose a reason for hiding this comment

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

This SlowLoadingRoute looks mysterious here as the code sample could be better to make it more clear what that is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not only is it mysterious, it's also wrong 😅, nice catch 👍

currentQueueStart = timestampWithMs();

instrumentedEmberQueues.forEach(queue => {
run.scheduleOnce(queue, null, () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this scheduled twice on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, once to put it into the queue and then once in the queue to put it towards the end once the queue is being processed (since they get created on the runloop's begin).

const measures = performance.getEntriesByName(measureName);
const measure = measures[0];

const startTimestamp = (measure.startTime + performance.timeOrigin) / 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Be aware that we had a lot of issues with environments where performance is not available. That's why in our utils we have things like crossPlatformPerformance function.

@kamilogorek
Copy link
Contributor

There's still performance.measure, performance.getEntriesByName and most importantly timeOrigin. See the issue with the last one here -

// Polyfill for performance.timeOrigin.
//
// While performance.timing.navigationStart is deprecated in favor of performance.timeOrigin, performance.timeOrigin
// is not as widely supported. Namely, performance.timeOrigin is undefined in Safari as of writing.
if (performance.timeOrigin === undefined) {
// As of writing, performance.timing is not available in Web Workers in mainstream browsers, so it is not always a
// valid fallback. In the absence of a initial time provided by the browser, fallback to INITIAL_TIME.
// @ts-ignore ignored because timeOrigin is a readonly property but we want to override
// eslint-disable-next-line deprecation/deprecation
performance.timeOrigin = (performance.timing && performance.timing.navigationStart) || INITIAL_TIME;
}

@k-fish k-fish merged commit 573c49f into master Sep 21, 2020
@k-fish k-fish deleted the feat/ember-add-render-and-initial-load branch September 21, 2020 21:36
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.

2 participants