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(scope-manager): implements AsyncHooks Scope Manager #103

Merged
merged 7 commits into from Jul 25, 2019

Conversation

vmarchaud
Copy link
Member

@vmarchaud vmarchaud commented Jul 13, 2019

I'm waiting on #100 to be merged so i can add more tests (and coverage) and itirate on this PR.

Specific mention: i believe we are forced to cast to any in the bindFunction, if someone have an idea feel free to tell !

@vmarchaud vmarchaud force-pushed the asynchooks-scope-manager branch 2 times, most recently from 7bcccbd to 22b2f20 Compare July 13, 2019 17:38
@codecov-io
Copy link

codecov-io commented Jul 13, 2019

Codecov Report

Merging #103 into master will decrease coverage by 0.13%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
- Coverage   98.87%   98.74%   -0.14%     
==========================================
  Files          27       29       +2     
  Lines        1784     1915     +131     
  Branches      201      221      +20     
==========================================
+ Hits         1764     1891     +127     
- Misses         20       24       +4
Impacted Files Coverage Δ
...kages/opentelemetry-scope-async-hooks/src/index.ts 100% <100%> (ø)
...ry-scope-async-hooks/src/AsyncHooksScopeManager.ts 96.49% <96.49%> (ø)

@bterlson
Copy link

@vmarchaud something like this might work

@vmarchaud
Copy link
Member Author

@bterlson Thanks for looking into it, sadly it doesn't wort either because your signature missed the fact that bindFunction return T, see this one

@vmarchaud vmarchaud force-pushed the asynchooks-scope-manager branch 2 times, most recently from cc92e16 to 57e5b32 Compare July 16, 2019 18:40
@vmarchaud vmarchaud force-pushed the asynchooks-scope-manager branch 2 times, most recently from 83ad44f to 12f232b Compare July 23, 2019 10:15
@vmarchaud vmarchaud marked this pull request as ready for review July 24, 2019 12:42
@vmarchaud
Copy link
Member Author

Ready to be merged following discussion from SIG
cc @rochdev

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.

A few small nits, but then should be good to go 👍

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.

LGTM. Could use more tests that the scope is actually propagated properly with all the different asynchronous constructs, but these can be added later.

@mayurkale22 mayurkale22 merged commit 0b7e083 into open-telemetry:master Jul 25, 2019
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
…ry-3.x

Update dependency @types/jquery to v3.5.0
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

5 participants