Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Update opentelemetry-instrumentation/src/opentelemetry/instrumentatio…
…n/propagators.py
  • Loading branch information
owais committed Apr 19, 2021
commit 8565c64d1050c93be8b4aef39bd7684432c95fdb
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
# limitations under the License.

"""
This module implements experimental propagators to inject trace response context
into HTTP responses. This is useful for server side frameworks that start traces
This module implements experimental propagators to inject trace context
into response carriers. This is useful for server side frameworks that start traces
when server requests and want to share the trace context with the client so the
client can add it's spans to the same trace.

Expand Down Expand Up @@ -44,25 +44,47 @@ def set_global_response_propagator(propagator):
_RESPONSE_PROPAGATOR = propagator


class DictHeaderSetter:
class Setter(ABC):
@abstractmethod
def set(self, carrier, key, value):
"""Inject the provided key value pair in carrier."""


class DictHeaderSetter(Setter):
def set(self, carrier, key, value): # pylint: disable=no-self-use
old_value = carrier.get(key, "")
if old_value:
value = "{0}, {1}".format(old_value, value)
carrier[key] = value


default_setter = DictHeaderSetter()
class FuncSetter(Setter):
"""FuncSetter coverts a function into a valid Setter. Any function that can
set values in a carrier can be converted into a Setter by using FuncSetter.
This is useful when injecting trace context into non-dict objects such
HTTP Response objects for different framework.

For example, it can be used to create a setter for Falcon response object as:

setter = FuncSetter(falcon.api.Response.append_header)

and then used with the propagator as:

propagator.inject(falcon_response, setter=setter)

This would essentially make the propagator call `falcon_response.append_header(key, value)`
"""

class FuncSetter:
def __init__(self, func):
self._func = func

def set(self, carrier, key, value):
self._func(carrier, key, value)


default_setter = DictHeaderSetter()


class ResponsePropagator(ABC):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@abstractmethod
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Will make the change shortly.

def inject(
Expand Down Expand Up @@ -100,5 +122,7 @@ def inject(
),
)
setter.set(
carrier, _HTTP_HEADER_ACCESS_CONTROL_EXPOSE_HEADERS, header_name,
carrier,
_HTTP_HEADER_ACCESS_CONTROL_EXPOSE_HEADERS,
header_name,
)