Skip to content
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

🐛 fix unexpected exception when no entry type is supported in PerformanceObserver #2899

Merged
merged 2 commits into from
Jul 26, 2024

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Jul 26, 2024

Motivation

Following #2855 and #2818, we are using more granular PerformanceObserver. We observe less entry types in the PerformanceObserver from performanceCollection, and in the new performanceObservable, we observe a single type at once.

We observed that in old versions of Safari, if all entry types supplied to .observe(..) are unsupported, an exception is thrown: entryTypes contained only unsupported types. Illustration:

// assuming 'resource' is supported but not 'longtask'
new PerformanceObserver().observe({ entryTypes: ['resource', 'longtask'] }) // succeeds
new PerformanceObserver().observe({ entryTypes: ['longtask'] }) // throws

Changes

To work around this issue, this PR wraps .observe(..) calls in try/catch blocks to prevent the SDK from crashing.

Testing

  • Local: even on BrowserStack we don't have the exact Safari version that triggers this issue (it should be Safari 11 with OSX 10.11 or below, see notes here). But I tested that we still collect performance resource entries in the Safari 11 version provided by BrowserStack.
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@BenoitZugmeyer BenoitZugmeyer requested a review from a team as a code owner July 26, 2024 13:27
@BenoitZugmeyer
Copy link
Member Author

/to-staging

@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jul 26, 2024

🚂 Branch Integration: starting soon, median merge time is 11m

Commit 51567db0e6 will soon be integrated into staging-30.

Use /to-staging -c to cancel this operation!

dd-mergequeue bot added a commit that referenced this pull request Jul 26, 2024
…ng-30

Integrated commit sha: 51567db

Co-authored-by: Benoît Zugmeyer <[email protected]>
@dd-devflow
Copy link
Contributor

dd-devflow bot commented Jul 26, 2024

🚂 Branch Integration: This commit was successfully integrated

Commit 51567db0e6 has been merged into staging-30 in merge commit 059bf94ebd.

Check out the triggered pipeline on Gitlab 🦊

@BenoitZugmeyer BenoitZugmeyer merged commit 73681aa into main Jul 26, 2024
21 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/make-sure-entry-type-is-supported branch July 26, 2024 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants