feat(tracing): Implement new browserTracingIntegration()
#10327
Conversation
size-limit report 📦
|
|
Note: For react, we may still need a
|
Due to #10327, this reverts changes to the browser tracing integration as we're actually going to replace this differently.
mydea
force-pushed
the
fn/newBrowserTracingIntegration
branch
from
6fc3734
to
92ba0ba
Compare
|
Actually I'd probably export utilities |
Also migrate angular as an example.
mydea
force-pushed
the
fn/newBrowserTracingIntegration
branch
from
92ba0ba
to
a4488c1
Compare
There was a problem hiding this comment.
The new APIs make sense to me and I think they work well with Angular and the other frameworks we mentioned in the call.
Reminder to myself that we should add some Angular e2e tests to cover this properly. But as we discussed we don't change anything behaviour-wise for the page load transaction so we should be good.
| return originalBrowserTracingIntegration({ | ||
| ...options, | ||
| instrumentPageLoad: true, | ||
| // We handle this manually | ||
| instrumentNavigation: false, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I guess one downside of this approach I just thought of is that there will be no navigation span/txn at all, if users don't register the TraceService. Previously they would have gotten the default browser one.
I think this is okay though - "incorrect" or incomplete SDK setup shouldn't be something we care too much about IMO. (Also, this is documented pretty clearly in docs)
There was a problem hiding this comment.
yeah, I think I will refactor this based on what we talked about that we actually never overwrite the defaults like this but only do it via checking if something else registered a hook instead!
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Extracted this out of #10327. This PR: * Introduces a new `browserTracingIntegration()` * Does NOT deprecate BrowserTracing yet, as custom implementations in Angular, Next, Sveltekit have not been migrated over yet, which would be weird. We can deprecate it once we moved these over. * Makes sure that custom implementations in Next & Sveltekit are "fixed" automatically * Uses a slim fork for the CDN bundles, to avoid shipping multiple implementations in there. * This means that in the CDN bundles, you can already use the new syntax, but you cannot pass a custom routing instrumentation anymore, and you also don't have the utility functions for it yet. I think this is the best tradeoff for now, and it's probably not a super common case to have custom routing instrumentation when using the Loader/CDN bundles (and if you do, you have to stick to `new BrowserTracing()` until v8). I copied the browser integration tests we have, which all pass!
|
this was implemented in a different PR. |
While looking to deprecate/replace
instrumentNavigationwith something withoutstartTransaction, I figured it may actually be better to already make the functional replacement forbrowserTracingIntegrationbehave this way, so we can do this in one swoop.This now exposes a new
browserTracingIntegration()which relies on hooks to start the idle spans. I implemented this in Angular as an example for how this would work then - which also means that you can now instrument Angular without any further config forbrowserTracingIntegration()there!This replaces #10324 and the previous, so far unreleased
browserTracingIntegration().