2.0.0 Release

[carlosalberto]

This issue tracks the push for releasing the new 2.0.0 API for OpenTracing-Python.

Based on the Java 0.31 API, the next version of the Python edition of OpenTracing is now available and ready for testing.

RC Branch: https://github.com/opentracing/opentracing-python/tree/v2.0.0

Current Status

• The Scope concept has been brought from the Java API, and is being validated.
• Tracer.start_active_span() defaults finish_on_close=True, after feedback received for the first Release Candidate.
• start_active_span has been defined to have operation_name as a required parameter, as opposed to how it's handled in start_span.
• Properties have been used for new members (such as Scope.span), following the current style (Span.context, for example).
• Imported a testbed suite, which includes a few instrumentation patterns, so API changes and experimental features can be tested out there. This is the equivalent to opentracing-testbed for the Java API.
• Documentation may need to be re-reviewed and extended.
• basictracer-python has been updated as well.

Release process

• The new API is being developed on the v2.0.0 branch.
• Revisions should be requested as Pull Requests against this branch.
• master will continue to point at the latest patch version of the v2.0.0 branch, until 2.0.0 is officially released.
• Changes to the release candidate will be released on PyPi as 2.0.0.rc1, 2.0.0.rc2, etc.

Current RC snapshots

• https://pypi.python.org/pypi/opentracing/2.0.0rc2
• https://pypi.python.org/pypi/basictracer/3.0.0rc2

[palazzem]

Follow-up of a concern I had while discussing the PR referenced in this issue (#64):

• Implement ScopeManager for in-process propagation (updated) #64 (comment)
• Implement ScopeManager for in-process propagation (updated) #64 (comment)

After thinking about it and taking in consideration use cases we have, I would like to continue the discussion here about the current implementation of start_active_span (code). In general, I think we should not expose an advanced functionality (closing or not closing a Scope) to end-users (developers), considering a wrong usage of this functionality will definitely lead to mistakes and wrong/missing/hard to debug traces.

I would prefer to expose the functionality for Tracer implementers (or contrib modules where we can make it explicit all the time) and to end-users with specific edge cases (i.e. using callbacks, asyncio loop with detached executions etc...).

Said that, if my first point makes sense, we should consider having a default value so that we cover most of the cases (optional parameter). Then we may have two questions:

1. What about the Java API that doesn't have a default?
2. What is the default?

My opinions:

1. I think here we're talking about implementations and not specifications. As we've already discussed during our last cross-language call, I think it's good enough in having a different behavior because Python (and Python use cases) are different from Java. I would say +1 in having a default value here on Python.
2. Since how most of the Python applications are designed, it should be very likely that end-users want to close a Span after closing the Scope. In some cases (and not all of them) where callbacks and coroutines are used, changing the behavior is still allowed. Giving our feedback most of our users never had this requirement, so a default of True is what I would advocate.

/cc @carlosalberto @tedsuo @yurishkuro @clutchski @beberlei @wkiser @itsderek23 (will add other contributors/tracer implementers as soon I'll be in touch with them).

[carlosalberto]

Hey @palazzem, thanks a lot for the feedback.

What about the Java API that doesn't have a default?

I think Java can stay default-less for now (in the future, when people may need to add such default value to Java, they will have to consider how it would fit with the rest of the languages/platforms, but that's another story).

What is the default?

Based on the feedback, I'd go for True.

Giving our feedback most of our users never had this requirement, so a default of True is what I would advocate.

This is a good compromise, I think (while still keeping the finish_span_on_close parameter for any future usage).

[tedsuo]

I agree that true would be a good default for Python. This default was removed from Java because it was shown that neither true nor false was "usually" correct. But since in Python it will almost always be true, a default makes sense.

[palazzem]

I think Java can stay default-less for now

Totally agreed. If we don't have metrics to say what is good, I'd say to force it to be explicit for Java.

But since in Python it will almost always be true, a default makes sense.

OK great! thanks for your feedbacks @tedsuo and @carlosalberto!
@carlosalberto are you going to update the PR? I can try it out even if an RC is not out.

[carlosalberto]

@palazzem Yes, I'm on the works of putting together the async frameworks examples + put the default + minor updates, and then will start doing the fun to do a second RC ;) Thanks again for the feedback!

[SEJeff]

@carlosalberto the MockTracer() is super nice for unit testing, but do you have a suggested way of getting the specific spans that cover a response? I ask here because it makes sense to add this to the docs for unit testing.

        client = Client()
        response = client.get('/traced_view/')
        tracer = settings.OPENTRACING_TRACER
        span = tracer._tracer.finished_spans()[-1] # TODO: tie the response to a span. This code is wrong (and I know that)
        assert response['numspans'] == '1' 
        assert span.tags.keys() == ['META']
        assert span.operation_name == 'traced_view_func'
        assert len(tracer._current_spans) == 0

The goal is to be capable of asserting a set of tags or metadata for a given request, where the test client only has a response object. I'm playing with the 2.0.0rc branch of this lib with this django middleware

[carlosalberto]

Hey @SEJeff

but do you have a suggested way of getting the specific spans that cover a response?

Usually, for the tests or examples, we do create a new MockTracer per case, so we start with a fresh one and know all the contained finished_spans() are ours:

settings.OPENTRACING_TRACER = MockTracer() # repeat per test case
client = Client()
...

Otherwise it gets too complicated, indeed, to handle what responses belongs to what operation. Hope that helps ;)

[tsunammis]

At dailymotion, we are looking forward that new version 2, do you have any idea when it will be released?

[carlosalberto]

@tsunammis I hope in the incoming weeks (2-3, I'd say, but not more than that) - we needed a little more time to test the asynchronous frameworks (Tornado, asyncio, gevent) to make sure it's all good (cc @yurishkuro )

In the meantime we will be preparing documentation (to be put in readthedocs.io). Thanks and we will keep you posted ;)

[tsunammis]

@carlosalberto Great! Good luck!

[palazzem]

@carlosalberto let me know if I can help someway (coding, reviews etc..). We're already testing it in our system, if you want we can share some results / caveats (especially for Tornado).

[yurishkuro]

@carlosalberto what is the plan for https://github.com/opentracing/opentracing-python/blob/v2.0.0/testbed/span_propagation.py ? It seems without it the new API is not actually usable in most frameworks, so it would make sense to have this module published, maybe as part of the official opentracing lib (it can use conditional imports).

[palazzem]

Agreed with @yurishkuro. Specific implementations of ScopeManager are needed to propagate properly scopes in different frameworks. At Datadog we offer the same granularity of ScopeManagers, though would be great if these are directly provided by OpenTracing. Otherwise people should rely on our documentation "to make it work".

[carlosalberto]

Hey @palazzem @yurishkuro.

So my original plan was to publish a polished version each of those ones (as well with proper tests and documentation) in their respective python-tornado and similar modules under opentracing-contrib.

So based in your feedback I'm wondering if we should indeed publish them as part of this repository too (as we do for opentracing.extra.scope_manager.ThreadLocalScopeManager). Would that make more sense to you? If that's the case, I can create a PR to 'upgrade' those ScopeManager children ;)

[yurishkuro]

We should have more people opine on that decision. I don't have strong preference for including them here vs contrib, but a slight preference for this repo to reduce the noise in dependencies. One concern would be versioning, say tornado comes with a version that makes existing ScopeManager for it incompatible, we'd have to do an OpenTracing lib release. We'd also potentially have to keep multiple versions (but that's regardless of the repo).

[carlosalberto]

Makes sense to me to ask the feedback/opinion to some more people. @tedsuo @palazzem ?

[palazzem]

In general, I would prefer having these implementations under this repository. Multiple versioning could be a real concern, especially because we may need to make new changes (hopefully not) in the future. In the same repository, it's easier to provide backward/forward compatibility of a specific implementation, making the release process straightforward.

[pglombardo]

As for Instana, we'll be testing the v2 pre-release packages this week but overall everything looks good. Also 👍 on centralizing the framework specific ScopeManager implementations here. I'll update if we find any issues/have questions.

[jimymodi]

When are we planning to release this ?

[carlosalberto]

@jimymodi We are actually slightly behind schedule - but we are waiting for review of #83 (some review has already been given, but waiting for one more), and then #82 and minor documentation updates.

My hope is that it will happen late next week ;)

[carlosalberto]

Hey everybody - I'd like to do the release rather very soon - existing issues are mostly #82 and #83 (and maybe some further, slight documentation touches for putting our stuff into https://readthedocs.org/)

Question for tracer implementors: have you ported/tested your own implementations under this API (maybe in a branch at the moment)? That's a nice thing to have before going forward and doing the actual release ;)

Let us know ;)

@palazzem @pglombardo @yurishkuro

[yurishkuro]

@carlosalberto Did you have a chance to replace context propagation in https://github.com/uber-common/opentracing-python-instrumentation/ with the scope manager? To me that would be the deciding test (and iirc that project uses mock tracer).

Are there any real Trace implementations upgraded to 2.x?

[pglombardo]

We're good to go @ Instana: instana/python-sensor#74

I believe this release has some breaking changes from prior versions. Should tracer implementations have some OT documentation to link to with the releases to best describe the changes in v2.0?

[palazzem]

@carlosalberto I've reviewed and approved both PRs. The API is good from our side and it's a great improvement from the 1.x version. ScopeManagers may need more work in the future but they're really close to our current implementations that are used in production environment (from us and other users).

I guess we can just do the change @yurishkuro suggested so we can see if it's working properly? Any help with that?

[carlosalberto]

@yurishkuro The Lightstep tracer is also updated to use ScopeManager, and I have initial support for Tornado in https://github.com/opentracing-contrib/python-tornado/tree/ot-scope-manager

I will try to squeeze some time to do the opentracing-python-instrumentation that over the end of the week - at least a prototype to make sure it works ;)

[carlosalberto]

Hey @pglombardo thanks for the update.

Should tracer implementations have some OT documentation to link to with the releases to best describe the changes in v2.0?

We will have a small Changelog, and we will put a blog entry to describe the changes overall (once we remaining bits are tested/ready). Think that would be enough for you?

[pglombardo]

Sure thats enough thanks @carlosalberto - just so each tracer implementation doesn't have to write their own docs explaining the changes. Somewhere central/authoritative to refer to...

[carlosalberto]

Hey @yurishkuro

Hey, sorry for the late ping. I went and tried out porting the mentioned instrumentation, and a prototype exists here: https://github.com/carlosalberto/opentracing-python-instrumentation/tree/ot_scopemanager_integration

Notes on the approach here (as they could be too long to be posted here ;) ) https://gist.github.com/carlosalberto/d49fe6ce785d23639d55c2dc004a3f0c

Let me know.

[carlosalberto]

Hey everybody - I was supposed to put a link to the Release Notes draft, but don't see it here (probably posted it elsewhere by mistake :/ ).

Anyway, let me know or comment something right in the document :)

https://docs.google.com/document/d/1ub2YNH1QLBi60F-syyvTzt-uu9wIYVX2BwE54dWad3c/edit?usp=sharing

@palazzem @yurishkuro @pglombardo

[palazzem]

@carlosalberto left a comment! Thanks for sharing the document!

[carlosalberto]

@palazzem @pglombardo @yurishkuro I have updated the document after some feedback (the important part is mentioning the asynchronous frameworks part).

Let me know ;)

[pglombardo]

The updated doc looks great/very helpful. Thanks!

[carlosalberto]

Waiting for the last important item, which is #89 Feedback appreciated!

[carlosalberto]

Merged and released. Thanks you all for your time, code and feedback!