-
-
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
ref(browser): Improve browserMetrics collection #13062
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -99,10 +99,10 @@ export function startTrackingWebVitals(): () => void { | |||
*/ | ||||
export function startTrackingLongTasks(): void { | ||||
addPerformanceInstrumentationHandler('longtask', ({ entries }) => { | ||||
if (!getActiveSpan()) { | ||||
return; | ||||
} | ||||
for (const entry of entries) { | ||||
if (!getActiveSpan()) { | ||||
return; | ||||
} | ||||
const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime); | ||||
const duration = msToSec(entry.duration); | ||||
|
||||
|
@@ -129,12 +129,12 @@ export function startTrackingLongAnimationFrames(): void { | |||
// we directly observe `long-animation-frame` events instead of through the web-vitals | ||||
// `observe` helper function. | ||||
const observer = new PerformanceObserver(list => { | ||||
if (!getActiveSpan()) { | ||||
return; | ||||
} | ||||
for (const entry of list.getEntries() as PerformanceLongAnimationFrameTiming[]) { | ||||
if (!getActiveSpan()) { | ||||
return; | ||||
} | ||||
if (!entry.scripts[0]) { | ||||
return; | ||||
continue; | ||||
} | ||||
|
||||
const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime); | ||||
|
@@ -143,20 +143,19 @@ export function startTrackingLongAnimationFrames(): void { | |||
const attributes: SpanAttributes = { | ||||
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.browser.metrics', | ||||
}; | ||||
|
||||
const initialScript = entry.scripts[0]; | ||||
if (initialScript) { | ||||
const { invoker, invokerType, sourceURL, sourceFunctionName, sourceCharPosition } = initialScript; | ||||
attributes['browser.script.invoker'] = invoker; | ||||
attributes['browser.script.invoker_type'] = invokerType; | ||||
if (sourceURL) { | ||||
attributes['code.filepath'] = sourceURL; | ||||
} | ||||
if (sourceFunctionName) { | ||||
attributes['code.function'] = sourceFunctionName; | ||||
} | ||||
if (sourceCharPosition !== -1) { | ||||
attributes['browser.script.source_char_position'] = sourceCharPosition; | ||||
} | ||||
const { invoker, invokerType, sourceURL, sourceFunctionName, sourceCharPosition } = initialScript; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why was this guard removed? We kept this here intentionally to ensure that we wouldn't do assignment unless the array item was defined. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello, At line 136,
already checks for scripts , so I have removed the redundant check here.
|
||||
attributes['browser.script.invoker'] = invoker; | ||||
attributes['browser.script.invoker_type'] = invokerType; | ||||
if (sourceURL) { | ||||
attributes['code.filepath'] = sourceURL; | ||||
} | ||||
if (sourceFunctionName) { | ||||
attributes['code.function'] = sourceFunctionName; | ||||
} | ||||
if (sourceCharPosition !== -1) { | ||||
attributes['browser.script.source_char_position'] = sourceCharPosition; | ||||
} | ||||
|
||||
const span = startInactiveSpan({ | ||||
|
@@ -179,11 +178,10 @@ export function startTrackingLongAnimationFrames(): void { | |||
*/ | ||||
export function startTrackingInteractions(): void { | ||||
addPerformanceInstrumentationHandler('event', ({ entries }) => { | ||||
if (!getActiveSpan()) { | ||||
return; | ||||
} | ||||
for (const entry of entries) { | ||||
if (!getActiveSpan()) { | ||||
return; | ||||
} | ||||
|
||||
if (entry.name === 'click') { | ||||
const startTime = msToSec((browserPerformanceTimeOrigin as number) + entry.startTime); | ||||
const duration = msToSec(entry.duration); | ||||
|
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 there a bug that this change fixes? I think it's probably fine to go through all entries but I'm curious.
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.
@KevinL10 would you mind checking if this is fine? I believe you added the long animation frame instrumentation so I'd like to run it by you.
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.
Sorry, I'm currently on a bus and can't reply promptly.
Here's my use case:
I asynchronously load Sentry and have just started optimizing performance. Upon initial execution of the LoAF callback, I found that the
entry
in the callback contains items withscripts
and items withoutscripts
. The current behavior skips reporting the remaining items withscripts
after encountering one withoutscripts
, so I've modified this behavior.Once again, thank you for your review.
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.
Thanks! This fix makes sense to me.