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

fix(vue tracing): Adjusted the startTransaction code location #6048

Closed
wants to merge 1 commit into from
Closed

fix(vue tracing): Adjusted the startTransaction code location #6048

wants to merge 1 commit into from

Conversation

ys-zhifu
Copy link

@ys-zhifu ys-zhifu commented Oct 26, 2022

Fix unexpected transaction name value of pageloadTransaction.

pageloadTransaction:If I modify the context in the beforeNavigate function Name, pageloadTransaction will be executed later SetName, which causes the name to be modified to to.name instead of the value I modified in beforeNavigate.

image

image

image

@lobsterkatie
Copy link
Member

@ys-zhifu Thanks for the contribution!

Can you please update the PR description to explain what problem you're solving and how your change solves it? (You can also remove the boilerplate.)

@ys-zhifu
Copy link
Author

@lobsterkatie Hi, I updated the description.

@Lms24
Copy link
Member

Lms24 commented Oct 27, 2022

Hi @ys-zhifu! I took a quick look at your PR and I would like to make a different change instead of the one you suggested. Here's why:

We moved the pageload transaction start to be earlier than before on purpose in #5983. The reasons for this are that first, a pageload transaction should always start as early as possible because it is meant to represent an entire pageload. Secondly, due to changes in how Vue router works in Vue 3, we missed a few child spans that started earlier than the pageload transaction started before the change.

Please correct me if I'm wrong but I think the actual problem you're facing is that your transaction name you set in beforeNavigate is overwritten here:

pageloadTransaction.setName(transactionName, transactionSource);

We can easily fix this by checking for the source of the transaction name. If it is custom, we'll simply not update it with the parameterized route. WDYT, does this solve your problem?

I'll quickly make this change and open up a PR for it. Thanks for bringing this to our attention!

Lms24 added a commit that referenced this pull request Oct 27, 2022
…ctions (#6060)

As brought to our attention in #6048, our pageload transaction start change (which makes the transaction start earlier) introduced in #5983 caused custom transaction names to be overwritten when `beforeNavigate` is used to update the transaction name. 

This patch fixes this problem by checking for the transaction source before (not) updating the current transaction name with the resolved route name once the router's `beforeEach` hook was called. Furthermore, it adds a test to cover this case. 

See #6048 (comment) for the motivation for creating this PR.
@Lms24
Copy link
Member

Lms24 commented Oct 27, 2022

Update: Opened and merged #6060. We'll release a new version with this fix soon. Closing this PR for now. Nevertheless, thanks for your efforts @ys-zhifu!

@Lms24 Lms24 closed this Oct 27, 2022
@ys-zhifu
Copy link
Author

@Lms24 Hi, i kown the reason.

When I use vuepress to build documents, I use Sentry monitoring. It is found that the transaction names are all the to.name of vueRouter, which leads to many different transaction names.

So I plan to adjust the transaction name in beforeNavigate to reduce the number.
I match to. path from vuerouter through to. name and pass it to context name. But when I actually use it, I find that the page transaction name will be modified. This value is not the value I set in beforeNavigate.

This is the data before adjustment
image

This is the data after adjustment
image

Finally, I found that I can reset the matcher of vuerouter to achieve the goal I want, that is, set the transaction name as to.path instead of to.name.

@ys-zhifu ys-zhifu deleted the fix-vue-tracing branch October 27, 2022 08:51
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