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

Implementing Propagators API to use Context #446

Merged
merged 18 commits into from
Mar 6, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import opentelemetry.ext.wsgi as otel_wsgi
from opentelemetry import propagators, trace
from opentelemetry.ext.flask.version import __version__
from opentelemetry.trace.propagation import get_span_from_context
from opentelemetry.util import time_ns

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -57,9 +58,8 @@ def _before_flask_request():
span_name = flask_request.endpoint or otel_wsgi.get_default_span_name(
environ
)
parent_span = propagators.extract(
otel_wsgi.get_header_from_environ, environ
)
context = propagators.extract(otel_wsgi.get_header_from_environ, environ)
parent_span = get_span_from_context(context).get_context()
Copy link
Member

Choose a reason for hiding this comment

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

I think we may need to add another line here to extract or pass through correlation context, since we're not attaching this context as we start the activation.

Would it make sense to add context attach / detach to these extensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think so. added an attach/detach to flask and wsgi extensions.


tracer = trace.get_tracer(__name__, __version__)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from opentelemetry import context, propagators
from opentelemetry.ext.http_requests.version import __version__
from opentelemetry.trace import SpanKind
from opentelemetry.trace.propagation import set_span_in_context


# NOTE: Currently we force passing a tracer. But in turn, this forces the user
Expand Down Expand Up @@ -76,7 +77,8 @@ def instrumented_request(self, method, url, *args, **kwargs):
# to access propagators.

headers = kwargs.setdefault("headers", {})
propagators.inject(tracer, type(headers).__setitem__, headers)
ctx = set_span_in_context(span)
propagators.inject(type(headers).__setitem__, headers, ctx)
mauriciovasquezbernal marked this conversation as resolved.
Show resolved Hide resolved
result = wrapped(self, method, url, *args, **kwargs) # *** PROCEED

span.set_attribute("http.status_code", result.status_code)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def setUp(self):
self.get_tracer = self.get_tracer_patcher.start()
self.span_context_manager = mock.MagicMock()
self.span = mock.create_autospec(trace.Span, spec_set=True)
self.span.get_context.return_value = trace.INVALID_SPAN_CONTEXT
self.span_context_manager.__enter__.return_value = self.span

def setspanattr(key, value):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@
from opentelemetry.ext.opentracing_shim import util
from opentelemetry.ext.opentracing_shim.version import __version__
from opentelemetry.trace import DefaultSpan
from opentelemetry.trace.propagation import (
get_span_from_context,
set_span_in_context,
)

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -677,11 +681,8 @@ def inject(self, span_context, format, carrier):

propagator = propagators.get_global_httptextformat()

propagator.inject(
DefaultSpan(span_context.unwrap()),
type(carrier).__setitem__,
carrier,
)
ctx = set_span_in_context(DefaultSpan(span_context.unwrap()))
propagator.inject(type(carrier).__setitem__, carrier, context=ctx)

def extract(self, format, carrier):
"""Implements the ``extract`` method from the base class."""
Expand All @@ -700,6 +701,7 @@ def get_as_list(dict_object, key):
return [value] if value is not None else []

propagator = propagators.get_global_httptextformat()
otel_context = propagator.extract(get_as_list, carrier)
ctx = propagator.extract(get_as_list, carrier)
otel_context = get_span_from_context(ctx).get_context()

return SpanContextShim(otel_context)
39 changes: 33 additions & 6 deletions ext/opentelemetry-ext-opentracing-shim/tests/test_shim.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,26 @@
# pylint:disable=no-member

import time
import typing
from unittest import TestCase

import opentracing

import opentelemetry.ext.opentracing_shim as opentracingshim
from opentelemetry import propagators, trace
from opentelemetry.context.propagation.httptextformat import HTTPTextFormat
from opentelemetry.context import Context
from opentelemetry.ext.opentracing_shim import util
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.trace.propagation import (
get_span_from_context,
set_span_in_context,
)
from opentelemetry.trace.propagation.httptextformat import (
Getter,
HTTPTextFormat,
HTTPTextFormatT,
Setter,
)


class TestShim(TestCase):
Expand Down Expand Up @@ -542,19 +553,35 @@ class MockHTTPTextFormat(HTTPTextFormat):
SPAN_ID_KEY = "mock-spanid"

@classmethod
def extract(cls, get_from_carrier, carrier):
def extract(
cls,
get_from_carrier: Getter[HTTPTextFormatT],
carrier: HTTPTextFormatT,
context: typing.Optional[Context] = None,
) -> Context:
trace_id_list = get_from_carrier(carrier, cls.TRACE_ID_KEY)
span_id_list = get_from_carrier(carrier, cls.SPAN_ID_KEY)

if not trace_id_list or not span_id_list:
return trace.INVALID_SPAN_CONTEXT
return set_span_in_context(trace.INVALID_SPAN)

return trace.SpanContext(
trace_id=int(trace_id_list[0]), span_id=int(span_id_list[0])
return set_span_in_context(
trace.DefaultSpan(
trace.SpanContext(
trace_id=int(trace_id_list[0]),
span_id=int(span_id_list[0]),
)
)
)

@classmethod
def inject(cls, span, set_in_carrier, carrier):
def inject(
cls,
set_in_carrier: Setter[HTTPTextFormatT],
carrier: HTTPTextFormatT,
context: typing.Optional[Context] = None,
) -> None:
span = get_span_from_context(context)
set_in_carrier(
carrier, cls.TRACE_ID_KEY, str(span.get_context().trace_id)
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

from opentelemetry import propagators, trace
from opentelemetry.ext.wsgi.version import __version__
from opentelemetry.trace.propagation import get_span_from_context
from opentelemetry.trace.status import Status, StatusCanonicalCode

_HTTP_VERSION_PREFIX = "HTTP/"
Expand Down Expand Up @@ -181,7 +182,8 @@ def __call__(self, environ, start_response):
start_response: The WSGI start_response callable.
"""

parent_span = propagators.extract(get_header_from_environ, environ)
context = propagators.extract(get_header_from_environ, environ)
parent_span = get_span_from_context(context)
span_name = get_default_span_name(environ)

span = self.tracer.start_span(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,3 @@
# 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.

from .binaryformat import BinaryFormat
from .httptextformat import HTTPTextFormat

__all__ = ["BinaryFormat", "HTTPTextFormat"]

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@
import typing
Copy link
Member

Choose a reason for hiding this comment

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

should tracecontext be moved to trace/propagation? It's a valid hierarchy. I don't see anything in the spec that calls out specifically where this should be: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/library-layout.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes more sense in trace/propagation than where it currently is. moved it.


import opentelemetry.trace as trace
from opentelemetry.context.propagation import httptextformat

_T = typing.TypeVar("_T")
from opentelemetry.context.context import Context
from opentelemetry.trace.propagation import (
get_span_from_context,
httptextformat,
set_span_in_context,
)

# Keys and values are strings of up to 256 printable US-ASCII characters.
# Implementations should conform to the `W3C Trace Context - Tracestate`_
Expand Down Expand Up @@ -61,32 +64,37 @@ class TraceContextHTTPTextFormat(httptextformat.HTTPTextFormat):

@classmethod
def extract(
cls, get_from_carrier: httptextformat.Getter[_T], carrier: _T
) -> trace.SpanContext:
cls,
get_from_carrier: httptextformat.Getter[
httptextformat.HTTPTextFormatT
],
carrier: httptextformat.HTTPTextFormatT,
context: typing.Optional[Context] = None,
) -> Context:
"""Extracts a valid SpanContext from the carrier.
"""
header = get_from_carrier(carrier, cls._TRACEPARENT_HEADER_NAME)

if not header:
return trace.INVALID_SPAN_CONTEXT
return set_span_in_context(trace.INVALID_SPAN, context)

match = re.search(cls._TRACEPARENT_HEADER_FORMAT_RE, header[0])
if not match:
return trace.INVALID_SPAN_CONTEXT
return set_span_in_context(trace.INVALID_SPAN, context)

version = match.group(1)
trace_id = match.group(2)
span_id = match.group(3)
trace_options = match.group(4)

if trace_id == "0" * 32 or span_id == "0" * 16:
return trace.INVALID_SPAN_CONTEXT
return set_span_in_context(trace.INVALID_SPAN, context)

if version == "00":
if match.group(5):
return trace.INVALID_SPAN_CONTEXT
return set_span_in_context(trace.INVALID_SPAN, context)
if version == "ff":
return trace.INVALID_SPAN_CONTEXT
return set_span_in_context(trace.INVALID_SPAN, context)

tracestate_headers = get_from_carrier(
carrier, cls._TRACESTATE_HEADER_NAME
Expand All @@ -99,29 +107,29 @@ def extract(
trace_options=trace.TraceOptions(trace_options),
trace_state=tracestate,
)

return span_context
return set_span_in_context(trace.DefaultSpan(span_context), context)

@classmethod
def inject(
cls,
span: trace.Span,
set_in_carrier: httptextformat.Setter[_T],
carrier: _T,
set_in_carrier: httptextformat.Setter[httptextformat.HTTPTextFormatT],
carrier: httptextformat.HTTPTextFormatT,
context: typing.Optional[Context] = None,
) -> None:
span_context = get_span_from_context(context).get_context()

context = span.get_context()

if context == trace.INVALID_SPAN_CONTEXT:
if span_context == trace.INVALID_SPAN_CONTEXT:
return
traceparent_string = "00-{:032x}-{:016x}-{:02x}".format(
context.trace_id, context.span_id, context.trace_options
span_context.trace_id,
span_context.span_id,
span_context.trace_options,
)
set_in_carrier(
carrier, cls._TRACEPARENT_HEADER_NAME, traceparent_string
)
if context.trace_state:
tracestate_string = _format_tracestate(context.trace_state)
if span_context.trace_state:
tracestate_string = _format_tracestate(span_context.trace_state)
set_in_carrier(
carrier, cls._TRACESTATE_HEADER_NAME, tracestate_string
)
Expand Down

This file was deleted.

Loading