-
Notifications
You must be signed in to change notification settings - Fork 660
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
Adding a working propagator, adding to integrations and example #137
Conversation
Adding a full, end-to-end example of propagation at work in the example application, including a test. Adding the use of propagators into the integrations.
from opentelemetry.context.propagation import httptextformat | ||
|
||
|
||
class TraceStateHTTPTextFormat(httptextformat.HTTPTextFormat): |
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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't the TraceState
be extracted/injected along with the rest of the SpanContext
? Why separate it here?
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.
After reading the rest of the PR, I see that this might just be a naming issue. Would you expect SpanContextHTTPTextFormat
to describe the same thing?
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.
I think the proper name would be W3CTraceContextHTTPTextFormatter
(or some abbreviation thereof).
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.
@c24t @toumorokoshi
Just to clarify, the intent behind TraceStateHTTPTextFormat
is to actually cover SpanContext
right? So it should simply just be renamed?
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.
As I understand it, yes.
_PROPAGATOR = Propagator(TraceStateHTTPTextFormat()) | ||
|
||
|
||
def get_global_propagator() -> 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.
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.
|
||
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 comment
The 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.
|
||
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 comment
The 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 [None]
if the header is not found, which looks suspicious (it now returns typing.List[typing.Optional[str]]
, but probably it should do either typing.Optional[typing.List[str]]
or even just typing.List[str]
, returning an empty list if the header wasn't found).
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.
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 comment
The 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.
from opentelemetry.context.propagation import httptextformat | ||
|
||
|
||
class TraceStateHTTPTextFormat(httptextformat.HTTPTextFormat): |
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.
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.
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 comment
The 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 Tracer.start_span
. In that case, you might use the trace.ParentSpan
type alias.
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 return span_context
.
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.
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 comment
The 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 SpanContext
with new (valid) IDs when we can't extract the span context. I don't know that we want to do the same thing here, and may want to use e.g. the invalid span instead.
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.
True, an invalid span context would also even better, then the return type would be simply SpanContext
.
@toumorokoshi: The ParentSpan
type already includes None
, so putting it in an Optional
is redundant.
) | ||
|
||
|
||
class 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.
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 comment
The 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.
prepared_request = self.send.call_args[0][1] | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using pytest yet, so please use unittest assertions, like self.assertIn
.
}, | ||
) | ||
# 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 comment
The 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 client.get
but the headers that were sent by the example app using requests?
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.
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.
# emulate b3 headers | ||
client.get( | ||
"/", | ||
headers={ |
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.
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 comment
The 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.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of get + set, use kwargs.setdefault
.
@@ -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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue that either both should be called ParentContext, or ParentSpan.
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.
And since I trust that the relevant parts of my above review will be addressed if applicable, I give an advance approval to speed things up. 😃
Fixing incorrect type annotation for WSGI getter for HTTPTextFormatter. Using setdefautl to enable more atomic and simple setting of headers. Using UnitTest style assertions vs PyTest.
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.
There's a lot of ongoing discussion about context propagation, and a lot of context (ha!) that I'm likely missing as a reviewer here. It'd be helpful if you could add a summary of the decisions you made in this PR: what deviates from the existing context prop spec and other clients, what is and isn't likely to change, etc. It's easy enough to comment on the implementation, harder to understand what this PR means in light of the larger discussion.
@@ -22,6 +22,8 @@ | |||
|
|||
from requests.sessions import Session | |||
|
|||
import opentelemetry.propagator as 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.
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
and opentelemetry.propagation
, which is why I'd prefer propagators
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. :P
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.
yeah, I can fix that. was not aware of the ability to import modules that way.
@@ -0,0 +1,20 @@ | |||
import opentelemetry.trace as trace |
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.
Heads up that these new files need the license boilerplate.
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.
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 comment
The 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.
@@ -0,0 +1,77 @@ | |||
import typing |
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.
We probably want to call this package propagators
, plural like the others. FWIW this was called propagation
in OC, but was also under the trace
package.
from opentelemetry.context.propagation import httptextformat | ||
|
||
|
||
class TraceStateHTTPTextFormat(httptextformat.HTTPTextFormat): |
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.
Wouldn't the TraceState
be extracted/injected along with the rest of the SpanContext
? Why separate it here?
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
help simplify the work require for integrations to use the intended | |
help simplify the work required for integrations to use the intended |
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 comment
The reason will be displayed to describe this comment to others. Learn more.
@Oberon00 what are your thoughts on making the Getter
/Setter
types generic here to get rid of this carrier object
?
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks for the PR!
|
||
def extract( | ||
self, get_from_carrier: httptextformat.Getter, carrier: object | ||
) -> typing.Union[trace.SpanContext, trace.Span, None]: |
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.
We might have covered it in another PR, but what's the use case for returning a Span
here?
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.
this is what the ParentSpan type is a union of. But there's probably no good reason, it could just be SpanContext.
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 comment
The 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 SpanContext
with new (valid) IDs when we can't extract the span context. I don't know that we want to do the same thing here, and may want to use e.g. the invalid span instead.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
carrier: An object that a place to define HTTP headers. | |
carrier: An object defines HTTP headers. |
Or something similar, assuming this is a typo.
from opentelemetry.context.propagation import httptextformat | ||
|
||
|
||
class TraceStateHTTPTextFormat(httptextformat.HTTPTextFormat): |
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.
After reading the rest of the PR, I see that this might just be a naming issue. Would you expect SpanContextHTTPTextFormat
to describe the same thing?
Make getter/setter types specific to context
Regarding the naming of TraceStateHTTPTextFormatter: this is intended to be a placeholder for an httptextformatter that can propagate using the w3c tracestate spec: https://www.w3.org/TR/trace-context/ Which of course is named TraceContext. I'll rename the httptextformatter, but sorry if that's where the confusion came from. It's intended to be replaced by the PR for #116 |
Renaming propagator to plural, to match other top level module names. Using static functions for setting / getting global httptextformat, and making propagator methods static. Renaming "TraceStateHTTP" to "TraceContextHTTP", since the latter is the correct name for the w3c spec.
Please resolve merge conflicts. |
4c38989
to
607a794
Compare
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.
LGTM, thanks for fielding all the comments and adding context to the description.
Replacing Propagator
with static inject
and extract
funcs is a pretty big change, but I expect this module to change a lot more as we add the binary formats and the spec evolves. At the moment there's not much reason for this module since it's just passing calls through to the global TraceContextHTTPTextFormat
.
Yes, the intention is for this PR to seed some of the changes that will be required once oteps#42 lands. A layer on top of httptextformatter is valuable for that purpose. Thanks for reviewing! |
for start_time and end_time Make lint happy Addressing comments Addressing comments Allowing 0 as start and end time Fix lint issues Metrics API RFC 0003 cont'd (open-telemetry#136) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * address comments * fix comments Adding a working propagator, adding to integrations and example (open-telemetry#137) Adding a full, end-to-end example of propagation at work in the example application, including a test. Adding the use of propagators into the integrations. Metrics API RFC 0009 (open-telemetry#140) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * handle, recordbatch * docs * Update recordbatch * black * Fix typo * remove ValueType * fix lint Console exporter (open-telemetry#156) Make use_span more flexible (closes open-telemetry#147). (open-telemetry#154) Co-Authored-By: Reiley Yang <[email protected]> Co-Authored-By: Chris Kleinknecht <[email protected]> WSGI fixes (open-telemetry#148) Fix http.url. Don't delay calling wrapped app. Skeleton for azure monitor exporters (open-telemetry#151) Add link to docs to README (open-telemetry#170) Move example app to the examples folder (open-telemetry#172) WSGI: Fix port 80 always appended in http.host (open-telemetry#173) Build and host docs via github action (open-telemetry#167) Add missing license boilerplate to a few files (open-telemetry#176) sdk/trace/exporters: add batch span processor exporter (open-telemetry#153) The exporters specification states that two built-in span processors should be implemented, the simple processor span and the batch processor span. This commit implements the latter, it is mainly based on the opentelemetry/java one. The algorithm implements the following logic: - a condition variable is used to notify the worker thread in case the queue is half full, so that exporting can start before the queue gets full and spans are dropped. - export is called each schedule_delay_millis if there is a least one new span to export. - when the processor is shutdown all remaining spans are exported. Implementing W3C TraceContext (fixes open-telemetry#116) (open-telemetry#180) * Implementing TraceContext (fixes open-telemetry#116) This introduces a w3c TraceContext propagator, primarily inspired by opencensus. fix time conversion bug (open-telemetry#182) Introduce Context.suppress_instrumentation (open-telemetry#181) Metrics Implementation (open-telemetry#160) * Create functions Comments for Meter More comments Add more comments Fix typos * fix lint * Fix lint * fix typing * Remove options, constructors, seperate labels * Consistent naming for float and int * Abstract time series * Use ABC * Fix typo * Fix docs * seperate measure classes * Add examples * fix lint * Update to RFC 0003 * Add spancontext, measurebatch * Fix docs * Fix comments * fix lint * fix lint * fix lint * skip examples * white space * fix spacing * fix imports * fix imports * LabelValues to str * Black formatting * fix isort * Remove aggregation * Fix names * Remove aggregation from docs * Fix lint * metric changes * Typing * Fix lint * Fix lint * Add space * Fix lint * fix comments * handle, recordbatch * docs * Update recordbatch * black * Fix typo * remove ValueType * fix lint * sdk * metrics * example * counter * Tests * Address comments * ADd tests * Fix typing and examples * black * fix lint * remove override * Fix tests * mypy * fix lint * fix type * fix typing * fix tests * isort * isort * isort * isort * noop * lint * lint * fix tuple typing * fix type * black * address comments * fix type * fix lint * remove imports * default tests * fix lint * usse sequence * remove ellipses * remove ellipses * black * Fix typo * fix example * fix type * fix type * address comments Implement Azure Monitor Exporter (open-telemetry#175) Span add override parameters for start_time and end_time (open-telemetry#179) CONTRIBUTING.md: Fix clone URL (open-telemetry#177) Add B3 exporter to alpha release table (open-telemetry#164) Update README for alpha release (open-telemetry#189) Update Contributing.md doc (open-telemetry#194) Add **simple** client/server examples (open-telemetry#191) Remove unused dev-requirements.txt (open-telemetry#200) The requirements are contained in tox.ini now. Fx bug in BoundedList for Python 3.4 and add tests (open-telemetry#199) * fix bug in BoundedList for python 3.4 and add tests collections.deque.copy() was introduced in python 3.5, this commit changes that by the deque constructor and adds some tests to BoundedList and BoundedDict to avoid similar problems in the future. Also, improve docstrings of BoundedList and BoundedDict classes Move util.time_ns to API. (open-telemetry#205) Add Jaeger exporter (open-telemetry#174) This adds a Jeager exporter for OpenTelemetry. This exporter is based on https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-jaeger. The exporter uses thrift and can be configured to send data to the agent and also to a remote collector. There is a long discussion going on about how to include generated files in the repo, so for now just put them here. Add code coverage Revert latest commit Fix some "errors" found by mypy. (open-telemetry#204) Fix some errors found by mypy (split from open-telemetry#201). Update README for new milestones (open-telemetry#218) Refactor current span handling for newly created spans. (open-telemetry#198) 1. Make Tracer.start_span() simply create and start the Span, without setting it as the current instance. 2. Add an extra Tracer.start_as_current_span() to create the Span and set it as the current instance automatically. Co-Authored-By: Chris Kleinknecht <[email protected]> Add set_status to Span (open-telemetry#213) Initial commit Initial version
Adding a full, end-to-end example of propagation at work in the
example application, including a test.
Adding the use of propagators into the integrations.
EDIT: What does this contain that matches with the long-term spec?
In reality not a lot, this is pretty adherent to the existing specification.. But the changes outlined in the RFCs will require a while to land due to many outstanding design questions such as:
I think we should review this PR for the target of achieving the alpha. For the purpose of reviewing what will arrive in the RFC, there will be another PR once the spec is finalized or getting closer.