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

Add BaseScopeManager interface #66

Merged

Conversation

vmarchaud
Copy link
Member

Based on #65

I mostly based the API on the Opencensus one, i will explain the choice i made for each function so we have a starting point for the discussion.

packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
packages/opentelemetry-context-base/src/types.ts Outdated Show resolved Hide resolved
@mayurkale22
Copy link
Member

@vmarchaud #65 has been merged, please rebase with the master.

@vmarchaud vmarchaud force-pushed the feat/add-context-interface branch 2 times, most recently from ac02295 to 4c08972 Compare July 4, 2019 13:02
@vmarchaud
Copy link
Member Author

I've changed the interface based on all feedbacks @mayurkale22 @rochdev @draffensperger

@vmarchaud vmarchaud force-pushed the feat/add-context-interface branch 3 times, most recently from 921366f to 28ba32a Compare July 8, 2019 16:06
@vmarchaud
Copy link
Member Author

Addressed comments by @rochdev. I've also renamed the packages to scope-base following comment by @rochdev saying that Context was already heavily used across the implementation so using scope will be clearer.

@vmarchaud vmarchaud changed the title Add ContextManager interface Add BaseScopeManager interface Jul 8, 2019
@codecov-io
Copy link

codecov-io commented Jul 8, 2019

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.03%. Comparing base (14310c3) to head (af3e99f).

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #66   +/-   ##
=======================================
  Coverage   98.03%   98.03%           
=======================================
  Files          13       13           
  Lines         204      204           
  Branches        8        8           
=======================================
  Hits          200      200           
  Misses          4        4           

@draffensperger
Copy link
Contributor

Given that this is just an interface for now, would it make sense to have it in @opentelemetry/types? What's the motivation for a separate package for the base scope?

I like the idea of having the actual implementations be in their own packages, but not sure the interface needs its own. @rochdev WDYT?

@vmarchaud
Copy link
Member Author

@draffensperger I believe the goal was the package to be usable outside of opentelemetry, so having the interface in core would mean add another dependency.

@rochdev
Copy link
Member

rochdev commented Jul 9, 2019

LGTM

@mayurkale22 @danielkhan is it fine to use Scope instead of Context from the spec perspective?

@mayurkale22
Copy link
Member

@mayurkale22 @danielkhan is it fine to use Scope instead of Context from the spec perspective?

I think it is fine to use Scope, looks like Java and .NET uses the same terminology https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/context/Scope.java

@vmarchaud vmarchaud force-pushed the feat/add-context-interface branch from 28ba32a to c0e1a10 Compare July 10, 2019 11:22
@vmarchaud
Copy link
Member Author

Addressed comments once again, i believe it's good to land now.
(@rochdev could you remove the requested changes ? i think it will block the PR otherwise)

@vmarchaud vmarchaud force-pushed the feat/add-context-interface branch from c0e1a10 to 6bc6c00 Compare July 11, 2019 09:05
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.

I would rename the first parameter of bind to target since it may not be an object but otherwise LGTM.

@mayurkale22
Copy link
Member

I would rename the first parameter of bind to target since it may not be an object but otherwise LGTM.

@vmarchaud Feel free to handle it in separate PR, merging it now. Thanks for the work and PR!

@mayurkale22 mayurkale22 merged commit 9d41576 into open-telemetry:master Jul 11, 2019
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
* Add context-base and context-asynchooks packages

* add BaseScopeManager interface open-telemetry#46
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
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