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

ext/opentracing-shim: implement inject and extract #256

Conversation

mauriciovasquezbernal
Copy link
Member

This commit implements inject() and extract() support for TEXT_MAP and
HTTP_HEADERS formats by using the configured OpenTelemetry propagators.

The support for binary propagators is not completed on opentelemetry-python
so this commit does not include for such format.

Solves: #243.

This commit implements inject() and extract() support for TEXT_MAP and
HTTP_HEADERS formats by using the configured OpenTelemetry propagators.

The support for binary propagators is not completed on opentelemetry-python
 so this commit does not include for such format.
self.assertEqual(
http_headers[MockHTTPTextFormat.SPAN_ID_KEY], str(7478)
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have the error as a separated test case? ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

I separated different formats in different test cases in 30de890.

self.shim.extract(opentracing.Format.BINARY, bytearray())


class MockHTTPTextFormat(HTTPTextFormat):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming it's not so trivial to verify against the actual builtin format(s)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not that hard actually but I wanted to keep these tests independently from actual implementations. Of course I am open to discuss why it could be better to test with the actual implementation instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, but lets wait for others to chime in ;)

Handle different formats in different test cases.
@carlosalberto carlosalberto self-requested a review November 5, 2019 17:37
@carlosalberto
Copy link
Contributor

Let's get this rolling!

…pentracing_shim/__init__.py

Co-Authored-By: Christian Neumüller <christian+github@neumueller.me>
@codecov-io
Copy link

codecov-io commented Nov 8, 2019

Codecov Report

Merging #256 into master will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage    85.5%   85.74%   +0.23%     
==========================================
  Files          33       33              
  Lines        1608     1620      +12     
  Branches      181      183       +2     
==========================================
+ Hits         1375     1389      +14     
+ Misses        186      184       -2     
  Partials       47       47
Impacted Files Coverage Δ
...src/opentelemetry/ext/opentracing_shim/__init__.py 95.9% <100%> (+2.26%) ⬆️

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 ff63d8c...cea2918. Read the comment docs.

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

@c24t
Copy link
Member

c24t commented Nov 11, 2019

@mauriciovasquezbernal looks like I don't have permission to push to the branch. I can merge this after you pick up master:

diff --cc ext/opentelemetry-ext-opentracing-shim/CHANGELOG.md
index 34017bb,f059e90..8fe7c21
--- a/ext/opentelemetry-ext-opentracing-shim/CHANGELOG.md
+++ b/ext/opentelemetry-ext-opentracing-shim/CHANGELOG.md
@@@ -2,9 -2,7 +2,9 @@@

  ## Unreleased

 +- Implement extract and inject support for HTTP_HEADERS and TEXT_MAP formats.
 +
- ## 0.2a.0
+ ## 0.2a0

  Released 2019-10-29

diff --cc ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py
index e2185b9,7271d79..dae4d84
--- a/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py
+++ b/ext/opentelemetry-ext-opentracing-shim/src/opentelemetry/ext/opentracing_shim/__init__.py
@@@ -15,8 -15,8 +15,9 @@@
  import logging

  import opentracing
+ from deprecated import deprecated

 +from opentelemetry import propagators
  from opentelemetry.ext.opentracing_shim import util

  logger = logging.getLogger(__name__)

@mauriciovasquezbernal
Copy link
Member Author

@c24t done!, not sure why you cannot push to my branch, Allow edits from maintainers. is enabled.

@c24t c24t merged commit fcdd9fe into open-telemetry:master Nov 12, 2019
clsung pushed a commit to clsung/opentelemetry-python that referenced this pull request Nov 14, 2019
This commit implements inject() and extract() support for TEXT_MAP and
HTTP_HEADERS formats by using the configured OpenTelemetry propagators.

The support for binary propagators is not completed on opentelemetry-python so
this commit does not include for such format.
@mauriciovasquezbernal mauriciovasquezbernal deleted the mauricio/implement_extract_and_inject_ot_shim branch April 14, 2020 21:51
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* chore: code cleanup

* fix: build
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.

5 participants