-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
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.
size-limit report
|
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 a few general questions, but otherwise looks good!
packages/ember/README.md
Outdated
); | ||
} | ||
|
||
export default instrumentRoutePerformance(SlowLoadingRoute); |
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.
This SlowLoadingRoute
looks mysterious here as the code sample could be better to make it more clear what that is.
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.
Not only is it mysterious, it's also wrong 😅, nice catch 👍
currentQueueStart = timestampWithMs(); | ||
|
||
instrumentedEmberQueues.forEach(queue => { | ||
run.scheduleOnce(queue, null, () => { |
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 scheduled twice on purpose?
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.
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; |
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.
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.
There's still sentry-javascript/packages/utils/src/misc.ts Lines 310 to 320 in c113427
|
…er-and-initial-load
… happens if the rest of performance isn't available
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:
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
Dummy app updated to include nested routes