-
Notifications
You must be signed in to change notification settings - Fork 608
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
Adding a working propagator, adding to integrations and example #137
Changes from 1 commit
384496c
4e9f075
d856f9d
1781bcd
9789e58
1056545
1a3dc75
607a794
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ | |
|
||
from requests.sessions import Session | ||
|
||
import opentelemetry.propagator as 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 | ||
|
@@ -72,6 +74,11 @@ def instrumented_request(self, method, url, *args, **kwargs): | |
# TODO: Propagate the trace context via headers once we have a way | ||
# to access propagators. | ||
|
||
headers = kwargs.get("headers", {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of get + set, use |
||
propagator.get_global_propagator().inject( | ||
tracer, type(headers).__setitem__, headers | ||
) | ||
kwargs["headers"] = headers | ||
result = wrapped(self, method, url, *args, **kwargs) # *** PROCEED | ||
|
||
span.set_attribute("http.status_code", result.status_code) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,9 +19,10 @@ | |
""" | ||
|
||
import functools | ||
import typing | ||
import wsgiref.util as wsgiref_util | ||
|
||
from opentelemetry import trace | ||
from opentelemetry import propagator, trace | ||
from opentelemetry.ext.wsgi.version import __version__ # noqa | ||
|
||
|
||
|
@@ -35,12 +36,9 @@ class OpenTelemetryMiddleware: | |
wsgi: The WSGI application callable. | ||
""" | ||
|
||
def __init__(self, wsgi, propagators=None): | ||
def __init__(self, wsgi): | ||
self.wsgi = wsgi | ||
|
||
# TODO: implement context propagation | ||
self.propagators = propagators | ||
|
||
@staticmethod | ||
def _add_request_attributes(span, environ): | ||
span.set_attribute("component", "http") | ||
|
@@ -87,8 +85,11 @@ def __call__(self, environ, start_response): | |
|
||
tracer = trace.tracer() | ||
path_info = environ["PATH_INFO"] or "/" | ||
parent_span = propagator.get_global_propagator().extract( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd name that parent_context (because it is no span but a context). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is that the right call? the trace/init.py defines the type as "ParentSpan" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd argue that either both should be called ParentContext, or ParentSpan. |
||
get_header_from_environ, environ | ||
) | ||
|
||
with tracer.start_span(path_info) as span: | ||
with tracer.start_span(path_info, parent_span) as span: | ||
self._add_request_attributes(span, environ) | ||
start_response = self._create_start_response(span, start_response) | ||
|
||
|
@@ -99,3 +100,15 @@ def __call__(self, environ, start_response): | |
finally: | ||
if hasattr(iterable, "close"): | ||
iterable.close() | ||
|
||
|
||
def get_header_from_environ( | ||
environ: dict, header_name: str | ||
) -> typing.Optional[str]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The type is wrong. If we want to use type annotations for extensions, we must also run mypy on them (I wouldn't be against that, btw, as it is also useful without explicit type annotations). Also this returns There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, good point. This has been an outstanding issue since the initial b3 PR. I'll fix that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually I realized the b3 code is fine. but will fix this the wsgi spec. |
||
"""Retrieve the header value from the wsgi environ dictionary. | ||
|
||
Returns: | ||
A string with the header value if it exists, else None. | ||
""" | ||
environ_key = "HTTP_" + header_name.upper().replace("-", "_") | ||
return [environ.get(environ_key)] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import opentelemetry.trace as trace | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heads up that these new files need the license boilerplate. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, will add. side question: why do we need this in every file? is that a better choice to make from a legal perspective? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT we're just following Apache's own guidance here. There may be a legal reason to do this, but that question is above my paygrade. |
||
from opentelemetry.context.propagation import httptextformat | ||
|
||
|
||
class TraceStateHTTPTextFormat(httptextformat.HTTPTextFormat): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured since we want to propagate TraceState by default, we can eliminate an invalid propagator by sticking this here for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personally I'd prefer to keep the API doing nothing. But open-telemetry/opentelemetry-specification#208 and open-telemetry/opentelemetry-specification#183 are still open, so I'm OK with this for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading the rest of the PR, I see that this might just be a naming issue. Would you expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the proper name would be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @c24t @toumorokoshi There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I understand it, yes. |
||
"""TODO: a propagator that extracts and injects tracestate. | ||
""" | ||
|
||
def extract( | ||
self, _get_from_carrier: httptextformat.Getter, _carrier: object | ||
) -> trace.SpanContext: | ||
return trace.INVALID_SPAN_CONTEXT | ||
|
||
def inject( | ||
self, | ||
context: trace.SpanContext, | ||
set_in_carrier: httptextformat.Setter, | ||
carrier: object, | ||
) -> None: | ||
pass |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,77 @@ | ||||||
import typing | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably want to call this package |
||||||
|
||||||
import opentelemetry.context.propagation.httptextformat as httptextformat | ||||||
import opentelemetry.trace as trace | ||||||
from opentelemetry.context.propagation.tracestatehttptextformat import ( | ||||||
TraceStateHTTPTextFormat, | ||||||
) | ||||||
|
||||||
|
||||||
class Propagator: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need to be a class? Could't we make these free helper functions and have the selected formatters directly be globals? Or if we want to give API implementations more freedom, we should move the concept of formatters outside the SDK layer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. helper functions would work fine here... there's nothing that an SDK has to override really, and this was more important when Context was an object that was passed into the constructor. could do work to set httpformatters as global. I think ultimately the shape of globals will change anyway, don't particularly mind how configuration of httptextformatters look for this specific PR. |
||||||
"""Class which encapsulates propagation of values to and from context. | ||||||
|
||||||
In contrast 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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
formatters. | ||||||
""" | ||||||
|
||||||
def __init__(self, httptextformat_instance: httptextformat.HTTPTextFormat): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIRC one of the open questions about propagation is whether we should have separate interfaces for text and binary formats. This PR seems to use separate interfaces, and default to HTTP/text. Is that intentional or just a consequence of the fact that the binary format hasn't been written yet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, as a nit: |
||||||
self._httptextformat = httptextformat_instance | ||||||
|
||||||
def extract( | ||||||
self, get_from_carrier: httptextformat.Getter, carrier: object | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Oberon00 what are your thoughts on making the https://github.com/toumorokoshi/opentelemetry-python/pull/1/files This is cribbed from the loader, there may be a better way to do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, thanks for the PR! |
||||||
) -> typing.Union[trace.SpanContext, trace.Span, None]: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might have covered it in another PR, but what's the use case for returning a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is what the ParentSpan type is a union of. But there's probably no good reason, it could just be SpanContext. |
||||||
"""Load the parent SpanContext from values in the carrier. | ||||||
|
||||||
Using the specified HTTPTextFormatter, the propagator will | ||||||
extract a SpanContext from the carrier. If one is found, | ||||||
it will be set as the parent context of the current span. | ||||||
|
||||||
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. | ||||||
""" | ||||||
span_context = self._httptextformat.extract(get_from_carrier, carrier) | ||||||
return span_context if span_context else trace.Tracer.CURRENT_SPAN | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why return the CURRENT_SPAN if extract failed? In effect, the return value can only be used as argument for But I think there is an argument to be made that if there is unexpectedly no incoming span context, one would usually prefer to start a new trace instead of continuing the existing one, which would be implemented by just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, you're completely right. I think I misunderstood and thought that CURRENT_SPAN was a sentinel value that was required to make sure no exceptions were raised. I'll fix this to just be Optional[ParentSpan] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't the current span be null anyway? Since we pick up the trace ID from the incoming request, any span that we've created prior won't belong to the same trace. I don't think the propagator should just return null here though. In OC we create a new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, an invalid span context would also even better, then the return type would be simply @toumorokoshi: The |
||||||
|
||||||
def inject( | ||||||
self, | ||||||
tracer: trace.Tracer, | ||||||
set_in_carrier: httptextformat.Setter, | ||||||
carrier: object, | ||||||
) -> None: | ||||||
"""Inject values from the current context into the carrier. | ||||||
|
||||||
inject enables the propagation of values into HTTP clients or | ||||||
other objects which perform an HTTP request. Implementations | ||||||
should use the set_in_carrier method to set values on the | ||||||
carrier. | ||||||
|
||||||
Args: | ||||||
set_in_carrier: A setter function that can set values | ||||||
on the carrier. | ||||||
carrier: An object that a place to define HTTP headers. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Or something similar, assuming this is a typo. |
||||||
Should be paired with set_in_carrier, which should | ||||||
know how to set header values on the carrier. | ||||||
""" | ||||||
self._httptextformat.inject( | ||||||
tracer.get_current_span().get_context(), set_in_carrier, carrier | ||||||
) | ||||||
|
||||||
|
||||||
_PROPAGATOR = Propagator(TraceStateHTTPTextFormat()) | ||||||
|
||||||
|
||||||
def get_global_propagator() -> Propagator: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know the getter / setter were supposed to be attached to the tracer. This would have created a cyclic dependency where Tracer (trace module) -> Propagator (propagator module) -> (trace module). I figure that there's a few RFCs floating around which are trying to tear propagators away from tracers anyway, so a separate global is as good as anywhere. |
||||||
return _PROPAGATOR | ||||||
|
||||||
|
||||||
def set_global_propagator(propagator: Propagator) -> None: | ||||||
global _PROPAGATOR # pylint:disable=global-statement | ||||||
_PROPAGATOR = propagator |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,57 @@ | ||
import unittest | ||
from unittest import mock | ||
|
||
import requests | ||
from werkzeug.test import Client | ||
from werkzeug.wrappers import BaseResponse | ||
|
||
import opentelemetry_example_app.flask_example as flask_example | ||
from opentelemetry.sdk import trace | ||
from opentelemetry.sdk.context.propagation import b3_format | ||
|
||
|
||
class TestFlaskExample(unittest.TestCase): | ||
@classmethod | ||
def setUpClass(cls): | ||
cls.app = flask_example.app | ||
|
||
def setUp(self): | ||
mocked_response = requests.models.Response() | ||
mocked_response.status_code = 200 | ||
mocked_response.reason = "Roger that!" | ||
self.send_patcher = mock.patch.object( | ||
requests.Session, | ||
"send", | ||
autospec=True, | ||
spec_set=True, | ||
return_value=mocked_response, | ||
) | ||
self.send = self.send_patcher.start() | ||
|
||
def tearDown(self): | ||
self.send_patcher.stop() | ||
|
||
def test_full_path(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is now a full-fledged propagation test. We could probably build on this once we have stuff like exporters. @c24t I think this will work for one of the test cases we discussed, although not as comprehensive as bringing up a full server. |
||
with self.app.test_client() as client: | ||
response = client.get("/") | ||
assert response.data.decode() == "hello" | ||
trace_id = trace.generate_trace_id() | ||
# We need to use the Werkzeug test app because | ||
# The headers are injected at the wsgi layer. | ||
# The flask test app will not include these, and | ||
# result in the values not propagated. | ||
client = Client(self.app.wsgi_app, BaseResponse) | ||
# emulate b3 headers | ||
client.get( | ||
"/", | ||
headers={ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use the B3 formatter's inject directly on the dict here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that might be a little tricky: it would require that I have direct access to the SpanContext for the app, which may not occur in situations where the app lives in a different thread or context than the test code itself. I feel like this is a more thorough test of the defined behavior, although I definitely see the merit of not effectively redefining the b3 interface. |
||
"x-b3-traceid": b3_format.format_trace_id(trace_id), | ||
"x-b3-spanid": b3_format.format_span_id( | ||
trace.generate_span_id() | ||
), | ||
"x-b3-sampled": "1", | ||
}, | ||
) | ||
# assert the http request header was propagated through. | ||
prepared_request = self.send.call_args[0][1] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be sure: This does not test the headers that were "sent" by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's not an easy way to differentiate between headers that were directly set by a user vs the headers that were set in the propagator: both are setting the headers keyword that is passed in as part of the request. Theoretically someone could modify the examples to send the same headers that the propagator is responsible for, but that's the not case today. Also the way that the integration is written, propagator headers will override any user-defined headers. |
||
headers = prepared_request.headers | ||
for required_header in {"x-b3-traceid", "x-b3-spanid", "x-b3-sampled"}: | ||
assert required_header in headers | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not using pytest yet, so please use unittest assertions, like |
||
assert headers["x-b3-traceid"] == b3_format.format_trace_id(trace_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
from opentelemetry import propagator
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I'd prefer
opentelemetry.propagation
, but that's based on personal taste ;)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then we've got
opentelemetry.context.propagation
andopentelemetry.propagation
, which is why I'd preferpropagators
if we keep this package structure....but in any case all I mean to complain about here is the
from x.y import y as x
import pattern. :PThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I can fix that. was not aware of the ability to import modules that way.