feat(vue): Implement vue browserTracingIntegration()
#10477
Conversation
| @@ -72,76 +70,105 @@ export function vueRouterInstrumentation( | |||
| name: WINDOW.location.pathname, | |||
| op: 'pageload', | |||
| origin: 'auto.pageload.vue', | |||
| tags, | |||
| data: { | |||
| attributes: { | |||
There was a problem hiding this comment.
I'd say it's ok to stop sending this as a tag and start sending it as an attribute - I don't think anybody will depend on this too much...? Does not seem too relevant for a user.
There was a problem hiding this comment.
agreed. I made the same change in the SvelteKit browserTracingIntegration. Realistically, we probably don't need it at all, given that we have sentry.origin but it doesn't hurt us to send it as an attribute.
There was a problem hiding this comment.
IMO we can just remove it now that we have sentry.origin across the board, let's save some bundle size.
That was the purpose of having the routing.instrumentation tag in the first place
| let transactionSource: TransactionSource = 'url'; | ||
| if (to.name && options.routeLabel !== 'path') { | ||
| transactionName = to.name.toString(); | ||
| transactionSource = 'custom'; | ||
| } else if (to.matched[0] && to.matched[0].path) { | ||
| transactionName = to.matched[0].path; | ||
| transactionSource = 'route'; | ||
| } |
There was a problem hiding this comment.
I think something here is slightly off but it's also off in the original implementation. See #10476 (comment)
Should we just always send route? I mean even if we take the name instead of the path, it still represents a route ultimately. Just a thought. Maybe we do it in a separate PR to fix this atomically in case we need to revert.
There was a problem hiding this comment.
yeah, good point 🤔 do we have any place where we rely on some behavior that if source===route we expect it to be a literal route, as in a URL path? 🤔 if not, probably OK to keep this route always I think - cc @AbhiPrasad
There was a problem hiding this comment.
afaik the only important distinction in Relay is between source url (where we apply server side clustering/grouping) vs all other sources (like route and custom, where we don't apply this). But not 100% sure anymore.
There was a problem hiding this comment.
I asked the ingest team if there's a difference.
There was a problem hiding this comment.
I guess internally we also only attach transaction names if they are not URLs to baggage.
I don't know the Relay specifics either, it'll be good to double check them.
There was a problem hiding this comment.
I'll merge this as is (as it is the same behavior as before) and as Lukas said, we can change it in a separate PR where we can be clear about the reasoning etc!
mydea
force-pushed
the
fn/vue-browserTracing
branch
2 times, most recently
from
f0cf86c to
99efe25
Compare
size-limit report 📦
|
This replaces the `vueRouterInstrumentation` and allows to deprecate browser tracing in the vue package. fix tests remove unneeded attribute
mydea
force-pushed
the
fn/vue-browserTracing
branch
from
99efe25 to
dcc693f
Compare
| // This also only happens during a navigation. A pageload will set the source as 'route'. | ||
| // TODO: Figure out what's going on here. | ||
| source: 'custom', | ||
| source: 'route', |
There was a problem hiding this comment.
@Lms24 I think I fixed the "issue" from this comment here 😅 (I check this both in the "old" format and in the new one, seems to be working in both now, hopefully.
This replaces the
vueRouterInstrumentationand allows to deprecate browser tracing in the vue package.Waiting for #10476, then we should put these changes on top of the E2E test to verify it still works.