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): Finish spans in component tracking before starting new ones for same operation #5918

Merged
merged 1 commit into from Oct 10, 2022

Conversation

lforst
Copy link
Member

@lforst lforst commented Oct 10, 2022

Attempts to fix #5693

We suspect that Vue does not always call cleanup hooks (e.g. updated, created, unmounted) in mixins. This would currently have the effect that we clobber old spans with new spans on hooks like beforeUpdated and never finish the spans.

This PR will simply finish unfinished spans for a particular lifecycle operation when they're not finished/cleaned up yet.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

We really need some vue tests 😭

In my eyes we should be storing the span using span ids when tracking, then we never have to worry about operations colliding with each other.

@lforst
Copy link
Member Author

lforst commented Oct 10, 2022

In my eyes we should be storing the span using span ids when tracking, then we never have to worry about operations colliding with each other.

Hm, but then we lose the opportunity to close old spans. There is a semantic difference and the question is what do we want operations that don't get cleaned up to look like?

Like this?

Screen Shot 2022-10-10 at 11 13 24

or like this?

Screen Shot 2022-10-10 at 11 13 36

My guess is that if they're not cleaned up, they likely got cancelled, in which case we should probably go for the first model (current) implementation.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

My guess is that if they're not cleaned up, they likely got cancelled, in which case we should probably go for the first model (current) implementation.

Yeah you know what, you're correct. Let's do this instead. I just read through the code and I was just misunderstanding some stuff.

@lforst lforst merged commit 516eba5 into master Oct 10, 2022
@lforst lforst deleted the lforst-vue-component-tracking-fix branch October 10, 2022 11:04
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.

Sentry fails to track Vue components and produces "deadline_exceeded" transactions
3 participants