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

feat(tracing): Track PerformanceObserver interactions as spans #7331

Merged
merged 12 commits into from Mar 9, 2023

Conversation

0Calories
Copy link
Contributor

@0Calories 0Calories commented Mar 3, 2023

Summary

We hypothesize that there is a 1:1 mapping of PerformanceObserver Interactions -> Interaction Transactions. This PR will allow us to confirm whether this hypothesis is true, so we can decide our next steps for refining interaction transactions. To elaborate, we're unsure whether interaction transactions that have no child spans are worth keeping or not, and there are a lot of transactions like this in prod.

Enhances the existing interaction transactions by adding the ability to track interaction events exposed by the PerformanceObserver API. Note that this is still hidden under an experimental flag along with interaction transactions.

For now, we are only tracking click interactions, but we can expand this functionality later as needed.

Example:

image

@0Calories 0Calories requested review from k-fish, AbhiPrasad and a team March 3, 2023 23:44
@0Calories 0Calories self-assigned this Mar 3, 2023
@0Calories 0Calories requested review from mydea and removed request for a team March 3, 2023 23:44
@AbhiPrasad
Copy link
Member

AbhiPrasad commented Mar 6, 2023

I know this is still in draft - but please make sure to add an integration test for this! You can model it after the existing interactions one: https://github.com/getsentry/sentry-javascript/tree/develop/packages/integration-tests/suites/tracing/browsertracing/interactions

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 20.22 KB (0%)
@sentry/browser - ES5 CDN Bundle (minified) 62.89 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.85 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified) 55.8 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 20.6 KB (0%)
@sentry/browser - Webpack (minified) 67.32 KB (0%)
@sentry/react - Webpack (gzipped + minified) 20.62 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 48.42 KB (+0.13% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 27.41 KB (+0.23% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.68 KB (+0.23% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 43.17 KB (+0.01% 🔺)
@sentry/replay - Webpack (gzipped + minified) 37.19 KB (0%)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 61.09 KB (+0.09% 🔺)
@sentry/browser + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 54.33 KB (0%)

@0Calories
Copy link
Contributor Author

0Calories commented Mar 6, 2023

@AbhiPrasad Would it be sufficient to modify that existing test so it also tests for the new interaction spans? It's already failing as is, so this will add coverage.

I'm going to add a new test as well, but I'm unsure whether it would be necessary.

@AbhiPrasad
Copy link
Member

Would it be sufficient to modify that existing test so it also tests for the new interaction spans? It's already failing as is, so this will add coverage.

That sounds reasonable to me!

@0Calories 0Calories marked this pull request as ready for review March 6, 2023 18:54
@0Calories
Copy link
Contributor Author

Hmmm I'm noticing the tests I added are flaky because sometimes the interaction spans are not getting created. This might be because of https://github.com/getsentry/sentry-javascript/pull/7331/files#diff-fa6ad17b9fe7cfcdf6f7ed76dd841d944709b7bc4abab6499db60a52d35b6681R92

I'm guessing that in these tests, the duration of these spans is sometimes below 10ms. Gonna look to see if I can get around this somehow

@AbhiPrasad
Copy link
Member

Can we add an event listener on click that is purposefully slow?

@0Calories
Copy link
Contributor Author

@AbhiPrasad That's worth a try, although I've tried setting durationThreshold to 0 and it still flakes sometimes 🙁

@0Calories
Copy link
Contributor Author

0Calories commented Mar 8, 2023

Tests have been fixed, looks like the main issue was that the script for adding the click event listener was being loaded before the DOM lol 😅

Ready for review now! @AbhiPrasad

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a screenshot of a transaction with these new spans attached to the PR description?

Feel free to merge after tests pass - we can cut a release with this tommorow so y'all can try it out.

@AbhiPrasad
Copy link
Member

Going to merge this in so we can cut a release!

@AbhiPrasad AbhiPrasad merged commit 361c5a4 into develop Mar 9, 2023
@AbhiPrasad AbhiPrasad deleted the feat/po-interaction-spans branch March 9, 2023 09:05
@hckhanh
Copy link

hckhanh commented Mar 18, 2023

Hi @AbhiPrasad @0Calories,
Is there any impact on Subscription usage when I enable this feature?

@0Calories
Copy link
Contributor Author

Hey @hckhanh,

Yes, please be careful with this feature as it will count towards your transaction quota! I've responded to your PR with instructions on how you can take advantage of this feature while discarding transactions that you don't want to send.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants