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): Add transaction source to VueRouter instrumentation #5381

Merged
merged 4 commits into from Jul 8, 2022

Conversation

lforst
Copy link
Member

@lforst lforst commented Jul 7, 2022

Ref: #5345

Set transaction source on the VueRouter instrumentation and update the route types (this more acts like documentation).

@lforst lforst mentioned this pull request Jul 7, 2022
15 tasks
@lforst lforst self-assigned this Jul 7, 2022
@AbhiPrasad AbhiPrasad added this to the Dynamic Sampling Context milestone Jul 7, 2022
@@ -83,7 +83,7 @@ export const createTracingMixins = (options: TracingOptions): Mixins => {
// Skip components that we don't want to track to minimize the noise and give a more granular control to the user
const name = formatComponentName(this, false);
const shouldTrack = Array.isArray(options.trackComponents)
? options.trackComponents.includes(name)
? options.trackComponents.indexOf(name) > -1
Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't get tests to work because there was a type error here. I could have messed with tsconfigs but there is an upside of doing indexOf() instead of includes() which is backwards compatibility. I think includes is ES6 only.

Copy link
Member

Choose a reason for hiding this comment

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

Lol, coincidence? #5389

@lforst lforst marked this pull request as ready for review July 8, 2022 06:53
@lforst lforst requested review from AbhiPrasad and Lms24 July 8, 2022 06:54
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.

Nice, LGTM 🚀

@@ -51,23 +62,38 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument
query: to.query,
};

// Determine a name for the routing transaction and where that name came from
let transactionName: string = to.path;
let transactionSource: 'url' | 'custom' | 'route' = 'url';
Copy link
Member

@Lms24 Lms24 Jul 8, 2022

Choose a reason for hiding this comment

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

Totally optional but wdyt about extracting'url' | 'custom' | 'route' to a type like in the ReactRouterV3 implementation? Just for consistency...

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, at the time of opening this I didn't see that we exported the TransactionSource type. Thanks!

@lforst lforst enabled auto-merge (squash) July 8, 2022 10:17
@lforst lforst merged commit 0dbbac6 into master Jul 8, 2022
@lforst lforst deleted the lforst-vue-transaction-source branch July 8, 2022 10:27
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

Successfully merging this pull request may close these issues.

None yet

3 participants