Added experimental HTTP backpropagators by owais · Pull Request #1762 · open-telemetry/opentelemetry-python

owais · 2021-04-12T13:43:20Z

Description

The experimental back propagators inject trace response headers into
HTTP responses. These are meant to be used by instrumentations and are
not considered stable.

Contrib PRs that need this feature: open-telemetry/opentelemetry-python-contrib#436

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Unit tests
Manual testing

Checklist:

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

srikanthccv · 2021-04-13T13:49:01Z

This looks clean. Does it become part api once it is moved from experimental to stable or it is something remains with instrumentation package?

owais · 2021-04-13T13:50:02Z

I think it'll remain in instrumentation package until the trace response proposal is merged into W3C and proposed in Otel.

owais · 2021-04-14T19:28:24Z

@aabmass @lonewolf3739 PTAL when you can. Thanks.

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

aabmass

LGTM

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

ocelotl · 2021-04-15T21:32:31Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+_BACK_PROPAGATOR = None
+
+
+def get_global_back_propagator():


I feel like instrumentation_propagator should be a better name, considering the path of this module. Also, do we really need these get and set methods? They give the exact same "protection" and namespacing as not having them at all.

I can change but it's already under instrumentation package so the import path would become opentelemetry.instrumentation.instrumentation_propagator which seems redundant and causes unnecessary stutter.

What I mean is that using back here is confusing. This is a propagator that is associated with instrumentation, right?

What do you think about keeping these get and set methods, @owais?

@ocelotl not necessarily, its called back propagator because it injects context into the response headers to propagate context backward to the caller. I think Owais has added it to the instrumentation package just because its a convenient place that instrumentation packages already have access to. Eventually, we would move this to opentelemetry-api

And I believe the get/set are added here to keep a similar API to set_global_textmap(), set_tracer_provider() etc. ?

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

ocelotl · 2021-04-15T21:50:56Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+class TraceResponsePropagator:
+    """Experimental propagator that injects tracecontext into HTTP responses."""
+
+    _HEADER_NAME = "traceresponse"


Nit: this is a class attribute, not a module-level constant, it should be lower cased.

ocelotl · 2021-04-15T22:02:28Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+
+    _HEADER_NAME = "traceresponse"
+
+    def _format_header(self, span_context):  # pylint: disable=no-self-use


I would suggest to handle this with an ABC and static and abstract methods:

from abc import ABC, abstractmethod class Parent(ABC): @staticmethod @abstractmethod def method(arg): pass class Child(Parent): @staticmethod def method(arg): print(arg) Child().method(4) class Child(Parent): @staticmethod def mathod(arg): print(arg) Child().mathod(4)

4 Traceback (most recent call last): File "tato.py", line 26, in <module> Child().mathod(4) TypeError: Can't instantiate abstract class Child with abstract methods method

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

ocelotl · 2021-04-16T16:31:58Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+_BACK_PROPAGATOR = None
+
+
+def get_global_back_propagator():


What I mean is that using back here is confusing. This is a propagator that is associated with instrumentation, right?

ocelotl · 2021-04-16T16:32:44Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+_BACK_PROPAGATOR = None
+
+
+def get_global_back_propagator():


What do you think about keeping these get and set methods, @owais?

ocelotl · 2021-04-16T16:47:40Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+
+
+class ResponsePropagator(ABC):
+    @abstractmethod


This feels like a class attribute actually. If you want to still use a method to access it, it should be a property:

class Parent: _the_attribute = None @property def _attribute(self): return self._the_attribute class Child(Parent): _the_attribute = "child_attribute" print(Child()._attribute)

OK. Will make the change shortly.

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

ocelotl · 2021-04-16T17:05:40Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+    def _format_header(
+        self, span_context
+    ) -> str:  # pylint: disable=no-self-use
+        return 'traceparent;desc="00-{trace_id}-{span_id}-{:02x}"'.format(


I guess it comes from this?

owais · 2021-04-16T17:31:29Z

What I mean is that using back here is confusing. This is a propagator that is associated with instrumentation, right?

You're right. Even this implementation is calling it either "back propagator" or "response propagator". Agree we should pick one. I don't have a strong preference. Which one do you suggest to use? @ocelotl

owais · 2021-04-16T17:32:50Z

What do you think about keeping these get and set methods, @owais?

@ocelotl Sorry, I'm not following. Could you please elaborate?

(for some reason I'm not able to comment in-line on some of your comments).

owais · 2021-04-16T17:34:10Z

What I mean is that using back here is confusing. This is a propagator that is associated with instrumentation, right?

They kind of are. In the sense that only instrumentation is using them today and we don't document them for users but technically it's not tried to instrumentations at all. Users can use it in their manual code just like regular propagators.

ocelotl · 2021-04-16T17:38:48Z

What I mean is that using back here is confusing. This is a propagator that is associated with instrumentation, right?

You're right. Even this implementation is calling it either "back propagator" or "response propagator". Agree we should pick one. I don't have a strong preference. Which one do you suggest to use? @ocelotl

I prefer response, it is more specific.

ocelotl · 2021-04-16T17:40:02Z

What do you think about keeping these get and set methods, @owais?

@ocelotl Sorry, I'm not following. Could you please elaborate?

(for some reason I'm not able to comment in-line on some of your comments).

I mean the get_global_back_propagator and set_global_back_propagator which don't actually provide any protection to the propagator object, so simply using a module-level variable is the same thing with two less methods.

owais · 2021-04-16T18:32:39Z

I mean the get_global_back_propagator and set_global_back_propagator which don't actually provide any protection to the propagator object, so simply using a module-level variable is the same thing with two less methods.

@ocelotl Ah. I feel the functions are much more nicer to use and are consistent with how other global objects are set by API (provider, propagators). I don't think they really add any harm. I can add some type checking or safety later if you'd like.

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

owais · 2021-04-16T18:39:17Z

@ocelotl @lzchen

Updated to call use "response" instead of "back".
Removed "ServerTiming" propagator implementation.

ocelotl

This should work, approving. Just a question remains regarding ABCs and extract.

ocelotl · 2021-04-16T21:24:03Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+        self._func(carrier, key, value)
+
+
+class ResponsePropagator(ABC):


Approving, but just a question: don't we have already an ABC for propagators? I understand that these are "back" propagators (and they propagate in only one way, if I understand correctly), but may there be any situation where extract also needs to be defined?

These are generally for web servers to respond with. May be someone uses Python compiled to JS in browser extract would be helpful. For any other type of client, the client is in full control and can start a trace before sending requests out. Only JS in browser is unable to do that as browser starts first request with JS having no control over it.

But questions like this is exactly why adding it as a propagator made more sense so we can flesh out how this should work in some time and may be provide other SIGs with a reference implementation and feedback for spec.

lzchen · 2021-04-19T01:21:16Z

@ocelotl
Are your questions addressed?

lzchen · 2021-04-19T15:51:10Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+    _RESPONSE_PROPAGATOR = propagator
+
+
+class DictHeaderSetter:


Similarly to normal propagators, shouldn't Setter be an ABC so others can create their own Setter s?

lzchen · 2021-04-19T15:52:48Z

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py

+default_setter = DictHeaderSetter()
+
+
+class FuncSetter:


Can we get some guidance (maybe some comments) on when FuncSetter should be used vs Setter?

Added doc string that describes intention and usage.

The experimental back propagators inject trace response headers into HTTP responses. These are meant to be used by instrumentations and are not considered stable.

…n/propagators.py

owais requested review from a team, aabmass and srikanthccv and removed request for a team April 12, 2021 13:43

owais force-pushed the response-propagator branch from 4febe52 to 7b11971 Compare April 12, 2021 13:44

owais mentioned this pull request Apr 12, 2021

Django trace response headers open-telemetry/opentelemetry-python-contrib#395

Closed

12 tasks

owais force-pushed the response-propagator branch 2 times, most recently from 48247ca to 3ff4d24 Compare April 12, 2021 16:16

This was referenced Apr 13, 2021

Added trace response headers to Flask open-telemetry/opentelemetry-python-contrib#431

Closed

Added trace response headers to Falcon open-telemetry/opentelemetry-python-contrib#432

Closed

owais force-pushed the response-propagator branch from 3ff4d24 to f6b04c4 Compare April 13, 2021 06:44

srikanthccv approved these changes Apr 15, 2021

View reviewed changes

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py Show resolved Hide resolved

aabmass approved these changes Apr 15, 2021

View reviewed changes

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py Show resolved Hide resolved

owais force-pushed the response-propagator branch from ff3ee9c to bedd0c6 Compare April 15, 2021 20:39

ocelotl suggested changes Apr 15, 2021

View reviewed changes

owais force-pushed the response-propagator branch 5 times, most recently from d502b0f to cb5c78b Compare April 16, 2021 07:15

owais requested a review from aabmass April 16, 2021 07:16

owais force-pushed the response-propagator branch 2 times, most recently from 092dcf3 to e23dec1 Compare April 16, 2021 07:22

owais requested a review from ocelotl April 16, 2021 07:53

ocelotl suggested changes Apr 16, 2021

View reviewed changes

owais force-pushed the response-propagator branch from 73a603b to af7aa98 Compare April 16, 2021 18:31

aabmass approved these changes Apr 16, 2021

View reviewed changes

opentelemetry-instrumentation/src/opentelemetry/instrumentation/propagators.py Outdated Show resolved Hide resolved

owais force-pushed the response-propagator branch 2 times, most recently from 1eb8c0d to 7fba05a Compare April 16, 2021 18:36

owais requested review from lzchen and ocelotl April 16, 2021 18:37

owais force-pushed the response-propagator branch 2 times, most recently from bf0c681 to 880becc Compare April 16, 2021 18:43

ocelotl approved these changes Apr 16, 2021

View reviewed changes

lzchen approved these changes Apr 19, 2021

View reviewed changes

lzchen reviewed Apr 19, 2021

View reviewed changes

owais force-pushed the response-propagator branch 2 times, most recently from 05973b7 to 879aba3 Compare April 19, 2021 16:44

Added experimental HTTP backpropagators

2f8c190

The experimental back propagators inject trace response headers into HTTP responses. These are meant to be used by instrumentations and are not considered stable.

owais force-pushed the response-propagator branch from 879aba3 to 18f8ac2 Compare April 19, 2021 16:45

Update opentelemetry-instrumentation/src/opentelemetry/instrumentatio…

8565c64

…n/propagators.py

owais force-pushed the response-propagator branch from 18f8ac2 to 8565c64 Compare April 19, 2021 16:47

lzchen approved these changes Apr 19, 2021

View reviewed changes

lzchen merged commit 6bd163f into open-telemetry:main Apr 19, 2021

owais deleted the response-propagator branch April 19, 2021 17:24


		_HEADER_NAME = "traceresponse"

		def _format_header(self, span_context): # pylint: disable=no-self-use

		self._func(carrier, key, value)


		class ResponsePropagator(ABC):

Conversation

owais commented Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

srikanthccv commented Apr 13, 2021

Uh oh!

owais commented Apr 13, 2021

Uh oh!

owais commented Apr 14, 2021

Uh oh!

Uh oh!

aabmass left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owais commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owais commented Apr 16, 2021

Uh oh!

owais commented Apr 16, 2021

Uh oh!

ocelotl commented Apr 16, 2021

Uh oh!

ocelotl commented Apr 16, 2021

Uh oh!

owais commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

owais commented Apr 16, 2021

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzchen commented Apr 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

owais commented Apr 12, 2021 •

edited

Loading

owais commented Apr 16, 2021 •

edited

Loading

owais commented Apr 16, 2021 •

edited

Loading