Use opentracing 2.0 #206

mihau · 2018-08-08T13:27:12Z

This pull request bumps opntracing to 2.0 and implements changes needed for api compatibility (mainly regarding scope managers). I assumed compatibility with 2.x only, but maybe it would be possible to run it with 1.x as well, not sure if that is a good idea though.
I used ThreadLocalScopeManager from opentracing as a default scope manager.

The tests may fail because of an opentracing issue I fixed in opentracing/opentracing-python#97

Resolves #199

black-adder

thanks for getting started on this!

jaeger_client/tracer.py

setup.py

jaeger_client/tracer.py

tovin07

Thank you for doing this PR, I just have some small comments.

tests/test_api.py

jaeger_client/tracer.py

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

Co-Authored-By: mihau <mszymanski@vewd.com> Signed-off-by: Michał Szymański <mszymanski@vewd.com>

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

codecov · 2019-04-12T19:49:16Z

Codecov Report

Merging #206 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   94.63%   94.65%   +0.02%     
==========================================
  Files          25       25              
  Lines        1902     1910       +8     
  Branches      256      257       +1     
==========================================
+ Hits         1800     1808       +8     
  Misses         67       67              
  Partials       35       35

Impacted Files	Coverage Δ
jaeger_client/tracer.py	`99.34% <100%> (+0.03%)`	⬆️
jaeger_client/config.py	`91.66% <100%> (+0.04%)`	⬆️

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 9771cc9...b4e2d10. Read the comment docs.

mihau · 2019-04-12T20:09:51Z

I've spotted an issue with what I believe is a problem of opentracing-python tests, which led me to overriding a test in this commit.
I opened an issue in opentracing-python to figure out what to do with it.

jaeger_client/config.py

yurishkuro · 2019-04-24T19:07:16Z

setup.py

@@ -40,7 +40,7 @@
        'threadloop>=1,<2',
        'thrift',
        'tornado>=4.3,<5',
-        'opentracing>=1.2.2,<2',
+        'opentracing>=2.0,<3.0',


pleasantly surprised that crossdock test works with 'opentracing_instrumentation>=2,<3'

yurishkuro · 2019-04-24T19:34:20Z

Aside from the scope manager passed as class vs. instance, this lgtm otherwise (i.e. WIP can be removed).

mihau · 2019-04-24T19:40:53Z

@yurishkuro and how do you feel about this commit do we leave it, or do we get it fixed in python-opentracing first?

yurishkuro · 2019-04-24T19:45:23Z

I think it's ok to leave it. Is there a ticket for opentracing-python? We should added it to the comments under // TODO remove after {ticket} is fixed

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

mihau · 2019-04-24T20:32:33Z

Ok, cool. I've added the TODO note. Removing the WIP status.

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

yurishkuro · 2019-04-24T22:17:36Z

jaeger_client/tracer.py


        :return: Returns an already-started Span instance.
        """
        parent = child_of
+
+        if self.active_span is not None and not ignore_active_span:


I think it also needs and not parent, because if child_of is passed explicitly, the active span should not be inherited (or am I mistaken?)

I think you're right, it makes sense. Although one could use ignore_active_span together with child_of to pass it explicitly. Not exactly intuitive but still an option. The tests don't seem to enforce it in any way (maybe it would be appropriate to add one to do so?).
I added the condition, you might find the formatting controversial (multiline if), let me know if you prefer a different way.

jaeger_client/tracer.py

tests/test_api.py

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro

Thanks for the PR!

mihau force-pushed the feature/opentracing-2.0-support branch from 1a86904 to 21e1ae5 Compare August 8, 2018 13:34

mihau mentioned this pull request Aug 8, 2018

Support OpenTracing-2.0.0 #199

Closed

black-adder reviewed Aug 8, 2018

View reviewed changes

jaeger_client/tracer.py Outdated Show resolved Hide resolved

setup.py Outdated Show resolved Hide resolved

jaeger_client/tracer.py Show resolved Hide resolved

mihau force-pushed the feature/opentracing-2.0-support branch from beb54a7 to 54c5e78 Compare August 9, 2018 13:35

tovin07 reviewed Mar 6, 2019

View reviewed changes

tests/test_api.py Show resolved Hide resolved

jaeger_client/tracer.py Outdated Show resolved Hide resolved

jaeger_client/tracer.py Outdated Show resolved Hide resolved

jaeger_client/tracer.py Outdated Show resolved Hide resolved

Michał Szymański added 4 commits April 12, 2019 16:50

Use opentracing 2.0

17ce0f2

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

remove redundant assignment

4239c86

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

use opentracing in any 2.x version

abcc40d

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

allow specifying scope manager in config

6a15518

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

mihau force-pushed the feature/opentracing-2.0-support branch from 028213f to 6a15518 Compare April 12, 2019 14:59

Apply suggestions from code review: formatting fixes

ef0dc0c

Co-Authored-By: mihau <mszymanski@vewd.com> Signed-off-by: Michał Szymański <mszymanski@vewd.com>

mihau force-pushed the feature/opentracing-2.0-support branch from 5b4a6f3 to ef0dc0c Compare April 12, 2019 15:06

remove unnecessary new line

e58ffd0

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

mihau force-pushed the feature/opentracing-2.0-support branch from 549df3e to e58ffd0 Compare April 12, 2019 15:13

Michał Szymański added 2 commits April 12, 2019 21:30

override test_start_activ_span

2a2e8a3

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

fix linter issues

dbd47f8

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

mihau force-pushed the feature/opentracing-2.0-support branch from dcb03ab to dbd47f8 Compare April 12, 2019 19:45

yurishkuro reviewed Apr 24, 2019

View reviewed changes

jaeger_client/config.py Outdated Show resolved Hide resolved

yurishkuro reviewed Apr 24, 2019

View reviewed changes

Michał Szymański added 2 commits April 24, 2019 22:06

pass an instance of a scope manager instead of a class

8fb2a74

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

add a TODO to remove api tests override

be9a70c

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

mihau force-pushed the feature/opentracing-2.0-support branch from bef78c5 to be9a70c Compare April 24, 2019 20:30

mihau changed the title ~~WIP Use opentracing 2.0~~ Use opentracing 2.0 Apr 24, 2019

remove trailing whitespace

87bfa6b

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

Merge branch 'master' into feature/opentracing-2.0-support

2deb3a0

yurishkuro reviewed Apr 24, 2019

View reviewed changes

jaeger_client/tracer.py Outdated Show resolved Hide resolved

yurishkuro reviewed Apr 24, 2019

View reviewed changes

tests/test_api.py Outdated Show resolved Hide resolved

Michał Szymański and others added 6 commits April 26, 2019 18:59

reference opentracing-python release in TODO message

48327b8

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

use named arguments

75884fc

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

don't use active span if parent is passed explicitly

98e53bb

Signed-off-by: Michał Szymański <mszymanski@vewd.com>

Fix lint

c4f829d

Signed-off-by: Yuri Shkuro <ys@uber.com>

Merge branch 'master' into feature/opentracing-2.0-support

490451b

Signed-off-by: Yuri Shkuro <ys@uber.com>

Bump OT to 2.1 and remove test override

b4e2d10

Signed-off-by: Yuri Shkuro <ys@uber.com>

yurishkuro approved these changes Apr 27, 2019

View reviewed changes

yurishkuro merged commit 5a237ce into jaegertracing:master Apr 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use opentracing 2.0 #206

Use opentracing 2.0 #206

mihau commented Aug 8, 2018 •

edited by yurishkuro

Loading

black-adder left a comment

tovin07 left a comment

codecov bot commented Apr 12, 2019 •

edited

Loading

mihau commented Apr 12, 2019

yurishkuro Apr 24, 2019

yurishkuro commented Apr 24, 2019

mihau commented Apr 24, 2019

yurishkuro commented Apr 24, 2019 •

edited

Loading

mihau commented Apr 24, 2019

yurishkuro Apr 24, 2019

mihau Apr 26, 2019

yurishkuro left a comment

Use opentracing 2.0 #206

Use opentracing 2.0 #206

Conversation

mihau commented Aug 8, 2018 • edited by yurishkuro Loading

black-adder left a comment

Choose a reason for hiding this comment

tovin07 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2019 • edited Loading

Codecov Report

mihau commented Apr 12, 2019

yurishkuro Apr 24, 2019

Choose a reason for hiding this comment

yurishkuro commented Apr 24, 2019

mihau commented Apr 24, 2019

yurishkuro commented Apr 24, 2019 • edited Loading

mihau commented Apr 24, 2019

yurishkuro Apr 24, 2019

Choose a reason for hiding this comment

mihau Apr 26, 2019

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

mihau commented Aug 8, 2018 •

edited by yurishkuro

Loading

codecov bot commented Apr 12, 2019 •

edited

Loading

yurishkuro commented Apr 24, 2019 •

edited

Loading