ref(browser): Improve browserMetrics collection #13062
Conversation
There was a problem hiding this comment.
Hey @horochx thanks for opening this PR!
(heads-up: I'm only assigning myself for internal tracking purposes since I'm reviewing this PR)
If I understand the changes correctly, moving the getActiveSpan checks out of the loop is meant as a slight performance improvement, correct? I think that's okay (though only marginally faster because we'd return in the first iteration anyway).
I still had one question but after it is addressed, we can merge this.
For the future as well as for this PR: Please add a brief description of the changes to the PR description. That would be much appreciated, especially if there's no issue with more context. Thanks!
| if (!entry.scripts[0]) { | ||
| return; | ||
| break; |
There was a problem hiding this comment.
This is a behaviour change. Would you mind explaining why we should break here?
There was a problem hiding this comment.
Thank you for pointing that out. It was a typo, and I have corrected it to continue. The change from return to continue is to ensure that the remaining entry containing initialScript in list.getEntries() are not skipped and are reported properly.
| if (!entry.scripts[0]) { | ||
| return; | ||
| continue; |
There was a problem hiding this comment.
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.
@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.
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 with scripts and items without scripts. The current behavior skips reporting the remaining items with scripts after encountering one without scripts, so I've modified this behavior.
Once again, thank you for your review.
There was a problem hiding this comment.
Thanks! This fix makes sense to me.
| if (sourceCharPosition !== -1) { | ||
| attributes['browser.script.source_char_position'] = sourceCharPosition; | ||
| } | ||
| const { invoker, invokerType, sourceURL, sourceFunctionName, sourceCharPosition } = initialScript; |
There was a problem hiding this comment.
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.
Hello,
At line 136,
already checks for
scripts, so I have removed the redundant check here.
Lms24
changed the title
This PR adds the external contributor to the CHANGELOG.md file, so that they are credited for their contribution. See #13062 Co-authored-by: Lms24 <[email protected]>
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).