Skip to content

Conversation

@mydea
Copy link
Member

@mydea mydea commented Mar 3, 2023

This adds a new option routeLabel for the vue SDK which allows to configure if you want to use the route name, if it is set.

When setting this to path (default is name), we'll always use the path of the route even if a name exists.

Usage:

vueRouterInstrumentation(vueRouter, { routeLabel: 'path' });

Closes #5013

@mydea mydea added Type: Feature Package: vue Issues related to the Sentry Vue SDK labels Mar 3, 2023
@mydea mydea requested a review from Lms24 March 3, 2023 08:36
@mydea mydea self-assigned this Mar 3, 2023
Comment on lines 49 to 56

/**
* What to use for route labels.
* By default, we use route.name (if set) and else the path.
*
* Default: 'name'
*/
routeLabel: 'name' | 'path';
Copy link
Member

Choose a reason for hiding this comment

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

h: Instead of making this a top level option, wdyt about adding this as an option to the routing instrumentation?

*
* @param router The Vue Router instance that is used
*/
export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrumentation {
Copy link
Member

Choose a reason for hiding this comment

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

RE other comment:
I'd add a second optional parameter with a VueRouterInstrumentationOptions object here, in which routeLabel would live

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually a much better idea 👍

@mydea
Copy link
Member Author

mydea commented Mar 3, 2023

Updated it to be an option for the router, which makes much more sense!

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks for moving the option - LGTM!

@mydea mydea merged commit 95a5f48 into develop Mar 3, 2023
@mydea mydea deleted the fn/vue-path branch March 3, 2023 12:23
expect(mockStartTransaction).toHaveBeenLastCalledWith({
name: '/login',
metadata: {
source: 'route',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually 'route' or shouldn't this be 'url' now? This has an impact on dynamic sampling and the performance product.

Copy link
Member Author

Choose a reason for hiding this comment

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

This just disables using the name, so it uses the parametrized route when available and falls back to the URL if not.

See:

let transactionName: string = to.path;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Package: vue Issues related to the Sentry Vue SDK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[vue] Router instrumentation logs route name if set; missing option to log path instead

4 participants