Adding attach/detach methods as per spec

[codeboten]

This change updates the Context API with the following:

• removes the remove_value method
• removes the set_current method
• adds attach and detach methods

Fixes #420

Signed-off-by: Alex Boten aboten@lightstep.com

[codecov-io]

Codecov Report

Merging #429 into master will increase coverage by 0.37%.
The diff coverage is 92.15%.

@@            Coverage Diff             @@
##           master     #429      +/-   ##
==========================================
+ Coverage   88.36%   88.73%   +0.37%     
==========================================
  Files          41       41              
  Lines        2080     2060      -20     
  Branches      238      237       -1     
==========================================
- Hits         1838     1828      -10     
+ Misses        167      160       -7     
+ Partials       75       72       -3

Impacted Files	Coverage Δ
opentelemetry-api/src/opentelemetry/util/loader.py	81.25% <ø> (ø)	⬆️
...try-ext-wsgi/src/opentelemetry/ext/wsgi/version.py	100% <100%> (ø)	⬆️
...ntelemetry-api/src/opentelemetry/trace/__init__.py	85.62% <100%> (+1.06%)	⬆️
...-ext-flask/src/opentelemetry/ext/flask/__init__.py	88.88% <100%> (ø)	⬆️
...telemetry-api/src/opentelemetry/context/context.py	100% <100%> (ø)	⬆️
...i/src/opentelemetry/context/contextvars_context.py	83.33% <100%> (-5.56%)	⬇️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	91.11% <100%> (+0.3%)	⬆️
.../src/opentelemetry/ext/opentracing_shim/version.py	100% <100%> (ø)	⬆️
...src/opentelemetry/ext/opentracing_shim/__init__.py	95.76% <100%> (-0.18%)	⬇️
...i/src/opentelemetry/context/threadlocal_context.py	100% <100%> (ø)	⬆️
... and 21 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 674658a...ae5d8f6. Read the comment docs.

[mauriciovasquezbernal]

I have a blocking comment on the token implementation for thread local.

I also see that set_current()/get_current() was renamed to attach()/detach() only in the context module, would it make sense to also rename it on the RuntimeContext and derived classes?

[codeboten]

I don't feel strongly one way or another regarding re-naming set_current()/get_current(), the implementation of the RuntimeContext is decoupled from the API so either way works for me. Happy to change it if people feel strongly about it.

[mauriciovasquezbernal]

LGTM, a couple of non blocking comments.

[ocelotl]

Is this class really necessary? I think we can just use Context instead of wrapping a Context object inside a "token" one.

[toumorokoshi]

I agree philosophically, although there's probably something in the spec that states that the token should not be the context. Although I don't see anything stating that now: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context.md#attach-context

[codeboten]

The reasoning for returning a token rather than a context was discussed in this pull request: open-telemetry/opentelemetry-specification#424 (comment)

There are few reasons:
Checking correctness - by having a restore that accepts a Token or just simply the previous Context you can check if the given value is the expected one (in other words you can see the calls to set as push to a stack and restore as pop where you check that the element you intend to pop is the expected one. This is critical and I've seen cases where auth tokens were leaked because of this issue (I don't think I can share too many things about this issue, but the root cause were people calling set without a paired restore).
This approach guarantees (at least tries to enforce because mistakes can still happen) that the Context is not "modified" by calling a function:

[ocelotl]

If so, then at least the underscore should be in self._context and not in _Token. We are returning an object, that makes its class part of a public API.

[codeboten]

Updated the class name

[toumorokoshi]

Thanks! The code looks good from a functional perspective. I'd say there's good feedback around ensuring the context objects themselves match the attach / detach method names, and using decorators or somehow calling out methods that need the context loaded first.

[toumorokoshi]

I agree philosophically, although there's probably something in the spec that states that the token should not be the context. Although I don't see anything stating that now: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context.md#attach-context

[toumorokoshi]

could we create a base class of the context tests, which we can extend and set the context constructor? seems like a great way to ensure standard behavior across context implementations.

[codeboten]

done

[ocelotl]

If so, then at least the underscore should be in self._context and not in _Token. We are returning an object, that makes its class part of a public API.

[c24t]

+1 to @toumorokoshi's comment that we should have some shared tests that run for all implementations of RuntimeContext. If we use entry_points this would even let users test their own implementations without changing any code.

Not a blocking comment, but I think it would make for a better API if context.set_value also called attach under the hood and returned a token, since AFAICT the only time you'd ever call set_value you would attach the new context immediately, as e.g. context.attach(context.set_value(...)). This is what contextvars does too.

[c24t]

It might be worth checking that we can detach contexts out of order too, maybe in a separate test_detach test:

In [2]: from opentelemetry import context

In [3]: t1 = context.attach(context.set_value('c', 1)); context.get_current()
Out[3]: {'c': 1}

In [4]: t2 = context.attach(context.set_value('c', 2)); context.get_current()
Out[4]: {'c': 2}

In [5]: context.detach(t1); context.get_current()
Out[5]: {}

In [6]: context.detach(t2); context.get_current()
Out[6]: {'c': 1}

I was surprised to see contextvars works this way. One difference between contextvars and threadlocals is that contextvars will raise if you try to reset using the same token twice. I don't know whether it's worth enforcing this behavoir for threadlocals though.

[codeboten]

test added. Didn't know about the reset behaviour either, good thinng to know!

[c24t]

Did you try making the token type a TypeVar? Just curious, I don't know that it's worth the extra typing boilerplate to do so.

[toumorokoshi]

LGTM! Thanks for addressing those changes.