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.

Sign up for GitHub

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

Jump to bottom

Adding Correlation Context API and propagator #471

Merged
merged 20 commits into from
Mar 16, 2020
Merged

Adding Correlation Context API and propagator #471

merged 20 commits into from
Mar 16, 2020

Conversation

codeboten
Copy link
Contributor

@codeboten codeboten commented Mar 9, 2020 •
edited
Loading

This change removes Distributed Context and replaces it with the Correlations Context API. This change also adds the Correlation Context Propagator to the global httptextformat propagator.

Fixes #416

Signed-off-by: Alex Boten aboten@lightstep.com

@codeboten codeboten requested a review from a team March 9, 2020 21:24
@codeboten codeboten changed the title Initial commit for Correlation Context API WIP: Initial commit for Correlation Context API Mar 9, 2020
@codeboten codeboten added the WIP Work in progress label Mar 9, 2020
@ocelotl
Initial commit for Correlation Context API
4d595f8 
This change removes Distributed Context and replaces it with the Correlations Context API. Things to do:
- add more tests
- implement correlation context propagation and add it to the default propagator

Signed-off-by: Alex Boten <aboten@lightstep.com>
@ocelotl ocelotl force-pushed the rename-distributed-context branch from 18dd676 to 4d595f8 Compare March 10, 2020 19:59
@codeboten codeboten changed the title WIP: Initial commit for Correlation Context API Initial commit for Correlation Context API Mar 12, 2020
@codeboten codeboten removed the WIP Work in progress label Mar 12, 2020
@codeboten codeboten changed the title Initial commit for Correlation Context API Adding Correlation Context API and propagator Mar 12, 2020
@codecov-io
Copy link

codecov-io commented Mar 12, 2020 •
edited
Loading

Codecov Report

Merging #471 into master will increase coverage by 0.14%.
The diff coverage is 97.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
+ Coverage   89.20%   89.34%   +0.14%     
==========================================
  Files          43       43              
  Lines        2177     2178       +1     
  Branches      243      248       +5     
==========================================
+ Hits         1942     1946       +4     
+ Misses        165      161       -4     
- Partials       70       71       +1
Impacted Files Coverage Δ
...lemetry/correlationcontext/propagation/__init__.py 95.45% <95.45%> (ø)
...i/src/opentelemetry/correlationcontext/__init__.py 100.00% <100.00%> (ø)
...etry-api/src/opentelemetry/propagators/__init__.py 94.44% <100.00%> (+19.44%) ⬆️

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 f52468b...39192ab. Read the comment docs.

mauriciovasquezbernal
mauriciovasquezbernal suggested changes Mar 13, 2020
View reviewed changes
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.

Overall looks good. I'm asking changes due to the fact that correlation context operations are modifying the value stores in the context.

opentelemetry-api/tests/correlationcontext/test_correlation_context.py Outdated Show resolved Hide resolved
opentelemetry-api/tests/correlationcontext/test_correlation_context.py Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/correlationcontext/propagation/__init__.py Outdated Show resolved Hide resolved
opentelemetry-api/tests/correlationcontext/test_correlation_context_propagation.py Outdated Show resolved Hide resolved
opentelemetry-api/tests/correlationcontext/test_correlation_context_propagation.py Outdated Show resolved Hide resolved
opentelemetry-api/tests/correlationcontext/test_correlation_context_propagation.py Show resolved Hide resolved
opentelemetry-api/tests/correlationcontext/test_correlation_context_propagation.py Show resolved Hide resolved
@codeboten codeboten added api Affects the API package. needs reviewers PRs with this label are ready for review and needs people to review to move forward. labels Mar 13, 2020
mauriciovasquezbernal
mauriciovasquezbernal approved these changes Mar 13, 2020
View reviewed changes
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.

LGTM, thanks for handling all my comments.

ocelotl
ocelotl suggested changes Mar 14, 2020
View reviewed changes
opentelemetry-api/src/opentelemetry/correlationcontext/propagation/__init__.py Outdated
See `opentelemetry.trace.propagation.httptextformat.HTTPTextFormat.extract`
"""

if not context:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if not context:
if context is not None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean if context is None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed this to check for None

opentelemetry-api/src/opentelemetry/correlationcontext/__init__.py Outdated
from opentelemetry.context import get_value, set_value
from opentelemetry.context.context import Context

CORRELATION_CONTEXT_KEY = "correlation-context"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be made private?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

opentelemetry-api/src/opentelemetry/correlationcontext/propagation/__init__.py Show resolved Hide resolved
opentelemetry-api/src/opentelemetry/correlationcontext/propagation/__init__.py Show resolved Hide resolved
opentelemetry-api/tests/correlationcontext/test_correlation_context.py Show resolved Hide resolved
@c24t c24t mentioned this pull request Mar 14, 2020
Merged
c24t
c24t approved these changes Mar 15, 2020
View reviewed changes
Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

LGTM, well written and great tests.

@codeboten codeboten requested a review from ocelotl March 15, 2020 19:27
@codeboten
Copy link
Contributor Author

Thanks for the reviews! @ocelotl are there anymore blocking changes?

@c24t c24t merged commit 264e6c3 into open-telemetry:master Mar 16, 2020
toumorokoshi added a commit to toumorokoshi/opentelemetry-python that referenced this pull request Mar 16, 2020
@toumorokoshi
Adding changelog entries for the 0.5b0 release
664b86d 
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 toumorokoshi mentioned this pull request Mar 16, 2020
Merged
c24t pushed a commit to c24t/opentelemetry-python that referenced this pull request Mar 16, 2020
@toumorokoshi @c24t
Adding changelog entries for the 0.5b0 release
8004571 
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.
@ffe4 ffe4 mentioned this pull request Aug 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mauriciovasquezbernal mauriciovasquezbernal mauriciovasquezbernal approved these changes

@c24t c24t c24t approved these changes

@ocelotl ocelotl ocelotl approved these changes

Assignees
No one assigned
Labels
api Affects the API package. needs reviewers PRs with this label are ready for review and needs people to review to move forward.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Correlations API
5 participants
@codeboten @codecov-io @ocelotl @mauriciovasquezbernal @c24t