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

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Feb 22, 2020

As per the the Propagators API spec: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-propagators.md, the propagation API now uses the Context API as a mechanism for propagation.

As part of this change:

  • inject/extract now take an optional context parameter
  • extract now returns a Context object
  • removed binary format as they're not currently in the spec
  • added CompositePropagator to support configuring multiple propagators for the same format

Fixes #415

@codeboten codeboten added the WIP Work in progress label Feb 22, 2020
@codeboten codeboten requested a review from a team February 22, 2020 01:07
@codecov-io
Copy link

codecov-io commented Feb 22, 2020

Codecov Report

Merging #446 into master will decrease coverage by 0.11%.
The diff coverage is 91.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #446      +/-   ##
==========================================
- Coverage   88.78%   88.66%   -0.12%     
==========================================
  Files          42       43       +1     
  Lines        2167     2206      +39     
  Branches      246      249       +3     
==========================================
+ Hits         1924     1956      +32     
- Misses        170      176       +6     
- Partials       73       74       +1
Impacted Files Coverage Δ
...opentelemetry/sdk/context/propagation/b3_format.py 87.27% <100%> (ø) ⬆️
...try-api/src/opentelemetry/propagators/composite.py 100% <100%> (ø)
...-ext-flask/src/opentelemetry/ext/flask/__init__.py 89.79% <100%> (+0.9%) ⬆️
...ts/src/opentelemetry/ext/http_requests/__init__.py 89.47% <100%> (ø) ⬆️
...src/opentelemetry/ext/opentracing_shim/__init__.py 96.03% <100%> (+0.09%) ⬆️
.../opentelemetry/trace/propagation/httptextformat.py 100% <100%> (ø)
...etry-api/src/opentelemetry/propagators/__init__.py 75% <71.42%> (ø)
...pi/src/opentelemetry/trace/propagation/__init__.py 84.61% <81.81%> (ø)
...ry/trace/propagation/tracecontexthttptextformat.py 84.28% <85.71%> (ø)
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py 68.14% <85.71%> (-0.05%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 005575e...90c0535. Read the comment docs.

@@ -13,7 +13,22 @@
# limitations under the License.
from typing import Optional

from opentelemetry.trace import INVALID_SPAN_CONTEXT, Span, SpanContext
from opentelemetry import trace as trace_api
from opentelemetry.context import get_value, set_current, set_value
Copy link
Member

Choose a reason for hiding this comment

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

set_current is unused. I wondered why pyflakes caught this and not pylint -- it looks like we ignore unused imports in init modules by default.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Nice! A couple questions, one those are discussed I think I'm happy to move to approved.

One thing I think that really needs some thought is whether context extracted from propagators should be attached as well. Probably the main blocker for me.

@@ -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.

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.

@toumorokoshi toumorokoshi added this to the Beta milestone Feb 27, 2020
@codeboten codeboten changed the title [WIP] Implementing Propagators API to use Context Implementing Propagators API to use Context Mar 5, 2020
@codeboten codeboten removed the WIP Work in progress label Mar 5, 2020
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

The PR overall looks pretty good! I have some comments, many of them are just details.

There is one thing I noticed after this PR, extract now returns a Context with a DefaultSpan inside instead of a SpanContext. If I am not wrong, the propagators were one of the main consumers of the API that allows to create a span passing a SpanContext as parent.

Maybe it's time to consider moving ahead with #345.

@@ -28,7 +32,8 @@ def get_as_list(dict_object, key):

def get_child_parent_new_carrier(old_carrier):

parent_context = FORMAT.extract(get_as_list, old_carrier)
ctx = FORMAT.extract(get_as_list, old_carrier)
parent_context = get_span_from_context(ctx).get_context()

Choose a reason for hiding this comment

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

Not related to this PR: I think we'll have to rename get_context() to get_spancontext(). I'm already confused with Context and SpanContext being two very different things.

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 agree, this would be good to update. Think there was a similar comment made previously.

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

An extra couple of comments. Once documentation is updated it should be ok from my side.

opentelemetry-api/tests/propagators/test_composite.py Outdated Show resolved Hide resolved
opentelemetry-api/tests/propagators/test_composite.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

LGTM! Just minor changes/suggestions 👍

# limitations under the License.

import unittest
from logging import ERROR
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed!

Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking into consideration all my comments. LGTM!

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

logger = logging.getLogger(__name__)


class CompositeHTTPPropagator(httptextformat.HTTPTextFormat):
Copy link
Member

Choose a reason for hiding this comment

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

I recall these are called "Chained" propagators in the OTEP. Not sure if anything official made it to the spec.

Can you confirm that the naming is consistent? if you haven't already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@toumorokoshi toumorokoshi merged commit 9ed98eb into open-telemetry:master Mar 6, 2020
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 6, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

Fix new ext READMEs (open-telemetry#444)

Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with:

HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/

Adding attach/detach methods as per spec (open-telemetry#429)

This change updates the Context API with the following:

- removes the remove_value method
- removes the set_current method
- adds attach and detach methods

Fixes open-telemetry#420

Co-authored-by: Chris Kleinknecht <[email protected]>

Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439)

OT Collector trace exporter (open-telemetry#405)

Based on the OpenCensus agent exporter.

Fixes open-telemetry#343

Co-authored-by: Chris Kleinknecht <[email protected]>

API: Renaming TraceOptions to TraceFlags (open-telemetry#450)

Renaming TraceOptions to TraceFlags, which is the term used to describe the
flags associated with the trace in the OpenTelemetry specification.

Closes open-telemetry#434

api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431)

Implements open-telemetry#221.
Also fixes open-telemetry#394.

Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them.

Lastly, moves InstrumentationInfo from trace.py in the sdk to utils.

Prepare to host on readthedocs.org (open-telemetry#452)

sdk: Implement observer instrument (open-telemetry#425)

Observer instruments are used to capture a current set of values at a point in
time [1].

This commit extends the Meter interface to allow to register an observer
instrument by pasing a callback that will be executed at collection time.
The logic inside collection is updated to consider these instruments and
a new ObserverAggregator is implemented.

[1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments

sdk: fix ConsoleSpanExporter (open-telemetry#455)

19d573a ("Add io and formatter options to console exporter (open-telemetry#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.

jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459)

Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already.

Users following the usage docs will currently run into this error:
`AttributeError: 'Tracer' object has no attribute 'add_span_processor'`

api: Implementing Propagators API to use Context  (open-telemetry#446)

Implementing Propagators API to use Context.

Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation.

Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces.

Adding a composite propagator.

Co-authored-by: Mauricio Vásquez <[email protected]>
toumorokoshi added a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 16, 2020
Total Changelog:

Documentations has been significantly overhauled, including:

* a getting started guide
* examples has been consolidated to an docs/examples folder
* several minor improvements to the examples in each extension's readme.

- Adding Correlation Context API and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding a global configuration module to simplify setting and getting
  globals
  ([open-telemetry#466](open-telemetry#466))
- Rename metric handle to bound metric
  ([open-telemetry#470](open-telemetry#470))
- Moving resources to sdk
  ([open-telemetry#464](open-telemetry#464))
- Implementing propagators to API to use context
  ([open-telemetry#446](open-telemetry#446))
- Implement observer instrument for metrics
  ([open-telemetry#425](open-telemetry#425))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Renaming TraceOptions to TraceFlags
  ([open-telemetry#450](open-telemetry#450))
- Renaming TracerSource to TraceProvider
  ([open-telemetry#441](open-telemetry#441))

- Adding Correlation Context SDK and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding OT Collector metrics exporter
  ([open-telemetry#454](open-telemetry#454))
- Improve validation of attributes
  ([open-telemetry#460](open-telemetry#460))
- Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span()
  (open-telemetry#469)
  ([open-telemetry#469](open-telemetry#469))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Make counter and MinMaxSumCount aggregators thread safe
  ([open-telemetry#439](open-telemetry#439))

- Initial release. Support is included for both trace and metrics.
c24t pushed a commit to c24t/opentelemetry-python that referenced this pull request Mar 16, 2020
Total Changelog:

Documentations has been significantly overhauled, including:

* a getting started guide
* examples has been consolidated to an docs/examples folder
* several minor improvements to the examples in each extension's readme.

- Adding Correlation Context API and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding a global configuration module to simplify setting and getting
  globals
  ([open-telemetry#466](open-telemetry#466))
- Rename metric handle to bound metric
  ([open-telemetry#470](open-telemetry#470))
- Moving resources to sdk
  ([open-telemetry#464](open-telemetry#464))
- Implementing propagators to API to use context
  ([open-telemetry#446](open-telemetry#446))
- Implement observer instrument for metrics
  ([open-telemetry#425](open-telemetry#425))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Renaming TraceOptions to TraceFlags
  ([open-telemetry#450](open-telemetry#450))
- Renaming TracerSource to TraceProvider
  ([open-telemetry#441](open-telemetry#441))

- Adding Correlation Context SDK and propagator
  ([open-telemetry#471](open-telemetry#471))
- Adding OT Collector metrics exporter
  ([open-telemetry#454](open-telemetry#454))
- Improve validation of attributes
  ([open-telemetry#460](open-telemetry#460))
- Re-raise errors caught in opentelemetry.sdk.trace.Tracer.use_span()
  (open-telemetry#469)
  ([open-telemetry#469](open-telemetry#469))
- Adding named meters, removing batchers
  ([open-telemetry#431](open-telemetry#431))
- Make counter and MinMaxSumCount aggregators thread safe
  ([open-telemetry#439](open-telemetry#439))

- Initial release. Support is included for both trace and metrics.
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 17, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

Fix new ext READMEs (open-telemetry#444)

Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with:

HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/

Adding attach/detach methods as per spec (open-telemetry#429)

This change updates the Context API with the following:

- removes the remove_value method
- removes the set_current method
- adds attach and detach methods

Fixes open-telemetry#420

Co-authored-by: Chris Kleinknecht <[email protected]>

Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439)

OT Collector trace exporter (open-telemetry#405)

Based on the OpenCensus agent exporter.

Fixes open-telemetry#343

Co-authored-by: Chris Kleinknecht <[email protected]>

API: Renaming TraceOptions to TraceFlags (open-telemetry#450)

Renaming TraceOptions to TraceFlags, which is the term used to describe the
flags associated with the trace in the OpenTelemetry specification.

Closes open-telemetry#434

api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431)

Implements open-telemetry#221.
Also fixes open-telemetry#394.

Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them.

Lastly, moves InstrumentationInfo from trace.py in the sdk to utils.

Prepare to host on readthedocs.org (open-telemetry#452)

sdk: Implement observer instrument (open-telemetry#425)

Observer instruments are used to capture a current set of values at a point in
time [1].

This commit extends the Meter interface to allow to register an observer
instrument by pasing a callback that will be executed at collection time.
The logic inside collection is updated to consider these instruments and
a new ObserverAggregator is implemented.

[1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments

sdk: fix ConsoleSpanExporter (open-telemetry#455)

19d573a ("Add io and formatter options to console exporter (open-telemetry#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.

jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459)

Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already.

Users following the usage docs will currently run into this error:
`AttributeError: 'Tracer' object has no attribute 'add_span_processor'`

api: Implementing Propagators API to use Context  (open-telemetry#446)

Implementing Propagators API to use Context.

Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation.

Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces.

Adding a composite propagator.

Co-authored-by: Mauricio Vásquez <[email protected]>
toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 26, 2020
Rename TracerSource to TracerProvider (open-telemetry#441)

Following discussion in open-telemetry#434, align the name with the specification.

Co-authored-by: Chris Kleinknecht <[email protected]>

Fix new ext READMEs (open-telemetry#444)

Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with:

HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/

Adding attach/detach methods as per spec (open-telemetry#429)

This change updates the Context API with the following:

- removes the remove_value method
- removes the set_current method
- adds attach and detach methods

Fixes open-telemetry#420

Co-authored-by: Chris Kleinknecht <[email protected]>

Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439)

OT Collector trace exporter (open-telemetry#405)

Based on the OpenCensus agent exporter.

Fixes open-telemetry#343

Co-authored-by: Chris Kleinknecht <[email protected]>

API: Renaming TraceOptions to TraceFlags (open-telemetry#450)

Renaming TraceOptions to TraceFlags, which is the term used to describe the
flags associated with the trace in the OpenTelemetry specification.

Closes open-telemetry#434

api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431)

Implements open-telemetry#221.
Also fixes open-telemetry#394.

Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them.

Lastly, moves InstrumentationInfo from trace.py in the sdk to utils.

Prepare to host on readthedocs.org (open-telemetry#452)

sdk: Implement observer instrument (open-telemetry#425)

Observer instruments are used to capture a current set of values at a point in
time [1].

This commit extends the Meter interface to allow to register an observer
instrument by pasing a callback that will be executed at collection time.
The logic inside collection is updated to consider these instruments and
a new ObserverAggregator is implemented.

[1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments

sdk: fix ConsoleSpanExporter (open-telemetry#455)

19d573a ("Add io and formatter options to console exporter (open-telemetry#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.

jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459)

Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already.

Users following the usage docs will currently run into this error:
`AttributeError: 'Tracer' object has no attribute 'add_span_processor'`

api: Implementing Propagators API to use Context  (open-telemetry#446)

Implementing Propagators API to use Context.

Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation.

Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces.

Adding a composite propagator.

Co-authored-by: Mauricio Vásquez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement the Propagation API
6 participants