-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(vue): Update Vue trackComponents
list to match components with or without <>
#13543
Changes from 3 commits
10bc596
2c67d73
e7bc6a3
8f48f36
ab13a0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -122,3 +122,101 @@ test('sends a pageload transaction with a route name as transaction name if avai | |
}, | ||
}); | ||
}); | ||
|
||
test('sends a lifecycle span for the tracked HomeView component - with `<>`', async ({ page }) => { | ||
const transactionPromise = waitForTransaction('vue-3', async transactionEvent => { | ||
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload'; | ||
}); | ||
|
||
await page.goto(`/`); | ||
|
||
const rootSpan = await transactionPromise; | ||
|
||
expect(rootSpan).toMatchObject({ | ||
contexts: { | ||
trace: { | ||
data: { | ||
'sentry.source': 'route', | ||
'sentry.origin': 'auto.pageload.vue', | ||
'sentry.op': 'pageload', | ||
}, | ||
op: 'pageload', | ||
origin: 'auto.pageload.vue', | ||
}, | ||
}, | ||
spans: expect.arrayContaining([ | ||
// enabled by default | ||
expect.objectContaining({ | ||
data: { | ||
'sentry.op': 'ui.vue.render', | ||
'sentry.origin': 'auto.ui.vue', | ||
}, | ||
description: 'Application Render', | ||
op: 'ui.vue.render', | ||
origin: 'auto.ui.vue', | ||
}), | ||
// enabled by default | ||
expect.objectContaining({ | ||
data: { | ||
'sentry.op': 'ui.vue.mount', | ||
'sentry.origin': 'auto.ui.vue', | ||
}, | ||
description: 'Vue <<Root>>', | ||
op: 'ui.vue.mount', | ||
origin: 'auto.ui.vue', | ||
}), | ||
expect.objectContaining({ | ||
data: { | ||
'sentry.op': 'ui.vue.mount', | ||
'sentry.origin': 'auto.ui.vue', | ||
}, | ||
description: 'Vue <<HomeView>>', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just making sure, are the double brackets intended? They're formatted here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good observation! I don't think they're intended. Let's remove them. |
||
op: 'ui.vue.mount', | ||
origin: 'auto.ui.vue', | ||
}), | ||
]), | ||
transaction: '/', | ||
transaction_info: { | ||
source: 'route', | ||
}, | ||
}); | ||
}); | ||
|
||
test('sends a lifecycle span for the tracked UserIdErrorView component - without `<>`', async ({ page }) => { | ||
const transactionPromise = waitForTransaction('vue-3', async transactionEvent => { | ||
return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload'; | ||
}); | ||
|
||
await page.goto(`/users-error/123`); | ||
|
||
const rootSpan = await transactionPromise; | ||
|
||
expect(rootSpan).toMatchObject({ | ||
contexts: { | ||
trace: { | ||
data: { | ||
'sentry.source': 'route', | ||
'sentry.origin': 'auto.pageload.vue', | ||
'sentry.op': 'pageload', | ||
}, | ||
op: 'pageload', | ||
origin: 'auto.pageload.vue', | ||
}, | ||
}, | ||
spans: expect.arrayContaining([ | ||
expect.objectContaining({ | ||
data: { | ||
'sentry.op': 'ui.vue.mount', | ||
'sentry.origin': 'auto.ui.vue', | ||
}, | ||
description: 'Vue <<UserIdErrorView>>', | ||
op: 'ui.vue.mount', | ||
origin: 'auto.ui.vue', | ||
}), | ||
]), | ||
transaction: '/users-error/:id', | ||
transaction_info: { | ||
source: 'route', | ||
}, | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,19 @@ function finishRootSpan(vm: VueSentry, timestamp: number, timeout: number): void | |
}, timeout); | ||
} | ||
|
||
/** Find if the current component exists in the provided `TracingOptions.trackComponents` array option. */ | ||
export function findTrackComponent(trackComponents: string[], formattedName: string): boolean { | ||
function extractComponentName(name: string): string { | ||
return name.replace(/^<([^\s]*)>(?: at [^\s]*)?$/, '$1'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on what you suggested @Lms24, just an added non-capture group. The ReDoS Checker passes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me, thanks! |
||
} | ||
|
||
const isMatched = trackComponents.some(compo => { | ||
return extractComponentName(formattedName) === extractComponentName(compo); | ||
}); | ||
|
||
return isMatched; | ||
} | ||
|
||
export const createTracingMixins = (options: TracingOptions): Mixins => { | ||
const hooks = (options.hooks || []) | ||
.concat(DEFAULT_HOOKS) | ||
|
@@ -84,8 +97,9 @@ 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.indexOf(name) > -1 | ||
? findTrackComponent(options.trackComponents, name) | ||
: options.trackComponents; | ||
|
||
// We always want to track root component | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { describe, expect, it } from 'vitest'; | ||
import { findTrackComponent } from '../../src/tracing'; | ||
|
||
describe('findTrackComponent', () => { | ||
describe('when user-defined array contains `<Component>`', () => { | ||
it('returns true if a match is found', () => { | ||
// arrange | ||
const trackComponents = ['<ABC>', '<XYZ>']; | ||
const formattedComponentName = '<XYZ>'; | ||
|
||
// act | ||
const shouldTrack = findTrackComponent(trackComponents, formattedComponentName); | ||
|
||
// assert | ||
expect(shouldTrack).toBe(true); | ||
}); | ||
}); | ||
describe('when user-defined array contains `Component` without the `<>`', () => { | ||
it('returns true if a match is found', () => { | ||
// arrange | ||
const trackComponents = ['ABC', 'XYZ']; | ||
const formattedComponentName = '<XYZ>'; | ||
|
||
// act | ||
const shouldTrack = findTrackComponent(trackComponents, formattedComponentName); | ||
|
||
// assert | ||
expect(shouldTrack).toBe(true); | ||
}); | ||
}); | ||
describe('when the vue file name is include in the formatted component name', () => { | ||
it('returns true if a match is found', () => { | ||
// arrange | ||
const trackComponents = ['ABC', 'XYZ']; | ||
const formattedComponentName = '<XYZ> at XYZ.vue'; | ||
|
||
// act | ||
const shouldTrack = findTrackComponent(trackComponents, formattedComponentName); | ||
|
||
// assert | ||
expect(shouldTrack).toBe(true); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of an unfortunate and very intransparent limitation of our e2e test setup but we shouldn't navigate to the same route in more than one e2e test. The reason is that we execute them in parallel in CI and the
waitForTransaction
call from one test might actually resolve for a transaction in another test, depending on the condition in the callback.To avoid this confusion, we generally create a new route for each test. Suggestion: Either we create new routes for the two new tests or we integrate them in already existing tests and simply check for the spans there. I'll let you make the call, I'm fine with either option.
Also, feel free to test both kinds
trackComponent
notations in one route/test :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, thanks for the info. i decided to go with creating a new route, and grouped the different notations together in a single test.