Skip to content

fix(vue): Update Vue trackComponents list to match components with or without <> #13543

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 9, 2024
Merged

fix(vue): Update Vue trackComponents list to match components with or without <> #13543

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
10bc596
fix(vue): Update Vue `trackComponents` list to match components with …
Zen-cronic Aug 30, 2024
2c67d73
test(vue): Add unit test for `findTrackComponent` function
Zen-cronic Sep 5, 2024
e7bc6a3
test(vue): Add e2e test cases for trackComponents span
Zen-cronic Sep 6, 2024
8f48f36
fix(vue): Remove extra angle bracket in component span description
Zen-cronic Sep 6, 2024
ab13a0b
test(vue): Add new route for trackComponent tests
Zen-cronic Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion packages/vue/src/tracing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,20 @@ function finishRootSpan(vm: VueSentry, timestamp: number, timeout: number): void
}, timeout);
}

// Find if the current component exists in the provided `TracingOptions.trackComponents` array option.
function findTrackComponent(trackComponents: string[], formattedName: string): boolean {
const isMatched = trackComponents.some(compo => {
let currentCompo = compo;

if (!(currentCompo.startsWith('<') && currentCompo.endsWith('>'))) {
currentCompo = `<${currentCompo}>`;
}
return formattedName === currentCompo;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

formattedName from formatComponentName() always return a name with < >.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it can also be <Component> at Component.vue.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can replace this with a more bundle size efficient version:

Suggested change
let currentCompo = compo;
if (!(currentCompo.startsWith('<') && currentCompo.endsWith('>'))) {
currentCompo = `<${currentCompo}>`;
}
return formattedName === currentCompo;
return compo.replace(/^<(.*)>$/, '$1') === formattedName

Copy link
Member

Choose a reason for hiding this comment

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

Ah in this case we don't even need the block anymore but could inline it to the some call. So feel free to do that as well :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey, I've taken your approach and found that formattedName must also be replaced:

function findTrackComponent(trackComponents: string[], formattedName: string): boolean {

  function extractComponentName(name: string): string {
    return name.replace(/^<(.*)>(?:.*)?$/, "$1");
  }
  const isMatched = trackComponents.some(compo => {
    return    extractComponentName(formattedName) === extractComponentName(compo)
  }
  );
  return isMatched;
}

The regex is needed for both the user provided component and the formatted component.

  • User provided: either <App> or App
  • Formatted: either <App> or <App> at App.vue.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Good observation, you're right! Also, I didn't think about the <App> at App.vue case.

There's one unfortunate side effect of this regex though: It can lead to polynomial build up when evaluating an input. Given that we run extractComponentName on user input I'd rather not risk the (still unlikely but possible) chance of some kind of ReDoS attack (I used this ReDoS checker to test it)

I played around with the regex to make it safer and keep it linear and arrived at this one: /^<([^\s]*)>( at [^\s]*)?$
I think this should still cover all component names but would appreciate it if you could double check it as well :)

Also, since this logic is getting a little bit more involved now, could you add a couple of unit test for findTrackComponent? It's fine to export the function just for testing purposes btw. Thanks!

(I don't want to overburden you with tests btw, I just want to keep stuff like this covered and understandable for everyone. So if you don't have the time, just let me know. We appreciate your hard work either way!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted, unit and e2e tests added.

});

return isMatched;
}

export const createTracingMixins = (options: TracingOptions): Mixins => {
const hooks = (options.hooks || [])
.concat(DEFAULT_HOOKS)
Expand Down Expand Up @@ -85,7 +99,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.indexOf(name) > -1
? findTrackComponent(options.trackComponents, name)
: options.trackComponents;

// We always want to track root component
Expand Down
Loading