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(vue): Allow to set routeLabel: 'path' to opt-out of using name #7326

Merged
merged 2 commits into from Mar 3, 2023

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

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?

@@ -38,6 +39,9 @@ interface VueRouter {
* @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
Member

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
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
3 participants