Skip to content

Add testbed for otel-ot-shim #274

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
Closed
wants to merge 8 commits into from
Closed

Add testbed for otel-ot-shim #274

wants to merge 8 commits into from

Conversation

mauriciovasquezbernal
Copy link
Member

This PR ports the OpenTracing testbed https://github.com/opentracing/opentracing-python/tree/master/testb to check that the ot-shim is working as expected using different frameworks.

Gevent doesn't support context vars yet gevent/gevent#1407, so those tests are not compatible with opentelemetry and were not ported.

@codecov-io
Copy link

codecov-io commented Nov 7, 2019 •
edited by codecov bot
Loading

Codecov Report

Merging #274 into master will decrease coverage by 4.58%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #274      +/-   ##
==========================================
- Coverage   89.50%   84.91%   -4.59%     
==========================================
  Files          43       38       -5     
  Lines        2229     1889     -340     
  Branches      248      217      -31     
==========================================
- Hits         1995     1604     -391     
- Misses        159      219      +60     
+ Partials       75       66       -9
Impacted Files Coverage Δ
...ry-sdk/src/opentelemetry/sdk/resources/__init__.py 70.83% <0.00%> (-22.03%) ⬇️
...elemetry-api/src/opentelemetry/metrics/__init__.py 87.93% <0.00%> (-5.03%) ⬇️
...opentelemetry/sdk/context/propagation/b3_format.py 84.61% <0.00%> (-2.66%) ⬇️
...sdk/src/opentelemetry/sdk/trace/export/__init__.py 86.79% <0.00%> (-2.57%) ⬇️
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py 85.18% <0.00%> (-2.39%) ⬇️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py 89.71% <0.00%> (-2.33%) ⬇️
...-ext-flask/src/opentelemetry/ext/flask/__init__.py 88.88% <0.00%> (-1.12%) ⬇️
...ntelemetry-api/src/opentelemetry/trace/__init__.py 84.56% <0.00%> (-0.25%) ⬇️
...src/opentelemetry/ext/opentracing_shim/__init__.py 95.90% <0.00%> (-0.14%) ⬇️
opentelemetry-api/src/opentelemetry/util/types.py 100.00% <0.00%> (ø)
... and 36 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 b799e54...8c6f945. Read the comment docs.

@carlosalberto
Copy link
Contributor

Hey, it looks overall good. I'm wondering about the updates for the build scripts, but I guess @c24t can ponder that ;)

@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/add_testbed_for_otshim branch from 2d30cca to c5ea413 Compare November 13, 2019 00:54
Oberon00
Oberon00 reviewed Nov 13, 2019
View reviewed changes
@carlosalberto
Copy link
Contributor

Did a second review and I feel it looks fine, but I'm wondering about the build/script part (such as https://github.com/open-telemetry/opentelemetry-python/pull/274/files#r345754382) - but other than that I'm fine with all of this.

@c24t if you feel the script changes are fine, lets go with it. Else, lets postpone merging it and have someone else working on this.

@toumorokoshi toumorokoshi added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Dec 9, 2019
@a-feld a-feld removed their request for review December 10, 2019 00:56
toumorokoshi
toumorokoshi suggested changes Dec 29, 2019
View reviewed changes
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.

Overall this looks good, and I feel like these patterns would be valuable to illustrate even for opentelemetry API examples.

There's a few parts that seem to not match the description, or I could be misunderstanding. I have comments around those parts.

@condorcet
Copy link

Please note supporting of contextvars in OT with relevant changes in testbed.

@condorcet
Copy link

I think we don't need to include tornado testbed here because of:

  1. Tornado < 6 has their own mechanism (stack_context) for storing context variables. In OT we using special scope manager based on this feature to store current context. This is the reason why we have testbed for tornado.
  2. OTel doesn't provide something like scope manager in OT. In terms of OT it's equal of using contextvars. We couldn't test this Tornado < 6 specific mechanism even if we want.
  3. Tornado >=6 is fully asyncio-based framework where their own stack_context was removed.

All this means that Tornado testbed is redundant and could be covered by contextvars testbed.
You can see in OT we don't include tornado testbed if Tornado >=6 https://github.com/opentracing/opentracing-python/blob/master/testbed/__main__.py#L14

@carlosalberto
Copy link
Contributor

Hey @condorcet Thanks for chiming it ;)

Yes, I think we should 'retire' the Tornado section, as it doesn't need its own thing and it's supposed to behave the same as asyncio, regarding to contextvars.

(I remember comments long, long time ago at being some differences between the Tornado and asyncio using contextvars, but couldn't find any note on this bit anymore ;) ).

@carlosalberto
Copy link
Contributor

Hey hey @toumorokoshi

So probably we ought to add a longer description + rationale about this testbed component. The idea is not to have it as a simple set of examples nor a test case suite, but rather as a set of (exoteric) examples that would allow us to validate active Span handling (when writing prototypes). Assertions are there as a matter of 'completeness' ;)

(I can tell you this were incredible useful back at the time of implementing this API, as we needed to support a few frameworks: tornado, gevent, asyncio, etc - good thing is that now we have contextvars!)

@mauriciovasquezbernal
ext/ot-shim: add testbed
54686a6 
This commit ports the OpenTracing testbed[1] to check that the ot-shim is
working as expected using different frameworks.

Gevent doesn't support context vars yet[2], so those tests are not compatible
with opentelemetry and were not ported.

[1] https://github.com/opentracing/opentracing-python/tree/master/testbed
[2] gevent/gevent#1407
@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/add_testbed_for_otshim branch from 91d547f to 54686a6 Compare January 15, 2020 14:38
@mauriciovasquezbernal mauriciovasquezbernal requested a review from a team January 15, 2020 14:38
@mauriciovasquezbernal
Copy link
Member Author

@condorcet @carlosalberto I'm going to remove the Tornado test cases then.

@mauriciovasquezbernal
remove tornado tests
f4b432c
@mauriciovasquezbernal
otel-ot-shim testbed: make asyncio good solution and document it
82a94ec 
In asyncio it is not needed to activate the span as the context is handled using
contextvars in this case. This commit makes that a good solution and adds some
comments to clarify why it is a good solution and why the thread is a bad one.
@mauriciovasquezbernal mauriciovasquezbernal force-pushed the mauricio/add_testbed_for_otshim branch from 4fcad3b to 82a94ec Compare January 16, 2020 15:48
@mauriciovasquezbernal mauriciovasquezbernal added the shim OpenTracing or OpenCensus compatibility label Jan 28, 2020
@codeboten codeboten requested a review from toumorokoshi May 21, 2020 16:39
@toumorokoshi toumorokoshi self-assigned this May 21, 2020
toumorokoshi
toumorokoshi approved these changes May 21, 2020
View reviewed changes
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.

Approving as I originally misunderstood the intent, and this is good code to validate opentracing that we should be adding into our suites. Thanks!

@toumorokoshi toumorokoshi mentioned this pull request May 24, 2020
Merged
@toumorokoshi
Copy link
Member

merged as part of #727. thanks!

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
@OlivierAlbertini
fix(core): tracerDelegate (open-telemetry#302)
a2541eb 
closes open-telemetry#274

Signed-off-by: Olivier Albertini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlosalberto carlosalberto carlosalberto approved these changes

@Oberon00 Oberon00 Oberon00 left review comments

@condorcet condorcet condorcet left review comments

@toumorokoshi toumorokoshi toumorokoshi approved these changes

@c24t c24t Awaiting requested review from c24t

@lzchen lzchen Awaiting requested review from lzchen

@reyang reyang Awaiting requested review from reyang

Assignees

@toumorokoshi toumorokoshi

Labels
hold needs reviewers PRs with this label are ready for review and needs people to review to move forward. shim OpenTracing or OpenCensus compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mauriciovasquezbernal @codecov-io @carlosalberto @condorcet @toumorokoshi @Oberon00