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

[Plugin User Interaction]: Improve causality of spans from bubbled events #172

Merged
merged 4 commits into from
Aug 18, 2020
Merged

Conversation

johnbley
Copy link
Member

Which problem is this PR solving?

  • Currently when bubbled events trigger multiple listeners, we create separate spans/traces for each of them, even though they were causally related. It also makes it hard to see whether multiple "clicks" happened or just one.

Short description of the changes

  • Track event bubbling and assign parent span relationship (lowest traced handler on event bubble gets top-most span, other higher-element handlers are tracked as child spans recursively).

@johnbley johnbley requested a review from a team August 10, 2020 15:16
@codecov
Copy link

codecov bot commented Aug 10, 2020

Codecov Report

Merging #172 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #172   +/-   ##
=======================================
  Coverage   94.30%   94.31%           
=======================================
  Files          77       77           
  Lines        3830     3836    +6     
  Branches      413      415    +2     
=======================================
+ Hits         3612     3618    +6     
  Misses        218      218           
Impacted Files Coverage Δ
...try-plugin-user-interaction/src/userInteraction.ts 93.95% <0.00%> (+0.14%) ⬆️

@dyladan dyladan changed the title Improve causality of spans from bubbled events [Plugin User Interaction]: Improve causality of spans from bubbled events Aug 10, 2020
@dyladan dyladan added the enhancement New feature or request label Aug 10, 2020
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Seems fine to me by I'm not an expert on this plugin. Would like to get @obecny feedback.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

overall lgtm, I just would like to ask you to update the image images/main-sync.jpg to show how the trace will look like now after this change.

@johnbley
Copy link
Member Author

overall lgtm, I just would like to ask you to update the image images/main-sync.jpg to show how the trace will look like now after this change.

There's no difference in the examples because of this change - this only affects scenarios where event listeners on bubbling events are added at multiple levels of the DOM tree, which the examples don't do.

@obecny obecny merged commit 8c68c38 into open-telemetry:master Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants