-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(tracing): Add long task collection #5529
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 adds long task collection to represent long running ui behaviour as spans in your frontend transactions. Behind an unexposed option for now.
@@ -69,7 +69,7 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { | |||
* | |||
* Default: undefined | |||
*/ | |||
_metricOptions?: Partial<{ _reportAllChanges: boolean }>; | |||
_metricOptions?: Partial<{ _reportAllChanges: boolean, _reportLongTasks: boolean }>; |
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 didn't really want to make another option bundle here, re-using something called 'metric options' when it makes spans doesn't sound totally right to me though.
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.
Can we just turn it on by default? Do we really need to gate to an option?
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.
Fair.
const transaction = getActiveTransaction() as IdleTransaction | undefined; | ||
let po : PerformanceObserver | undefined = undefined; | ||
const entryHandler: PerformanceEntryHandler = (entry: PerformanceEntry): void => { | ||
if (transaction?.endTimestamp && po) { |
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.
Checking for transaction being finished using end, if that still works, otherwise this just makes a span for long tasks, just keeping it simple to start, we can add attribution etc. later if it seems helpful.
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.
Why do we need to do this? Let's just attach it to any running transaction. Also we can't use optional chaining - it's increased final bundle as it's esnext feature.
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'll need to add tests before we can merge to validate.
We have browser integration tests - we can test if the span gets added here (and muck around with different scenarios and compare between chrome/safari/firefox): https://github.com/getsentry/sentry-javascript/tree/master/packages/integration-tests
const transaction = getActiveTransaction() as IdleTransaction | undefined; | ||
let po : PerformanceObserver | undefined = undefined; | ||
const entryHandler: PerformanceEntryHandler = (entry: PerformanceEntry): void => { | ||
if (transaction?.endTimestamp && po) { |
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.
Why do we need to do this? Let's just attach it to any running transaction. Also we can't use optional chaining - it's increased final bundle as it's esnext feature.
@@ -148,6 +148,7 @@ export class BrowserTracing implements Integration { | |||
|
|||
const { _metricOptions } = this.options; | |||
startTrackingWebVitals(_metricOptions && _metricOptions._reportAllChanges); | |||
startTrackingLongTasks(_metricOptions && _metricOptions._reportLongTasks); |
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.
IMO we rm -rf
the option and just
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.
yeah I'm fine with that
size-limit report 📦
|
@@ -19,6 +19,10 @@ module.exports = async ({ page, url, requests }) => { | |||
description: 'GET http://example.com', | |||
op: 'http.client', | |||
}, | |||
{ | |||
description: 'Long Task', | |||
op: 'ui.long-task', |
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.
woah :o
Seems like a new experimental feature, but we may be able to use TaskAttributionTiming to get some contextual data for LongTasks, which we could attach as additional data on LongTask spans |
The task attributions doesn't have good details IMO (spec: https://w3c.github.io/longtasks/#sec-TaskAttributionTiming), I don't know how much value we'll get by attaching it to the span. |
Yeah you're right, that's unfortunate. I was hoping that we might be able to get some useful info from |
… index 0 in the list
…removed if we find it to not be used.
f0879db
to
63a171c
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.
Good to merge this in when you are! We'll need to update the docs page to add the new experimental option: https://github.com/getsentry/sentry-docs/blob/master/src/platforms/javascript/common/performance/instrumentation/automatic-instrumentation.mdx
fixes #3807
Summary
This adds long task collection to represent long running ui behaviour as spans in your frontend transactions. Behind an unexposed option for now.
For context, we've been collecting long tasks in our main sentry UI now, and they can be quite helpful to determine where and when the ui is being blocked (and might hint where to add custom spans if a user chose to do so).
Screenshot
Example of us using it in Sentry already