Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagator Class [POC for review] #121

Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

from requests.sessions import Session

from opentelemetry import propagator


# NOTE: Currently we force passing a tracer. But in turn, this forces the user
# to configure a SDK before enabling this integration. In turn, this means that
Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

result.headers.__setitem__, result.headers
)

return result

# TODO: How to handle exceptions? Should we create events for them? Set
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,19 @@
import unittest
from unittest import mock

import opentelemetry.ext.http_requests
import requests
import urllib3
from opentelemetry import trace

import opentelemetry.ext.http_requests
from opentelemetry import propagator, trace
from opentelemetry.context import Context


class TestRequestsIntegration(unittest.TestCase):

# TODO: Copy & paste from test_wsgi_middleware
def setUp(self):
propagator.set_propagator(propagator.Propagator(Context, None, None))
self.span_attrs = {}
self.tracer = trace.tracer()
self.span_context_manager = mock.MagicMock()
Expand Down
109 changes: 109 additions & 0 deletions opentelemetry-api/src/opentelemetry/propagator/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
# Copyright 2019, OpenTelemetry Authors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import typing

from opentelemetry.context import BaseRuntimeContext

from . import binaryformat, httptextformat


class Propagator:
"""Class which encapsulates propagation of values to and from context.

In contract to using the formatters directly, a propagator object can
help own configuration around which formatters to use, as well as
help simplify the work require for integrations to use the intended
formatters.
"""

def __init__(
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

self,
context: BaseRuntimeContext,
httptextformat_instance: httptextformat.HTTPTextFormat,
binaryformat_instance: binaryformat.BinaryFormat,
):
self._context = context
self._httptextformat = httptextformat_instance
self._binaryformat = binaryformat_instance

def extract(
self, get_from_carrier: httptextformat.Getter, carrier: object
) -> None:
"""Extract context data from the carrier, add to the context.

Using the http_format specified in the constructor, extract the
data form the carrier passed and add values into the context object.

Args:
get_from_carrier: a function that can retrieve zero
or more values from the carrier. In the case that
the value does not exist, return an empty list.
carrier: and object which contains values that are
used to construct a SpanContext. This object
must be paired with an appropriate get_from_carrier
which understands how to extract a value from it.
"""

def inject(
self, set_in_carrier: httptextformat.Setter, carrier: object
) -> None:
"""Inject values from context into a carrier.

inject enables the propagation of values into HTTP clients or
other objects which perform an HTTP request. Using the
httptextformat configured, inject the context data into
the carrier with the set_in_carrier method passed.

Args:
set_in_carrier: A setter function that can set values
on the carrier.
carrier: An object that a place to define HTTP headers.
Should be paired with set_in_carrier, which should
know how to set header values on the carrier.
"""

def from_bytes(self, byte_representation: bytes) -> None:
"""Populate context with data that existed in the byte representation.

Using the configured binary_format, extract values from the bytes object
passed into the context object configured.

Args:
byte_representation: the bytes to deserialize.
"""

def to_bytes(self) -> bytes:
"""Creates a byte representation of the context configured.

to_bytes should read values from the configured context and
return a bytes object to represent it.

Returns:
A bytes representation of the DistributedContext.
"""


def set_propagator(propagator_instance: Propagator) -> None:
"""Set the propagator instance singleton.
"""
global _PROPAGATOR # pylint:disable=global-statement
_PROPAGATOR = propagator_instance


def global_propagator() -> typing.Optional[Propagator]:
"""Return the singleton propagator instance."""
return _PROPAGATOR


_PROPAGATOR = None # type: typing.Optional[Propagator]
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def to_bytes(context: BaseRuntimeContext) -> bytes:
def from_bytes(
context: BaseRuntimeContext, byte_representation: bytes
) -> None:
"""Populate context with context data that existed in the byte representation.
"""Populate context with data that existed in the byte representation.

from_bytes should add values into the context from the data serialized in the
byte_representation passed. If it is not possible to read in a proper
Expand Down
5 changes: 2 additions & 3 deletions opentelemetry-api/tests/test_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from importlib import reload
import os
import sys
import unittest
from importlib import reload

from opentelemetry import loader
from opentelemetry import trace
from opentelemetry import loader, trace

DUMMY_TRACER = None

Expand Down
15 changes: 15 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/propagator/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from opentelemetry.propagator import Propagator as PropagatorAPI


class Propagator(PropagatorAPI):
def extract(self, get_from_carrier, carrier):
self._httptextformat.extract(self._context, get_from_carrier, carrier)

def inject(self, set_in_carrier, carrier):
self._httptextformat.inject(self._context, set_in_carrier, carrier)

def from_bytes(self, byte_representation):
self._binaryformat.from_bytes(self._context, byte_representation)

def to_bytes(self):
return self._binaryformat.to_bytes(self._context)
4 changes: 2 additions & 2 deletions opentelemetry-sdk/tests/propagator/test_b3_format.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@
# limitations under the License.

import unittest
import opentelemetry.trace as api_trace

import opentelemetry.context as context
import opentelemetry.sdk.propagator.b3_format as b3_format
import opentelemetry.sdk.trace as trace

import opentelemetry.trace as api_trace
from opentelemetry.sdk.trace import tracer

FORMAT = b3_format.B3Format()
Expand Down
2 changes: 1 addition & 1 deletion opentelemetry-sdk/tests/trace/test_trace.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from unittest import mock
import unittest
from unittest import mock

from opentelemetry import trace as trace_api
from opentelemetry.sdk import trace
Expand Down