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

chore: fixing running task in a proper zone preserving the active zone #88

Merged
merged 4 commits into from
Jun 24, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Jun 17, 2020

Which problem is this PR solving?

For full fix another PR is needed from sdk
open-telemetry/opentelemetry-js#1209

Short description of the changes

  • it run the task in active zone preserving the spawned zone instead of root zone

@obecny obecny self-assigned this Jun 17, 2020
@obecny obecny added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 17, 2020
@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

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

@@           Coverage Diff           @@
##           master      #88   +/-   ##
=======================================
  Coverage   94.51%   94.52%           
=======================================
  Files          62       62           
  Lines        3520     3522    +2     
  Branches      372      372           
=======================================
+ Hits         3327     3329    +2     
  Misses        193      193           
Impacted Files Coverage Δ
...try-plugin-user-interaction/src/userInteraction.ts 92.85% <0.00%> (+0.07%) ⬆️

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm

@obecny
Copy link
Member Author

obecny commented Jun 23, 2020

@open-telemetry/javascript-maintainers ^^

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.

I'm sorry I thought I had already reviewed this. I looked through the code but must have forgotten to click the approve button

}
return activeZone.run(() => {
try {
return plugin._tracer.withSpan(span as api.Span, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have to do Type Assertion here (span as api.Span)?

Copy link
Member

Choose a reason for hiding this comment

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

should not be required as the assertion on line 319 should remove undefined from the type

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a compiler error / issue. the internal _createSpan returns api.Span or undefined. Even though there is if check for span existence the compiler still complains. If you have a better idea how to change it I'm open for suggestion.

@obecny obecny requested review from dyladan and mayurkale22 June 24, 2020 19:37
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.

Looks good to me. We should have another PR to bump all otel deps to 0.9

@mayurkale22 mayurkale22 merged commit 5732ed8 into open-telemetry:master Jun 24, 2020
@owais
Copy link

owais commented Jun 30, 2020

Thank you for getting this fixed @obecny. Can we release a new version for this package any time soon?

@dyladan
Copy link
Member

dyladan commented Jun 30, 2020

Yes I'll try to cut this today or tomorrow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants