Propagator Class [POC for review] #121

toumorokoshi · 2019-09-05T05:38:12Z

This propagator class helps streamline integrations with formatters.

Starting this of as a PR for review. This is built on top of #89

Adding a UnifiedContext, composing DistributedContext and SpanContext. This will enable propagators to extract and inject values from either system, enabling more sophisticated schemes and standards to propagate data. This also removes the need for generics and propagators that only consume one or the other, requiring integrators to do extra work to wire propagators appropriately. Modifying the API of the propagators to consume the context as a mutable argument. By passing in the context rather than returning, this enables the chained use of propagators, allowing for situations such as supporting multiple trace propagation standards simulatenously.

Migrating formatters outside of context. The design goal is to allow context to be it's own object agnostic of the requirements of specific telemetry. Instead, moving it under a broader propagators module. Removing UnifiedContext in lieu of directly using Context. UnifiedContext adds an unneeded layer of abstraction that would then require more operations to migrate into context. Removing binaryformat and httptextformat modules specific to SpanContext and DistributedContext. These are now handled by a single formatter.

Removing the last few mentions of UnifiedContext Fixing rst syntax of the context module.

a-feld · 2019-09-05T15:07:24Z

opentelemetry-sdk/src/opentelemetry/sdk/propagator/b3_format.py

+        # the SpanContext or DistributedContext from a
+        # context object declared in opentelemetry-api.
+        # this should be more standard.
+        context[Tracer.CONTEXT_SLOT_NAME] = trace.SpanContext(


I gave this PR a glance and I love where this is heading! Rather than passing the entire context to the propagator have we considered passing the slot to the constructor instead?

If the slot was passed, the slot get/set interface can be used to set the context, making the API more standard.

the hope here was to give the the ability to the formatter to modify any context. I feel like if we pass explicit slots, then this would remove the flexibility.

But I think this discussion should probably continue in #89

What are the use cases for a formatter modifying multiple context slots? And could those use cases be covered by creating multiple instances of the formatter with different slots in the constructor?

Actually I think I understand the use-case, correct me if I'm wrong. Is this trying to cover the case where formats (to be defined) may transport span context and distributed context simultaneously?

While this doesn't cover named tracers, I can see handling those cases as a separate PR. 👍

Yep! Exactly. OTEP here: open-telemetry/oteps#37

Yes, I'll pull up an issue on named tracers. I have a couple thoughts.

here's my thoughts on the named tracer scenario: just creating multiple context objects instead. issue for discussion here: #126

Named are AFAIK meant to solve a different set of use cases, e.g. disabling a troublesome part of instrumentation.

I feel like if we pass explicit slots, then this would remove the flexibility.

True, then we might as well go back to returning things (it feels like sin(a: float, result_storage: SinusResultSlot) -> None: just awkward).

Python3.4 and 3.5 do not support types on class attributes.

A Propagator class can help streamline integrations that require extracting and injecting context.

Making the requests test pass again. Fixing propagator impl in the SDK. The correct fields were not being used.

In version of python before 3.7, a full import path involving a submodule was failing tests.

Oberon00 · 2019-09-06T10:14:46Z

ext/opentelemetry-ext-http-requests/src/opentelemetry/ext/http_requests/__init__.py

@@ -77,6 +79,10 @@ def instrumented_request(self, method, url, *args, **kwargs):
            span.set_attribute("http.status_code", result.status_code)
            span.set_attribute("http.status_text", result.reason)

+            propagator.global_propagator().inject(


Probably off-topic, but: You are injecting the headers after the request was sent here, and into the response headers instead of the request headers.

I also wonder if we should automatically fall back to setter.__setitem__ if not callable(setter). Requiring users to pass dunder methods is a bit arcane.

sorry yes, good point. Will fix that.

defaulting to setitem is a good idea. Just requires someone to write a wrapper object if that doesn't already exist. A bound callable isn't a bad idea either.

I'll file an issue and pick that up in another PR.

Oberon00 · 2019-09-06T10:24:39Z

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

+    formatters.
+    """
+
+    def __init__(


Either remove the __init__ or implement the whole class in the API. Right now, the cut between API and SDK seems a bit misplaced. Also, currently the user needs to set both the global propagator and both formatters. If the propagator is only a dumb bundle of formatters (which now do not only format but also inject & extract data), there is no reason for it to be overridable in API-implementations.

In fact, the sketch document https://docs.google.com/document/d/1fxnfChmZauNlUJ7zqGs31x7gH70s9s_q1BWpvYVHNqA/edit which seems to be the latest version of the currently drafted context API changes (linked from https://docs.google.com/document/d/1rgXZik2_-2fU1KpVi7nze5ygWA8NhaLE8VmD20H4P8M/edit) has

func do(ctx context.Context, req *http.Request) { headers := req.Headers() propagator := dctx.Global().PropagatorForHTTP(headers) propagator.Inject(ctx, propagator) return http.Do(ctx, req) }

I agree, just maybe still not clear on how much has to be an API or not.

One thing to note here is that the formatters themselves now depend on the SDK, because there's no convention / specification around how the SpanContext / DistributedContext should be loaded into or retrieved from the Context object. My thought there was to just define what slots the context should fill.

With regards to the sketch: that's a possible option too. Then we would need globals for the formatters. Also I was hoping we could also bring in the logic to use multiple formatters, since that's often brought up (e.g. supporting both B3 and TraceContext).

For now I'll move propagators out of the SDK and into the API.

Oberon00 · 2019-09-06T11:57:42Z

opentelemetry-sdk/src/opentelemetry/sdk/propagator/b3_format.py

@@ -15,7 +15,8 @@
 import typing

 import opentelemetry.trace as trace
-from opentelemetry.context.propagation.httptextformat import HTTPTextFormat
+from opentelemetry.propagator.httptextformat import HTTPTextFormat
+from opentelemetry.sdk.trace import Tracer


 class B3Format(HTTPTextFormat):


I understand that we want to make propagation simpler to use. But at the lowest, building-block level, I think there should still be something that does not depend on the context, if only a free function extract_b3_spancontext(get_from_carrier, carrier) -> SpanContext.

Could you elaborate with the specific reason why that has value?

I have only the abstract reason that you should always strive to keep dependencies to as few as possible.
I imagine it could make combining multiple extractors simpler too.

Oberon00

As discussed in the last Python SIG meeting, we need something for propagation and merging this PR as-is (or after fixing easily fixable issues) is OK for the Alpha.

toumorokoshi · 2019-09-19T04:02:58Z

This is not needed for alpha, as #137 is merged.

toumorokoshi · 2019-11-08T05:09:23Z

closing in favor of #278, please continue any discussion there.

toumorokoshi added 4 commits September 2, 2019 11:56

re-adding missing BinaryFormat API.

37f57b6

Removing mention of UnifiedContext

1f37b2a

Removing the last few mentions of UnifiedContext Fixing rst syntax of the context module.

toumorokoshi requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00 and reyang as code owners September 5, 2019 05:38

a-feld reviewed Sep 5, 2019

View reviewed changes

toumorokoshi added 5 commits September 5, 2019 09:17

Fixing sphinx warnings

dc063a2

fixing type annotations for sphinx

c031df3

reverting type changes on class attributes

3d8dbde

Python3.4 and 3.5 do not support types on class attributes.

Authoring a propagator class

4e55a5e

A Propagator class can help streamline integrations that require extracting and injecting context.

Fixing some errors

b755b0c

Making the requests test pass again. Fixing propagator impl in the SDK. The correct fields were not being used.

toumorokoshi force-pushed the feature/propagators-object branch from 6e8d433 to b755b0c Compare September 6, 2019 04:24

Moving imports to relative

e4d43cb

In version of python before 3.7, a full import path involving a submodule was failing tests.

Oberon00 suggested changes Sep 6, 2019

View reviewed changes

Oberon00 reviewed Sep 6, 2019

View reviewed changes

toumorokoshi mentioned this pull request Sep 6, 2019

Consider using __setitem__ / __getitem__ instead of explicit setters / getters for propagators #125

Closed

Oberon00 mentioned this pull request Sep 6, 2019

Proposing a single formatter for all contexts open-telemetry/oteps#37

Closed

Oberon00 approved these changes Sep 13, 2019

View reviewed changes

Oberon00 mentioned this pull request Sep 13, 2019

unifying propagator formatters #89

Closed

Oberon00 added this to the Alpha Release milestone Sep 13, 2019

toumorokoshi removed this from the Alpha Release milestone Sep 19, 2019

toumorokoshi closed this Nov 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagator Class [POC for review] #121

Propagator Class [POC for review] #121

toumorokoshi commented Sep 5, 2019

a-feld Sep 5, 2019

toumorokoshi Sep 5, 2019

a-feld Sep 5, 2019

a-feld Sep 5, 2019 •

edited

Loading

toumorokoshi Sep 5, 2019

toumorokoshi Sep 6, 2019

Oberon00 Sep 9, 2019

Oberon00 Sep 9, 2019

Oberon00 Sep 6, 2019

toumorokoshi Sep 6, 2019

Oberon00 Sep 6, 2019

Oberon00 Sep 6, 2019

toumorokoshi Sep 6, 2019

Oberon00 Sep 6, 2019

toumorokoshi Sep 6, 2019

Oberon00 Sep 6, 2019

Oberon00 left a comment •

edited

Loading

toumorokoshi commented Sep 19, 2019

toumorokoshi commented Nov 8, 2019

Propagator Class [POC for review] #121

Propagator Class [POC for review] #121

Conversation

toumorokoshi commented Sep 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-feld Sep 5, 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

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

Oberon00 left a comment • edited Loading

Choose a reason for hiding this comment

toumorokoshi commented Sep 19, 2019

toumorokoshi commented Nov 8, 2019

a-feld Sep 5, 2019 •

edited

Loading

Oberon00 left a comment •

edited

Loading