Adding Context API

[codeboten]

This change implements the Context API portion of OTEP #66. The CorrelationContext API and Propagation API changes will come in future PRs. We're leveraging entrypoints to support other implementations of the Context API if/when necessary. For backwards compatibility, this change uses aiocontextvars for Python versions older than 3.7.

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

[codecov-io]

Codecov Report

Merging #395 into master will increase coverage by 1.71%.
The diff coverage is 93.02%.

@@            Coverage Diff            @@
##           master    #395      +/-   ##
=========================================
+ Coverage   86.58%   88.3%   +1.71%     
=========================================
  Files          39      42       +3     
  Lines        1998    2060      +62     
  Branches      231     236       +5     
=========================================
+ Hits         1730    1819      +89     
+ Misses        200     170      -30     
- Partials       68      71       +3

Impacted Files	Coverage Δ
...telemetry-api/src/opentelemetry/context/context.py	100% <ø> (ø)	⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	91.2% <ø> (+0.43%)	⬆️
...elemetry-api/src/opentelemetry/metrics/__init__.py	95.58% <100%> (+7.45%)	⬆️
...k/src/opentelemetry/sdk/metrics/export/__init__.py	100% <100%> (ø)	⬆️
...etry-sdk/src/opentelemetry/sdk/metrics/__init__.py	98.46% <100%> (-0.6%)	⬇️
.../src/opentelemetry/sdk/metrics/export/aggregate.py	100% <100%> (ø)
...y-api/src/opentelemetry/context/default_context.py	100% <100%> (+15%)	⬆️
...c/opentelemetry/sdk/context/contextvars_context.py	88.23% <100%> (+28.71%)	⬆️
...xt-zipkin/src/opentelemetry/ext/zipkin/__init__.py	84.41% <100%> (ø)	⬆️
...c/opentelemetry/sdk/context/threadlocal_context.py	100% <100%> (+31.25%)	⬆️
... and 8 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 f29111a...1f9bd76. Read the comment docs.

[Oberon00]

I'd prefer writing these methods in the form

if context is None:
  context = get_current()
return some_action(context)

or even return some_action(context or get_current()).

EDIT: is (not) None is fine

[Oberon00]

If this is supposed to represent the current context, then we cant just use a global variable. We need a context-local variable (or at least a thread local variable). But this is already what the context inside does, so it seems we have mixed up "current Context" and "configured context manager".

[codeboten]

Maybe my interpretation of what set_current is intended to achieve is wrong. My understanding is that set_current is used here to update the global context which can then be referenced other places in the application with updated values rather than updating the context within a context manager. This I think can be achieved with the global here, though I might be missing something.

[ocelotl]

@Oberon00 , what do you mean with "configured context manager"?

[Oberon00]

I think we have to distinguish four things:

1. A single context object, which is basically just a String -> Object dictionary. There is only at most one value for each key in a particular context. I think we are currently missing that concept.
2. The current value of a particular context variable/key in some context (what get_value returns).
3. The context manager, which is responsible for determining what the "current" context is, creating a new context object based on the parent context if a new logical execution context is entered / get_current is called in a logical execution context where no context object exists yet, etc.. This is what we currently call (IMHO confusingly) "Context".
4. The logical execution context, an abstract concept, not a class or object. E.g. a thread of execution, the context of a async task, a greenlet, etc. The context manager defines what an execution context is and ensures that there is a 1:1 mapping from logical execution contexts to context objects.

EDIT: set_context should then set the context object in the current logical execution context.

EDIT2: As a concrete example, consider setting a value in a context object and then passing it (e.g. via a queue) to another, already running, thread. In that case, the other thread must be able to read that value back. This is not currently the case, as our "Context" is in fact a context manager and will return the return value of get_value(key, context) is potentially different when called from different logical execution contexts, even for identical context arguments.

EDIT3: I asked the spec folks: open-telemetry/opentelemetry-specification#424 (comment)

[codeboten]

Some clarifications:

1. A single context object, which is basically just a String -> Object dictionary. There is only at most one value for each key in a particular context. I think we are currently missing that concept.

This is currently represented by the Context interface which is implemented by either ContextVarsContext or ThreadLocalContext depending on the version of Python

3. The context manager, which is responsible for determining what the "current" context is, creating a new context object based on the parent context if a new logical execution context is entered / get_current is called in a logical execution context where no context object exists yet, etc.. This is what we currently call (IMHO confusingly) "Context".

This is implemented via the get_current & set_current methods

[ocelotl]

I was about to comment the same. Regarding point 1, here is the class that represents the concept of a single context object. It is implemented here, here and here.

Regarding your first EDIT, there is nothing named set_context. Did you mean set_current?

[Oberon00]

@codeboten @ocelotl: No, ContextVarsContext and ThradLocalContext are not a contexts in the OTEP 66 sense. E.g. ThreadLocalContext is a mapping (thread, key) -> value instead of just key -> value. That's what I wanted to clarify in my EDIT2.

For example, in the OTEP, Propagator.extract returns a context. Which return type would you use given the types in this PR, so that you can give the context to another thread or execution context and it still keeps its values?

[c24t]

I think @Oberon00 has a good point here, and this will not work as expected.

Using his context queue example:

from queue import Queue
import asyncio

from opentelemetry import context
from opentelemetry.sdk.context.contextvars_context import ContextVarsContext

context.set_current(ContextVarsContext())

q = Queue()

async def main():
    q.put(context.get_current().copy())
    q.put(context.get_current().copy())
    asyncio.gather(work(1), work(2))

async def work(val):
    context.set_current(q.get_nowait())
    context.get_current().set_value('k', val)
    # At this point context.get_current() should be the one we popped from the
    # queue, and current_context.get_value('k') should be `val`

await main()

This works as expected, but only incidentally. If the call to set_current happens in one task in between the set_current and get_current calls in another, we get some weird behavior. Running effectively the same code, but manipulating the call order with an Event shows this:

from queue import Queue
from threading import Event
import asyncio

from opentelemetry import context
from opentelemetry.sdk.context.contextvars_context import ContextVarsContext

context.set_current(ContextVarsContext())

# Helper to print Contexts
def tos(context):
    return "<Context at {}: {}>".format(
        hex(id(context)),
        ','.join(["{}={}".format(k, v.get(None))
                  for k, v in context._contextvars.items()])
    )

q = Queue()
run1 = Event()
run2 = Event()

async def main():
    q.put(context.get_current().copy())
    q.put(context.get_current().copy())
    asyncio.gather(a1(), a2())

async def a1():
    # (Step 1)
    context.set_current(q.get_nowait())
    context.get_current().set_value('k', 1)

    # Expected context is current, k=1
    print("[task 1] {}".format(tos(context.get_current())))

    # (GOTO 2)
    run2.set(); await asyncio.sleep(0); run1.wait();

    # (Step 3)
    # Current context has changed AND k=None because the call to `set` k=2
    # happened in the other task!
    print("[task 1] {}".format(tos(context.get_current())))


async def a2():
    run2.wait()

    # (Step 2)
    context.set_current(q.get_nowait())
    context.get_current().set_value('k', 2)

    # Expected context is current, k=2
    print("[task 2] {}".format(tos(context.get_current())))

    # (GOTO 3)
    run1.set(); await asyncio.sleep(0);

await main()

This prints:

[task 1] <Context at 0x110349dd0: k=1>
[task 2] <Context at 0x110349250: k=2>
[task 1] <Context at 0x110349250: k=None>

Task 1 gets the context from the set_current call in task 2, but doesn't get its context variable values because set calls are scoped to the task! You could show roughly the same thing with threads and threadlocals.

(Sorry for the long example here)

[Oberon00]

@ocelotl Yes, sorry I meant set_current.

[codeboten]

Thank for the clarifications. I've updated the Context class to be an immutable object. The portion of the Context class that was used to wrap threadlocal or contextvars implementation details is now RuntimeContext.

I still need to update the PR to not use a global _CONTEXT variable, but I think this change is at least a step in the right direction.

[Oberon00]

Requesting changes mainly because of #395 (comment)

[mauriciovasquezbernal]

Some very small nits I found around. I think we are close to get this finished.

I just want to notice that the current PR is changing the way contextvars and thread locals are used. Before this PR, a contextvar / threadlocal object was used to save each element in the context, for instance the current span were saved in its own contextvar / threadlocal. This PR is using a single contextvar / threadlocal to save the whole context object.

I'm a little bit afraid of the overhead of this solution, for instance setting the active span now requires to create a new context (copying all the elements of this) and then set it as current. However this is not a blocking comment, probably we could improve this later on if we really find it is a problem.

[mauriciovasquezbernal]

I'm not totally sure this is the right place for this test. It is not strictly related with context, probably should be moved to the tracer tests.

[mauriciovasquezbernal]

I think it's good to go, otherwise we'll remain blocked forever in this one.

[c24t]

Tests look good, switching to an immutable Context looks like it solves the problem of values leaking between tasks, and loading the context class via entry_points looks like a great idea. No more substantial comments from me, this LGTM to merge.

I don't know about the utility of having separate API and SDK classes. Is there a use case where we'd rather have a DefaultRuntimeContext than a ContextVarsRuntimeContext? I don't think the arguments for no-op Tracer and Meter apply the same way to the context.

One thing I'd still like to see here in a comment or PR description for documentation, because it's not clear to me even after several reviews:

• what does this do that the old AsyncRuntimeContext couldn't (and vice versa)?
• what does this do that bare ContextVars couldn't (and vice versa)?

[c24t]

If the alias is to avoid shadowing with Span.context below, maybe now is a good time to rename it Span.span_context.

[codeboten]

I'd like to propose making this change along with the Propagation API change, which will update this code anyways. #415

[c24t]

Do we ever expect to have more than one TracerSource? I expected TracerSource to be a singleton, and all tracers to share the same current span .

[codeboten]

My takeaway from the SIG meeting this morning was that we expect a single TracerSource, so this change and the comment above could be removed if that's the case.

[codeboten]

I don't know about the utility of having separate API and SDK classes. Is there a use case where we'd rather have a DefaultRuntimeContext than a ContextVarsRuntimeContext? I don't think the arguments for no-op Tracer and Meter apply the same way to the context.

I'm in agreement here, having the ContextVarsRuntimeContext in the SDK may not be useful. Having the RuntimeContext interface be available and configurable through entry_points gives us enough flexibility to support alternative implementations in the future. That being said, I would vote in favour of moving ContextVarsRuntimeContext and ThreadLocalRuntimeContext into the api, to reduce the amount of work users need to do in order to get a useful implementation. This could be revisited once we have a mechanism for simplifying configuration.

One thing I'd still like to see here in a comment or PR description for documentation, because it's not clear to me even after several reviews:

* what does this do that the old `AsyncRuntimeContext` couldn't (and vice versa)?

This implementation gives us the flexibility of supporting additional RuntimeContext without additional changes in the project. Third parties could provide a new implementation for RuntimeContext and make it available through entry_points and environment variable configuration without modifying the api or sdk. This would not have been the case previously.

* what does this do that bare `ContextVars` couldn't (and vice versa)?

If we did not plan on supporting Python 3.4, the Context API could potentially have leveraged ContextVars directly for all cases. Though I suppose this would have prevented us from supporting certain concurrency implementations (ie. older versions of tornado.concurrent).