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

feat(ah-scope): add scope propagation for event emitters #146

Merged
merged 1 commit into from
Aug 16, 2019

Conversation

vmarchaud
Copy link
Member

Making a new PR for event emitters support with async hooks.
Link to initial PR issue with the current implementation: #103 (comment)

I was saying that i would expect this behavior:

let ee = new EventEmitter()
ee = scopeManager.bind(ee, { test: 1 })
ee.on('test', () => {
// i would expect that scopeManager.active() would be { test: 1 } here.
})

@rochdev is the implementation using the scope at the .on moment would have the same behavior ? I'm not sure.

Copy link
Contributor

@draffensperger draffensperger left a comment

Choose a reason for hiding this comment

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

Nice use of strong typing here 👍

@mayurkale22 mayurkale22 added this to the SDK complete milestone Jul 31, 2019
Copy link
Member

@rochdev rochdev left a comment

Choose a reason for hiding this comment

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

Left a few comments, but otherwise looks good 👍

@vmarchaud
Copy link
Member Author

@rochdev I addressed your comments !

@codecov-io
Copy link

codecov-io commented Aug 2, 2019

Codecov Report

Merging #146 into master will increase coverage by 0.9%.
The diff coverage is 98.54%.

@@            Coverage Diff            @@
##           master     #146     +/-   ##
=========================================
+ Coverage   93.32%   94.22%   +0.9%     
=========================================
  Files          47       47             
  Lines        1587     1732    +145     
  Branches       94      114     +20     
=========================================
+ Hits         1481     1632    +151     
+ Misses        106      100      -6
Impacted Files Coverage Δ
...pe-async-hooks/test/AsyncHooksScopeManager.test.ts 100% <100%> (ø) ⬆️
...ry-scope-async-hooks/src/AsyncHooksScopeManager.ts 97.67% <95.74%> (-2.33%) ⬇️
...telemetry-node-tracer/src/instrumentation/utils.ts 89.47% <0%> (-6.36%) ⬇️
...ckages/opentelemetry-core/src/common/NoopLogger.ts 14.28% <0%> (-5.72%) ⬇️
...kages/opentelemetry-basic-tracer/test/Span.test.ts 100% <0%> (ø) ⬆️
...s/opentelemetry-core/test/trace/tracestate.test.ts 100% <0%> (ø) ⬆️
...lemetry-core/test/trace/ProbabilitySampler.test.ts 100% <0%> (ø) ⬆️
...metry-node-tracer/src/instrumentation/constants.ts 100% <0%> (ø) ⬆️
.../opentelemetry-core/src/trace/spancontext-utils.ts 100% <0%> (ø) ⬆️
... and 20 more

@vmarchaud
Copy link
Member Author

I'm not sure why but the coverage sometimes ignore all the event emitters functions, here the output on my laptop:
image

@rochdev
Copy link
Member

rochdev commented Aug 5, 2019

I'm not sure why but the coverage sometimes ignore all the event emitters functions

Interesting. Do you get a different behavior by environment or even on your laptop it's sometimes different?

@vmarchaud
Copy link
Member Author

@rochdev Its varying even on my laptop. I didn't get that problem with the old PR thought (with the event emitters and promises directly inside).

@vmarchaud vmarchaud force-pushed the ah-scope-ee branch 5 times, most recently from cedad8b to 565230b Compare August 14, 2019 13:17
@vmarchaud
Copy link
Member Author

@rochdev if you could re-review please

@vmarchaud vmarchaud force-pushed the ah-scope-ee branch 2 times, most recently from c369818 to 598a1e3 Compare August 15, 2019 20:56
@mayurkale22 mayurkale22 merged commit 0941b7f into open-telemetry:master Aug 16, 2019
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
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.

6 participants