[WIP] Context Prop #325

codeboten · 2019-12-10T17:53:31Z

This is a continuation of the work started in #278:

This is a PR to start implementing the context-propagation-spec as outlined in the following documents:

We could break up the pieces into more consumable pull requests with issues and tests for each. What I'd like to get out of this PR though is for folks to review the code examples below to understand if the otep is implemented as it should be. I'd like to use the feedback here to also recommend any changes that need to happen in the otep.

TODO:

~~support merging context~~
review correlationcontext implementation
~~update branch from master, this includes the changes for named tracers~~
add tests for new functionality
review documentation for all updated code

Global initialization

def configure_opentelemetry(flask_app: flask.Flask):
    trace.set_preferred_tracer_implementation(lambda T: Tracer())

    # Global initialization
    (baggage_extractor, baggage_injector) = correlation_context.http_propagator()
    (b3_extractor, b3_injector) = b3_format.http_propagator()
    propagation.set_http_extractors([b3_extractor, baggage_extractor])
    propagation.set_http_injectors([b3_injector, baggage_injector])

Extracting and injecting from HTTP headers

@app.route("/")
def handle_request(ctx):
    extractors = propagation.http_extractors()
    with propagation.extract(request.headers, context=ctx, extractors=extractors) as ctx1:
        with tracer.start_as_current_span("service-span", context=ctx1) as ctx2:
            with tracer.start_as_current_span("external-req-span", context=ctx2) as ctx3:
                headers = {"Accept": "application/json"}
                version = CorrelationContextManager.value("version", context=ctx3)
                if version == "2.0":
                    return fetch_from_service_c(ctx3)
                return fetch_from_service_b(ctx3)

def fetch_from_service_b(ctx) -> str:
    # Inject the contexts to be propagated. Note that there is no direct
    # reference to tracing or baggage.
    headers = {"Accept": "application/json"}
		injectors = propagation.http_injectors()
    propagation.inject(headers, context=ctx, injectors=injectors)
    resp = requests.get("https://opentelemetry.io", headers=headers)
    return resp.text

Simpler API with context prop using current context

@app.route("/")
def handle_request():
    with propagation.extract(request.headers):
        with tracer.start_as_current_span("service-span"):
            with tracer.start_as_current_span("external-req-span"):
                version = CorrelationContextManager.value("version")
                if version == "2.0":
                    return fetch_from_service_c()

                return fetch_from_service_b()

def fetch_from_service_b() -> str:
    # Inject the contexts to be propagated. Note that there is no direct
    # reference to tracing or baggage.
    headers = {"Accept": "application/json"}
    propagation.inject(headers)
    resp = requests.get("https://opentelemetry.io", headers=headers)
    return resp.text

Implementing a propagator

class B3Extractor(HTTPExtractor):
    @classmethod
    def extract(cls, context, carrier, get_from_carrier=_getter):
			sc = trace.SpanContext(
          trace_id=int(trace_id, 16),
          span_id=int(span_id, 16),
          trace_options=trace.TraceOptions(options),
          trace_state=trace.TraceState(),
      )
      return Context.set_value(context, ContextKeys.span_context_key(), sc)

class B3Injector(HTTPInjector):
    @classmethod
    def inject(cls, context, carrier, set_in_carrier=_setter):
        sc = Context.value(context, trace.propagation.ContextKeys.span_context_key())
        sampled = (trace.TraceOptions.SAMPLED & sc.trace_options) != 0
        set_in_carrier(carrier, TRACE_ID_KEY, format_trace_id(sc.trace_id))
        set_in_carrier(carrier, SPAN_ID_KEY, format_span_id(sc.span_id))
        set_in_carrier(carrier, SAMPLED_KEY, "1" if sampled else "0")

Implementing a concern

    def start_span(...):
        if parent is Tracer.CURRENT_SPAN:
            parent = self.get_current_span()

        parent_context = parent
        if isinstance(parent_context, trace_api.Span):
            parent_context = parent.get_context()
        
				if parent_context is None:
            parent_context = Context.value(
              Context.current(),
              trace.propagation.ContextKeys.span_context_key()
            )
        ...
        sc = trace_api.SpanContext(
            parent_context.trace_id,
            generate_span_id(),
            parent_context.trace_options,
            parent_context.trace_state
        )
        span = Span(name=name, context=sc)
        Context.set_value(Context.current(), ContextKeys.span_context_key(), sc)

The scope of current context

def do_something():
    Context.set_value(empty_ctx, "say-something", "foo")
  
def scope_things():
    empty_ctx = Context.current()
    Context.set_value("say-something", "foo")
    second_ctx = Context.current()
    print(Context.value("say-something"))  # prints "foo"

    do_something()

    third_ctx = Context.current()
    print(Context.value("say-something"))  # prints "bar"

    print(empty_ctx.get("say-something"))  # prints ""
    print(second_ctx.get("say-something"))  # prints "foo"
    print(third_ctx.get("say-something"))  # prints "bar"

Referencing multiple contexts

merged_ctx = merge_context_correlation(Context.current(), other_ctx)
Context.set_current(merged_ctx)

Falling back to explicit contexts

# Most of the time, the extract function operates on the current context.
extract(headers)

# When a context needs to be extracted without changing the current 
# context, fall back to the explicit API.
extract(headers, context=other_context)

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

toumorokoshi

I've left comments around a couple things, but overall I think the implementation matches up a lot of what exists in the spec (or will).

I'm going to call out the main topics for discussion, and I don't see this PR addressing them completely yet:

Named Tracers and Spans from Extractor

As I mentioned in the comments, right now named tracers namespace their active spans, while extractor sets it on the unnamed tracer. And there's no guarantee that the unnamed tracer will be what people use. Theoretically it shouldn't be for integrations (e.g. wsgi and requests), which will probably be the ones responsible for extracting / injecting values into and from the current span.

Here's my thoughts:

Create a new context that merges the extracted values with the existing context
Start a new span with the information from the context object. This should work fine with create_span using the current span as the parent.

I'd argue here that, to make things make sense, the named tracer would still need to write to the same context key that everything else does. This mean that practically, the "name" of the tracer is manifested as a key that is included in the SpanContext, to be processed. It would not be possible to have multiple active spans, one per tracer instance. One could still have multiple active spans by virtue of multiple active contexts.

Context propagating across thread boundaries

This can be addressed separately, but I don't believe there's anything in the code that ensures that spawned threads will always use a context copied from the context of the thread they originated from. Although I think it only affects complex scenarios such a traced request spawning worker threads that live past the existing thread / span.

I'm noting request changes primarily to highlight a few of the areas I'm concerned about. But overall I think it's a huge step forward!

toumorokoshi · 2019-12-15T21:46:27Z

examples/opentelemetry-example-app/src/opentelemetry_example_app/context_propagation_example.py

+from opentelemetry.baggage import BaggageManager
+
+
+def configure_opentelemetry(flask_app: flask.Flask):


this whole function looks good to me.

toumorokoshi · 2019-12-15T21:47:53Z

examples/opentelemetry-example-app/src/opentelemetry_example_app/context_propagation_example.py

+def hello():
+    tracer = trace.tracer()
+    tracer.add_span_processor(BatchExportSpanProcessor(ConsoleSpanExporter()))
+    with propagation.extract(request.headers):


It looks like this differs from the example you put in the description. Is there one that you preferred?

In my opinion, this method looks great, and reduces a lot of boilerplate in comparison with the other example posted.

right, this is the same as the "Simpler API" example for anyone that doesn't need to use the context explicitly. As I suspect this would be the more common case, I like how much simpler it is.

toumorokoshi · 2019-12-15T21:50:39Z

opentelemetry-api/src/opentelemetry/distributedcontext/__init__.py

@@ -78,48 +78,64 @@ def __init__(
        self.value = value


-class DistributedContext:
+class CorrelationContext:


I don't know if you saw the latest memo in the context-propagation chat room, but @tedsuo is planning on modifying the OTEP to only include CorrelationContext, which baggage will utilize.

So could remove the baggage module now.

toumorokoshi · 2019-12-15T21:56:23Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -319,7 +323,7 @@ def __init__(

    def get_current_span(self):


should we support methods like these, instead of making it static?

Thinking about it a little bit, this might be necessary because we have named tracers?

Generally the spec calls for making methods static that manipulate the Context. I'm thinking through the ramifications but I do you have any thoughts on the Tracer object and it's tendency to abstract interfacing with the context?

Ultimately we will have to change that interface, because extractor / injectors work with the context rather than the tracer.

there's been talk of storing both current span and the extracted span context in the Context. This means this interface will need to change anyways

Gitter conversation: https://gitter.im/open-telemetry/language-agnostic-wg?at=5df8127155d9392300228e87

toumorokoshi · 2019-12-15T22:06:28Z

opentelemetry-api/src/opentelemetry/trace/propagation/context/__init__.py

+def with_span_context(
+    ctx: BaseRuntimeContext, span_context: SpanContext
+) -> BaseRuntimeContext:
+    return Context.set_value(


This is where I think there'll be an issue with named tracers. The named tracer object assumes that it will set the context at a specific slot, while this method uses the Context.span_context_key(), which probably doesn't align with whatever slot the named tracer has taken.

So unless the main Tracer matches the slot named defined in span_context_key, you will be manipulating a span that will have no impact on the tracer object.

Yes, based on the spec meeting this morning, it sounds like there's still some discussions as to the direction of named tracers. Not sure what the right thing to do is here.

toumorokoshi · 2019-12-15T22:11:52Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+            ctx = Context()
+            cls.set_current(ctx)
+        snapshot = Context()
+        snapshot.contents = copy.deepcopy(


this I believe does ensure the desired behavior specified in the spec, but deepcopies can get very expensive.

I know the Go implementation does a copy-on-write implementation. Another alternative I could think of is a parented context, that reached into the parent if the child doesn't have the key in question.

Thoughts on any of that? I want to outline the problems and I'll try to come back with something more valuable later :)

+1 on deepcopies being too expensive, will take a look at the go implementation.

toumorokoshi · 2019-12-15T22:38:21Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+        return cls.current()
+
+    @classmethod
+    def current(cls) -> "Context":


I'm not sure this is the right method name. It does quite a bit more than just get to the current context, as it creates a copy and returns that, without replacing the existing context.

Yeah, something like snapshot might be more accurate here.

+1 for snapshot.

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

codeboten · 2020-01-15T16:42:57Z

opentelemetry-api/src/opentelemetry/propagation/__init__.py

+        extractors = get_http_extractors()
+
+    for extractor in extractors:
+        # TODO: improve this


add try/except. return on the first successful extraction

What about if there are contexts for multiple concerns on the same carrier?
I think this should loop through all the extractors logging an error/warning when one fails.

codeboten · 2020-01-15T17:05:35Z

opentelemetry-sdk/tests/context/test_context.py

+                "test_span4",
+                "test_span5",
+                "futures_test",
+            ],


add test to check if parent is futures_test for each test_span

c24t

I think it should be possible to simplify the context API. Naively, I'd expect this API to be similar to the java prototype but wrapping ContextVar instead of io.grpc.Context.

The existing *RuntimeContext classes are complicated in part because they were meant to expose a nice dict-like interface to the user. If we're hiding them behind Context we probably don't want to keep these classes as they are now.

I only looked at the context API here, more reviews to come.

c24t · 2020-01-16T02:19:01Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+        return self.snapshot.get(key)
+
+    @classmethod
+    def value(


What's the benefit of making these static if we're passing the context instance as an arg anyway? Calling this as context.value('key') seems better than Context.value('key', context).

On the other hand, if you expect most calls not to include the context, context_module.current().value('key') is more complicated than Context.value('key'), but in that case I'd argue for adding a convenience method.

There is also context.get('key') 🤔

c24t · 2020-01-16T03:13:38Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+            _CONTEXT[key] = kwargs[key]
+        yield
+        for key in kwargs:
+            _CONTEXT[key] = snapshot[key]


Does it matter that this wouldn't delete keys set inside the block?

Also if a non referenced slot in kwargs is changed, it won' t be restored.

from opentelemetry.context import Context Context.set_value('a', 'xxx') Context.set_value('b', 'yyy') print(Context.current().snapshot) with Context.use(a='foo'): print(Context.current().snapshot) Context.set_value('a', 'i_want_to_mess_it_but_wont_work') Context.set_value('b', 'i_want_to_mess_it') print(Context.current().snapshot)

Prints:

{'a': 'xxx', 'b': 'yyy'} {'a': 'foo', 'b': 'yyy'} {'a': 'xxx', 'b': 'i_want_to_mess_it'}

c24t · 2020-01-16T03:18:51Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+    _CONTEXT = ThreadLocalRuntimeContext()
+
+
+class Context:


This may be one too many layers of context API onion.

As I understand it, the point of adding this module in #57 was largely the same as in this PR: to provide a single global context, and hide access to the context behind an API so we could swap out the implementation as needed.

If we didn't have to worry about the contextvars package missing (or exotic 3p threading models, about which more later), this could have reasonably been written without aContext object at all. In that case opentelemetry.context would provide two methods:

get_current() -> ContextVar[Dict[str, object]]

set_current(context: ContextVar[Dict[str, object]]) -> None

or, hiding the contextvar:

get_current() -> Dict[str, object]

set_current(context: Dict[str, object]) -> None

As compared to this PR, what would we lose with this simpler approach?

What would we lose if AsyncRuntimeContext and ThreadLocalRuntimeContext implemented Context, instead of Context being composed of one of these?

c24t · 2020-01-16T03:35:25Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+
+class Context:
+    def __init__(self) -> None:
+        self.snapshot = _CONTEXT.snapshot()


As I understood it, the point of using threadlocals and ContextVars was to avoid this kind of manual context management. If the context is backed by a threadlocal we should get a new copy for each thread we spawn, if it's backed by a ContextVar we should get a new copy for each new async context. When would we need to copy this ourselves?

To make that Context object immutable?

Correct, a new copy would be made available within a new thread or async context. But as @mauriciovasquezbernal's reply says, the spec suggests in the scope example: https://github.com/open-telemetry/oteps/blob/master/text/0066-separate-context-propagation.md#the-scope-of-current-context get_current returns an immutable Context.

c24t · 2020-01-16T03:36:35Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+    @classmethod
+    @contextmanager
+    def use(cls, **kwargs: typing.Dict[str, object]) -> typing.Iterator[None]:
+        snapshot = {key: _CONTEXT[key] for key in kwargs}


This gets a lot simpler using a single ContextVar to store the context:

def use(cls, **kwargs: typing.Dict[str, object]) -> typing.Iterator[None]: token = cv.set(kwargs) try: yield finally: cv.reset(token)

which may be one argument for letting AsyncRuntimeContext implement Context.

mauriciovasquezbernal

I'm submitting a partial review, I haven't reviewed all the code yet.
So far it looks quite good to me!

mauriciovasquezbernal · 2020-01-17T16:32:15Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+
+class Context:
+    def __init__(self) -> None:
+        self.snapshot = _CONTEXT.snapshot()


To make that Context object immutable?

mauriciovasquezbernal · 2020-01-17T16:37:38Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+        return self.snapshot.get(key)
+
+    @classmethod
+    def value(


There is also context.get('key') 🤔

mauriciovasquezbernal · 2020-01-17T16:50:15Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+        return None
+
+    @classmethod
+    def set_value(cls, key: str, value: "object") -> "Context":


I see that there is a discrepancy between value() and set_value().
Why there is a context argument in value but is is missing in set_value?.

What about if you want to set a value on a context different than the current one?, I think the following two options should be allowed:

# provide explicit context ctx1 = it_does_matter_where_i_get_it_from ctx2 = Context.set_value("name", "mauricio", ctx1) # work with current context ctx3 = Context.set_value("name", "mauricio")

mauriciovasquezbernal · 2020-01-17T17:23:46Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+
+        return call_with_current_context
+
+    def apply(self, ctx: "Context") -> None:


If Context is immutable this method should not exist, perhaps it should be a class method intended to be used on the current context?

Would it make sense to have a non-class method def merge(self, ctx: "Context") -> "Context" that doesn't modify the context object?

mauriciovasquezbernal · 2020-01-17T18:58:50Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+            _CONTEXT[key] = kwargs[key]
+        yield
+        for key in kwargs:
+            _CONTEXT[key] = snapshot[key]


Also if a non referenced slot in kwargs is changed, it won' t be restored.

from opentelemetry.context import Context Context.set_value('a', 'xxx') Context.set_value('b', 'yyy') print(Context.current().snapshot) with Context.use(a='foo'): print(Context.current().snapshot) Context.set_value('a', 'i_want_to_mess_it_but_wont_work') Context.set_value('b', 'i_want_to_mess_it') print(Context.current().snapshot)

Prints:

{'a': 'xxx', 'b': 'yyy'} {'a': 'foo', 'b': 'yyy'} {'a': 'xxx', 'b': 'i_want_to_mess_it'}

mauriciovasquezbernal · 2020-01-17T19:00:20Z

opentelemetry-api/src/opentelemetry/context/__init__.py

@@ -138,15 +138,119 @@ def __repr__(self):
        asyncio.run(main())


The documentation above has to be updated as well.

Fixed the restore issue. Will update the docs once the API changes are closer to done

mauriciovasquezbernal · 2020-01-17T19:52:03Z

opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py

+    def extract(
+        cls,
+        carrier,
+        context: typing.Optional[Context] = None,


Shouldn't the context parameter be passed down to with_span_context() so it can use it to decide if set the span context in the current context or in the passed context?

mauriciovasquezbernal · 2020-01-17T20:41:17Z

opentelemetry-api/src/opentelemetry/trace/propagation/context/__init__.py

+    return INVALID_SPAN_CONTEXT
+
+
+def with_span_context(span_context: SpanContext) -> Context:


I did a similar comment in the http trace extract: Shouldn't this function receive a context parameter to set the span on this? The parameter could be optional and if context is None the current one could be used.

c24t

One more round of comments on inject/extract. I still have some big open questions about explicit context propagation, but this is more because I don't understand the intent of the spec than because of any change here.

Still TODO: review correlation context.

c24t · 2020-01-17T23:19:14Z

opentelemetry-api/src/opentelemetry/context/propagation/__init__.py

+    Setter,
+)
+
+__all__ = [


I know it predates this change, but AFAICT we don't actually need __all__ here: #213 (comment).

c24t · 2020-01-17T23:29:16Z

opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py

@@ -15,17 +15,19 @@
 import abc


Does "format" still have any meaning? Would we lose anything by renaming this module http.py?

What about "text"? It doesn't look like the API distinguishes between text- and byte-valued carriers, is there any reason we still have binaryformat.py?

c24t · 2020-01-17T23:37:09Z

opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py



-class HTTPTextFormat(abc.ABC):
+class HTTPExtractor(abc.ABC):


Maybe a question for the spec, but what makes these "HTTP" extractor/injectors? It doesn't look like there's anything specific to HTTP in here, and the spec defines inject and extract generically enough to apply to any protocol.

That's a good point. I'll rename the API from HTTPExtractor and HTTPInjector to Extractor and Injector, since it is generic enough at this point. I'll also move that interface into the propagation/__init__.py module directly. This can be changed later if it no longer makes sense, once the spec is flushed out for the propagation API.

There are three things because of which "HTTP" kinda makes sense

To distinguish them from binary injectors (HTTP implies text based).

To distinguish them from injectors which just return a single string object (namely the binary ones) (HTTP injectors are based on key-value maps)

(Probably) to indicate the that the injector uses only HTTP-compatible keys and values (e.g. ASCII, starts with a-zA-Z, contains no spaces)

Maybe TextMapExtractor would be a better name. I think the 3rd point does not necessarily need to be in the interface. If we still want that, OpenTracing-cpp had an empty interface for HTTPHeader deriving from the corresponding TextMap interface (e.g. https://github.com/opentracing/opentracing-cpp/blob/4bb431f7728eaf383a07e86f9754a5b67575dab0/include/opentracing/propagation.h#L176), we could do the same so we could check isinstance(myTextMapExtractor, HTTPExtractor).

Good call @Oberon00, I missed that Getter and Setter specified str keys.

How would you expect the binary versions to work now? They're not specified in https://github.com/tedsuo/rfcs/blob/context-prop-2/text/0066-separate-context-propagation.md#Propagation-API, but the API changes to (HTTP) injectors/extractors would seem to make https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-propagators.md#binary-format obsolete.

What would it look like ff we split this up into http/text and binary versions? Something like this?

TextSetter = typing.Callable[[CarrierT, str, str], None] TextGetter = typing.Callable[[CarrierT, str], Sequence[str]]

BinarySetter = typing.Callable[[CarrierT, T], None] BinaryGetter = typing.Callable[[CarrierT], T]

(Note that I changed ContextT here to CarrierT to make it clear this is the carrier arg.)

Under the old spec, T would be a SpanContext or DistributedContext, and we set/get all fields in the carrier at once. Under the new spec, T could be any "concern".

It's not clear to me why we shouldn't be able to set/get individual keys for binary carriers as well as text carriers. Now that the carrier is generic, what would we lose by making the key and value types generic as well?

c24t · 2020-01-17T23:41:35Z

opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py

+        context: typing.Optional[Context] = None,
+        set_in_carrier: typing.Optional[Setter[ContextT]] = None,
+    ) -> None:
+        """Inject values from a SpanContext into a carrier.


Reminder that these docs need to be updated.

c24t · 2020-01-18T00:04:38Z

opentelemetry-api/src/opentelemetry/correlationcontext/propagation/binaryformat.py



+# TODO: can this be removed until it is needed?


👍

AFAICT we don't even need separate text and binary "formats" with the new API.

c24t · 2020-01-18T00:34:09Z

opentelemetry-api/src/opentelemetry/propagation/__init__.py

+    carrier: ContextT,
+    context: typing.Optional[Context] = None,
+    extractors: typing.Optional[
+        typing.List[httptextformat.HTTPExtractor]


Nit: this should probably be Sequence, like many of our other Lists in type annotations.

c24t · 2020-01-18T00:35:55Z

opentelemetry-api/src/opentelemetry/propagation/__init__.py

+def extract(
+    carrier: ContextT,
+    context: typing.Optional[Context] = None,
+    extractors: typing.Optional[


Do we have a use case for injecting/extracting with a different set of injector/extractors than the global one ?

c24t · 2020-01-18T00:37:38Z

opentelemetry-api/src/opentelemetry/propagation/__init__.py

+
+
+_HTTP_TEXT_INJECTORS = [
+    DefaultHTTPInjector


If these are only used in inject and extract, what's the difference between an empty list and a list with a single no-op injector/extractor?

c24t · 2020-01-18T00:39:56Z

opentelemetry-api/src/opentelemetry/context/propagation/httptextformat.py

@@ -93,9 +99,120 @@ def extract(

        """

+
+class HTTPInjector(abc.ABC):
+    @classmethod


Why make these classmethods? Is it an error to ever instantiate an injector/extractor?

c24t · 2020-01-18T00:49:15Z

opentelemetry-api/src/opentelemetry/trace/__init__.py

@@ -416,7 +417,9 @@ class Tracer:
    # This is the default behavior when creating spans.
    CURRENT_SPAN = Span()

-    def get_current_span(self) -> "Span":
+    def get_current_span(
+        self, context: typing.Optional[Context] = None


As I understand it, returning the context here (and accepting it as an arg below) is required for "explicit" context propagation. But I still don't understand the use case for maintaining multiple contexts, and since the rest of the code assumes implicit (or "automatic" as per the spec) context propagation, I suspect there are more changes we'd have to make to make this work.

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

mauriciovasquezbernal

Another round of general comments.

mauriciovasquezbernal · 2020-01-20T17:17:53Z

opentelemetry-api/src/opentelemetry/propagation/__init__.py

+    injectors: typing.Optional[
+        typing.List[httptextformat.HTTPInjector]
+    ] = None,
+    context: typing.Optional[Context] = None,


Is set_in_carrier missing in this function?

Fixed. Good catch, thanks!

mauriciovasquezbernal · 2020-01-20T18:43:57Z

opentelemetry-api/src/opentelemetry/propagation/__init__.py

+            headers. Should be paired with set_in_carrier, which
+            should know how to set header values on the carrier.
+    """
+    if context is None:


Noob question: couldn't it be done using default values in the function arguments?

mauriciovasquezbernal · 2020-01-20T18:49:39Z

opentelemetry-api/src/opentelemetry/propagation/__init__.py

+        injector.inject(context=context, carrier=carrier)
+
+
+_HTTP_TEXT_INJECTORS = [


Any specific reason to use a list instead of a tuple?

mauriciovasquezbernal · 2020-01-20T19:07:16Z

opentelemetry-api/src/opentelemetry/context/propagation/__init__.py


-__all__ = ["BinaryFormat", "HTTPTextFormat"]
+def set_in_dict(


I see that it's not used anywhere in the API and the default values for getter are None in the API.
Is this and get_as_list like helper functions that the API provide?

mauriciovasquezbernal · 2020-01-20T19:27:20Z

examples/opentelemetry-example-app/src/opentelemetry_example_app/context_propagation_example.py

+            if version == "2.0":
+                return fetch_from_service_c()
+
+        return fetch_from_service_b()


I think it should have one more indentation level.

examples/opentelemetry-example-app/src/opentelemetry_example_app/context_propagation_example.py

mauriciovasquezbernal · 2020-01-20T20:12:16Z

opentelemetry-api/src/opentelemetry/correlationcontext/propagation/__init__.py

+    KEY = "correlation-context"
+
+    @classmethod
+    def span_context_key(cls) -> str:


Is span_context_key the right name?

mauriciovasquezbernal · 2020-01-20T21:06:26Z

opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py


-        On exiting, the context manager will restore the parent
-        DistributedContext.
+    @classmethod


Is it needed to implement these methods?

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

mauriciovasquezbernal · 2020-01-21T02:01:36Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -507,7 +518,7 @@ def __init__(
    ):
        # TODO: How should multiple TracerSources behave? Should they get their own contexts?
        # This could be done by adding `str(id(self))` to the slot name.
-        self._current_span_slot = Context.register_slot("current_span")
+        self._current_span_name = "current_span"


I think there is something wrong, are there two slots with an almost equal name?, current-span and current_span?

mauriciovasquezbernal · 2020-01-21T02:04:19Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py

@@ -529,8 +540,9 @@ def get_tracer(
            ),
        )

-    def get_current_span(self) -> Span:
-        return self._current_span_slot.get()
+    def get_current_span(self, context: Optional[Context] = None) -> Span:


Shouldn't it use span_from_context?

mauriciovasquezbernal · 2020-01-21T02:13:17Z

opentelemetry-sdk/src/opentelemetry/sdk/context/propagation/b3_format.py

+        carrier,
+        context: typing.Optional[Context] = None,
+        get_from_carrier: typing.Optional[Getter[_T]] = get_as_list,
+    ):


Nit: missing return type annotation?

mauriciovasquezbernal · 2020-01-21T02:19:40Z

opentelemetry-api/src/opentelemetry/propagation/__init__.py

+        carrier: ContextT,
+        context: typing.Optional[Context] = None,
+        set_in_carrier: typing.Optional[Setter[ContextT]] = None,
+    ) -> None:


According to the OTEP it should return the carrier. Do you think it should return it? I'm not able to understand the motivation of returning them.

That's a good question. Looking at the examples, it looks like the simplified API injects directly into the carrier. but the previous example shows it setting the request header to the return value.

Changes the Context to be an ABC that is implemented by BaseContext which is then extended by ThreadLocalRuntimeContext and AsyncRuntimeContext. Signed-off-by: Alex Boten <aboten@lightstep.com>

mauriciovasquezbernal · 2020-01-21T15:34:03Z

opentelemetry-sdk/src/opentelemetry/sdk/correlationcontext/__init__.py

@@ -15,52 +15,48 @@
 import typing
 from contextlib import contextmanager

-from opentelemetry import distributedcontext as dctx_api
+from opentelemetry import correlationcontext as dctx_api


Should dctx_api be renamed to something else?

mauriciovasquezbernal · 2020-01-21T15:59:44Z

opentelemetry-sdk/tests/context/test_context.py

+        "test_span5",
+    ]
+
+    def do_some_work(self, name):


I'd suggest to rename this as there is already do_work() defined above.

mauriciovasquezbernal · 2020-01-21T16:33:57Z

opentelemetry-sdk/tests/context/test_context.py

+
+
+class TestContext(unittest.TestCase):
+    spans = [


Could be span_names a better name?

mauriciovasquezbernal · 2020-01-21T16:36:06Z

opentelemetry-sdk/tests/context/test_context.py

+        spans_names_list = [span.name for span in span_list]
+        self.assertListEqual(
+            [
+                "test_span1",


I think this test has to be reworked to avoid testing the order of the finished spans as it is random.

mauriciovasquezbernal · 2020-01-21T16:38:55Z

opentelemetry-sdk/tests/context/test_context.py

+                # Start the load operations
+                for span in self.spans:
+                    executor.submit(
+                        contextvars.copy_context().run,


What if ThreadLocalRuntimeContext is used instead of AsyncRuntimeContext?
Context includes with_current_context, why not to use that?

mauriciovasquezbernal · 2020-01-21T17:00:38Z

opentelemetry-sdk/tests/context/test_context.py

+    def test_propagation(self):
+        pass
+
+    def test_with_futures(self):


Perhaps this should use asyncio instead of threads?

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

mauriciovasquezbernal

I think the fact that we provide automatic context and some functions (like set_value(), value(), etc) are used on the current or in a different context creates some confusion.

The current approach is to define a Context class and define some methods that receive a context parameter, if the value passed is None then the operation is performed on the current context, otherwise it is performed on the passed object. I think this class is getting complicated as it handles both cases, accessing the current context and accessing a snapshot of it.

As @c24t already pointed above, maybe it could be better to implement such methods directly on the module and not in the Context class and have a Context / ContextView class that is a immutable snapshot of the context.

I created the following pseudo code to clarify my ideas, I think a similar approach could work.

# place to save the current context
_slots = {} ....


def set_value(key, value, context = None):
  # Function inside the module that performs the action on the current context
  # or in the passsed one based on the context object
  if context:
    ret = Context()
    ret.snapshot = context.snapshot # deep copy to avoid having the same snapshot in both
    ret.snapshot[key] = value
    return ret
  
  # update value on current context:
  slot = _register_slot(key)
  slot.set(value)
  return current()
  
  
def value(key, context = None):
  if context:
    return context.value(key)
    
  # get context from current context
  slot = slots[key]
  return slot.get()


def current():
  ret = Context()
  for key, value in _slots:  
    ret.snapshot[key] = value.get()
  
  return ret


def set_current(context):
  slots.cleart() # remove current data
  
  for key, value in current.snapshot:
    slots[key] = value

def _register_slot():
  # creates a slot based on the implmentation to use, contextvars or threadlocal


    
class Context:
  # Context represents a snapshot of the context, basically wraps a dict object
  def __init__():
    snapshot = {}

  def value(key):
    return snapshot[key]

  #def set_value() not implemented as Context is immutable

mauriciovasquezbernal · 2020-01-21T20:01:02Z

opentelemetry-api/src/opentelemetry/context/base_context.py

+        """
+
+
+class BaseContext(Context):
    class Slot:


Is it needed to define Slot inside BaseContext? I personally find it easier to understand if Slot is an independent (abstract) class and it is implemented by ThreadLocalRuntimeContext & AsyncRuntimeContext.

mauriciovasquezbernal · 2020-01-22T15:30:11Z

opentelemetry-api/src/opentelemetry/context/base_context.py

+        API provides a function which takes a Context.
+        """
+        keys = self._slots.keys()
+        for name in keys:


Is it really needed to clear all the slots before removing them?

mauriciovasquezbernal · 2020-01-22T19:00:09Z

opentelemetry-api/src/opentelemetry/trace/propagation/context/__init__.py

+    span = span_from_context(context=context)
+    if span:
+        return span.get_context()
+    sc = current().value(ContextKeys.span_context_key(), context=context)  # type: ignore


current() returns a snapshot of the current context but only a key/slot is used, shouldn't be a more straightforward way to get a single "slot" without creating a full snapshot?

mauriciovasquezbernal · 2020-01-22T19:16:02Z

opentelemetry-api/src/opentelemetry/trace/propagation/context/__init__.py

+
+
+def with_span(span: Span, context: Optional[Context] = None) -> Context:
+    return current().set_value(ContextKeys.span_key(), span, context=context)


Why to get a snapshot of the current context when the context parameter is passed?
Shouldn't it be only something like return context.set_value() in that case?

mauriciovasquezbernal · 2020-01-22T19:25:20Z

opentelemetry-api/src/opentelemetry/context/base_context.py

-            self[key] = snapshot[key]
+    def value(
+        self, key: str, context: typing.Optional["Context"] = None
+    ) -> typing.Optional["object"]:


I find this method rather confusing. It's part of class BaseContext that implements Context, it receives a context parameter of type Context and, if context is passed the value is got from that, otherwise from self.

In other words, A.do_work(par=B) is actually doing work on B and not in A.

mauriciovasquezbernal · 2020-01-22T19:33:42Z

opentelemetry-api/src/opentelemetry/context/base_context.py

@@ -67,64 +147,98 @@ def register_slot(
                cls._slots[name] = cls.Slot(name, default)


Slot could be replaced by a variable that is set to AsyncRuntimeContext or ThreadLocalRuntimeContext based on a falling back mechanism similar to the one implemented in new_context().

mauriciovasquezbernal · 2020-01-22T19:35:19Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+    return dest.merge(source)
+
+
+_CONTEXT = new_context()


Probably you don't need to have an object here but the type of context to use.

codeboten · 2020-01-23T06:01:37Z

After spending a bunch of time on this, I agree that it's confusing for the same code to try and behave in two different ways. I was trying to keep the Context API as an abstract class to make it easier to move the implementation in the SDK down the road, but I'm not sure the added level of complexity makes sense here and there's alternative approaches to accomplishing this. Will submit an update to the PR shortly, which implements what @c24t, @ocelotl and @mauriciovasquezbernal are suggesting. Thanks for all the feedback.

Simplifying the Context API implementation significantly: - removing multiple layers of Context classes, replacing it with `context` module level functions - moved Slot classi out of BaseContext - broke off threads/futures test into separate tests files, which allowed me to move test_context back into the api package where it belongs Signed-off-by: Alex Boten <aboten@lightstep.com>

mauriciovasquezbernal

I reviewed the last changes in the context part and it looks good to me, just few nits.

mauriciovasquezbernal · 2020-01-23T18:47:48Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+        return ret
+
+    # update value on current context:
+    slot = _register_slot(name)


Can we avoid taking the lock when updating slots that are registered already?
Maybe a conditional here to call _register_slot() only on new ones?

mauriciovasquezbernal · 2020-01-23T20:23:31Z

opentelemetry-api/src/opentelemetry/context/async_context.py

+            self.contextvar.set(value)
+
+    class AsyncRuntimeContext(base_context.Context):
+        def with_current_context(


I'm wondering if this method has to be different for both implementations.

Wouldn't an implementation like the following work?

opentelemetry-python/opentelemetry-api/src/opentelemetry/context/base_context.py

Lines 112 to 130 in ccb97e5

def with_current_context(

self, func: typing.Callable[..., "object"]

) -> typing.Callable[..., "object"]:

"""Capture the current context and apply it to the provided func.

"""

caller_context = self.snapshot()

def call_with_current_context(

*args: "object", **kwargs: "object"

) -> "object":

try:

backup_context = self.snapshot()

self.apply(caller_context)

return func(*args, **kwargs)

finally:

self.apply(backup_context)

return call_with_current_context

mauriciovasquezbernal · 2020-01-23T20:25:20Z

opentelemetry-api/src/opentelemetry/context/__init__.py

+    _slots.clear()  # remove current data
+
+    for key, val in context.snapshot.items():
+        slot = _register_slot(key)


Nit: it could be optimized by taking the lock once and having a lock-less function that registers the slot. For instance

slot = _slot_class(key) slot.set(val) _slots[key] = slot

* fix(grpc): fix client/server span propagation * fix(test): uncomment grpc patch * fix: linting, add missing unwrap * docs(grpc): add supported versions to readme

toumorokoshi and others added 3 commits November 7, 2019 15:21

stopgap PR to start the discussion on context propagation.

5f84c2b

Merge remote-tracking branch 'origin/master' into context-prop

6bea6ec

adding Baggage API

bbb583d

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

codeboten added the WIP Work in progress label Dec 10, 2019

codeboten changed the title ~~Context prop~~ [WIP] Context Prop Dec 10, 2019

Alex Boten added 11 commits December 10, 2019 10:01

no-op implementations of baggage injector/extractor

c3f408b

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

cleaning up baggage prop interface

8ec6106

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

make extract/inject static

9788f69

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

rename DistributedContext to CorrelationContext

6a46a25

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

adding ContextKeys for trace and distributedcontext

5ef40df

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

moving api/propagators -> api/propagation. adding more to context api

0442f29

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

add from_context/with_span_context helpers

622d787

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

splitting propagator into extractor/injector

317e937

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

checkpoint checkin, this PR will not leave draft mode

64cfe9b

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

implement context scoping

22a5c64

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

minor cleanup

375b78e

toumorokoshi suggested changes Dec 16, 2019

View reviewed changes

Alex Boten added 2 commits December 16, 2019 10:28

clean up example code

5ca5328

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

pass context where needed

b475933

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

toumorokoshi mentioned this pull request Dec 16, 2019

[WIP] PR to start the discussion on context propagation. #278

Closed

Alex Boten added 10 commits December 16, 2019 14:41

removing baggage module

c4829c4

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

small lint improvements

c7610fa

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

more lint fixes

7489eb5

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

fixing a few more lint issues

a4c4150

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

fixing inject/extract signatures and tracecontext tests

0946846

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

fixing tests

033e27e

rename current -> snapshot

cc813bb

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

fix remaining sdk tests

7391372

small context cleanup, return obj, remove unused __getattr__

fa6d437

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

store both current span and extracted span context

5fa8e02

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

codeboten commented Jan 15, 2020

View reviewed changes

c24t reviewed Jan 16, 2020

View reviewed changes

c24t mentioned this pull request Jan 16, 2020

Implement baggage support in the OT shim #239

Closed

mauriciovasquezbernal reviewed Jan 17, 2020

View reviewed changes

c24t reviewed Jan 18, 2020

View reviewed changes

Alex Boten added 5 commits January 20, 2020 09:37

Removing context correlation propagators

6965c33

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

Lint changes

c129e82

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

Adding test and fixing restore behaviour

0a6c385

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

adding restore test

57ad2d2

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

Pass context to set_value

cfdfc62

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

mauriciovasquezbernal reviewed Jan 20, 2020

View reviewed changes

Alex Boten added 3 commits January 20, 2020 13:51

Rename HTTPInjector HTTPExtractor to Injector Extractor

257627c

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

Moving propagation api into propagation/__init__.py

b210df8

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

Fix example

73dbfab

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

mauriciovasquezbernal reviewed Jan 21, 2020

View reviewed changes

Context refactor

b9be7bd

Changes the Context to be an ABC that is implemented by BaseContext which is then extended by ThreadLocalRuntimeContext and AsyncRuntimeContext. Signed-off-by: Alex Boten <aboten@lightstep.com>

codeboten marked this pull request as ready for review January 21, 2020 17:26

codeboten requested a review from a team January 21, 2020 17:26

mauriciovasquezbernal reviewed Jan 21, 2020

View reviewed changes

Alex Boten added 2 commits January 21, 2020 10:58

Adding set_in_carrier parameter, updating docs

d70a47c

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

rename dctx_api

1e3ee56

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

mauriciovasquezbernal reviewed Jan 22, 2020

View reviewed changes

mauriciovasquezbernal reviewed Jan 23, 2020

View reviewed changes

codeboten mentioned this pull request Jan 28, 2020

Add CorrelationContext and stub Context API #379

Closed

codeboten closed this Jan 30, 2020

codeboten deleted the context-prop branch October 9, 2020 18:50

		from opentelemetry.baggage import BaggageManager


		def configure_opentelemetry(flask_app: flask.Flask):

		@@ -319,7 +323,7 @@ def __init__(

		def get_current_span(self):


		return call_with_current_context

		def apply(self, ctx: "Context") -> None:

		@@ -138,15 +138,119 @@ def __repr__(self):
		asyncio.run(main())

		return INVALID_SPAN_CONTEXT


		def with_span_context(span_context: SpanContext) -> Context:

		injector.inject(context=context, carrier=carrier)


		_HTTP_TEXT_INJECTORS = [


		__all__ = ["BinaryFormat", "HTTPTextFormat"]
		def set_in_dict(



		def with_span(span: Span, context: Optional[Context] = None) -> Context:
		return current().set_value(ContextKeys.span_key(), span, context=context)

		@@ -67,64 +147,98 @@ def register_slot(
		cls._slots[name] = cls.Slot(name, default)

	def with_current_context(
	self, func: typing.Callable[..., "object"]
	) -> typing.Callable[..., "object"]:
	"""Capture the current context and apply it to the provided func.
	"""

	caller_context = self.snapshot()

	def call_with_current_context(
	args: "object", *kwargs: "object"
	) -> "object":
	try:
	backup_context = self.snapshot()
	self.apply(caller_context)
	return func(args, *kwargs)
	finally:
	self.apply(backup_context)

	return call_with_current_context

[WIP] Context Prop #325

[WIP] Context Prop #325

Conversation

codeboten commented Dec 10, 2019 • edited Loading

Global initialization

Extracting and injecting from HTTP headers

Simpler API with context prop using current context

Implementing a propagator

Implementing a concern

The scope of current context

Referencing multiple contexts

Falling back to explicit contexts

toumorokoshi left a comment

Choose a reason for hiding this comment

Named Tracers and Spans from Extractor

Context propagating across thread boundaries

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c24t left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten commented Dec 10, 2019 •

edited

Loading

codeboten Dec 17, 2019 •

edited

Loading

mauriciovasquezbernal Jan 21, 2020 •

edited

Loading

c24t left a comment •

edited

Loading

c24t left a comment •

edited

Loading