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

Support and feedback from Sentry #78

Open
JonasBa opened this issue Mar 8, 2023 · 3 comments
Open

Support and feedback from Sentry #78

JonasBa opened this issue Mar 8, 2023 · 3 comments

Comments

@JonasBa
Copy link

JonasBa commented Mar 8, 2023

We (Sentry.io) are experimenting with adding support for JS Self Profiling to our JavaScript SDK family, see PR in getsentry/sentry-javascript#7273. Initial results look promising and profiles look valuable, especially when paired with our automated browser performance. Automated performance instrumentation creates new transactions for each page load and navigation event + tracks long tasks and creates spans for every http request during the transaction and profiling enriches all that with stack samples.

The screenshot below shows a profile that was collected by user initiated navigation event in our sentry dashboard (collected from a local development environment). The top transaction view shows requests and long tasks that were collected by automated instrumentation while the bottom list shows the profile collected during that time - the x axis is synced. There are two notable things we can see right away:

  • Our automated browser instrumentation has collected a long task performance resource entry at 1.6s into the transaction. During that time our main thread was blocked for close to 400ms -
  • By cross referencing the two axes we can see that the long task originated from a call to FromProfile function followed by react lifecycle updates.

https://user-images.githubusercontent.com/9317857/222572711-27a99e3e-2891-4cbd-bab3-849d87ed397f.png

The information collected from profiling allows us to immediately understand the issue and narrow down to it's root cause. Without profiling data, our best solution would have been to try and guess where this problem might be coming from and play a game of whac-a-mole by adding manual instrumentation in the hope that we would catch the culprit. Not to mention resolution time for an issue like this would be greatly increased as we would be required to redeploy our app each time and hope that we catch the issue again whereas js self profiling enables us to go from issue to root cause analysis (and ultimately even regression monitoring) without having to make any changes in our codebase.

Some feedback on the spec:

  • In order to enable JS self profiling, the js-profiling document policy header needs to be sent with the document response - this is fine, however there is no simple way (we may have missed it?) of checking for the presence of that policy header from our javascript SDK and attempting to initialize the profiler when that header is not sent results in an error being thrown and a violation being logged inside the browser console. We can catch the error by wrapping the constructor call in a try/catch block, but we cannot prevent the violation from being logged which is unfortunate and unnecessarily pollutes the console since we are already catching the error. Since the error is already being thrown, logging the violation seems excessive and could probably be removed.

Since the exact violation also correctly points to our SDK code hubextensions.js:104 [Violation] Document policy violation: js-profiling is not allowed in this document. it exposes us to confusion and questions from our users.

  • This is more of a comment, but browser related code is minified 99% of the time - this is why I shared the screenshot from our local development environment as our production profiles are minified which makes the visualization unreadable. At Sentry, we do have the infrastructure in place to resolve frames from source maps and many of our user already upload those to us, but this is something everyone wanting to support he spec will need to build.

We are excited to see this spec improve, profiling would be a great tool in web developers toolbox 👏

@acomminos
Copy link
Collaborator

acomminos commented Mar 14, 2023

In order to enable JS self profiling, the js-profiling document policy header needs to be sent with the document response - this is fine, however there is no simple way (we may have missed it?) of checking for the presence of that policy header from our javascript SDK and attempting to initialize the profiler when that header is not sent results in an error being thrown and a violation being logged inside the browser console.

This is being discussed for document policy as a whole at WICG/document-policy#27. Unfortunately, this doesn't seem to have made much progress.

While solving this at the document policy level is the right thing to do, I wonder if there's some kind of stopgap that makes sense to spec in -- or at least, as you mentioned, make the constructor failing less noisy (at least, in the Chrome implementation).

It may also be worth evaluating if new origin isolation primitives make it safer to block on profiler instantiation, thereby eliminating the hard requirement for the document policy to be set (leaving it solely as a flag for an AoT performance tradeoff).

@JonasBa
Copy link
Author

JonasBa commented Mar 15, 2023

While solving this at the document policy level is the right thing to do, I wonder if there's some kind of stopgap that makes sense to spec in -- or at least, as you mentioned, make the constructor failing less noisy (at least, in the Chrome implementation).

This could be a good short term solution.

One additional comment regarding the spec from our side is that the marker extensions proposal from @cnpsc seem like a critical addition to the spec. Speaking from personal experience, GC and layout are such common causes of janky experiences and developers should have visibility into them as well.

@JonasBa
Copy link
Author

JonasBa commented Aug 4, 2023

For folks reading this, we recently released support for this via our JS SDK - https://docs.sentry.io/platforms/javascript/profiling/

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

No branches or pull requests

2 participants