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

Add Tracer#start_active_scope and deprecate Tracer#start_active_span #36

Closed
wants to merge 1 commit into from
Closed

Add Tracer#start_active_scope and deprecate Tracer#start_active_span #36

wants to merge 1 commit into from

Conversation

indrekj
Copy link
Contributor

@indrekj indrekj commented Jun 29, 2018

After some discussion in
opentracing/opentracing-python#64 (comment)
and in the cross-language gitter, it was decided that #start_active_scope
makes more sense than #start_active_span because the return type is a
Scope, not a Span. Python and other similar languages will also change it.

After some discussion in
opentracing/opentracing-python#64 (comment)
and in the cross-language gitter, it was decided that #start_active_scope
makes more sense than #start_active_span because the return type is a
Scope, not a Span. Python and other similar languages will also change it.
@indrekj
Copy link
Contributor Author

indrekj commented Jun 29, 2018

@pglombardo
Copy link
Contributor

My thoughts just for the record:

  1. I've seen it said multiple times that we'd like to eventually hide/abstract scope from users or at least make it more transparent - but this change puts "scope" in the method name bringing Scopes to the cognitive forefront.

  2. start_active_span - to understand the "active" in the method name you have to have an idea of Scopes and what they do/are for. Hence if the return is a scope, you know & understand why that is the return value

  3. start_active_scope - This name makes it's clear that the return is a scope which is good, but it raises the question, do I now have to create a span on that scope? Did I just name the scope? I think this is more confusing to new users than the former.

IMO this change introduces more confusion than it solves.

@carlosalberto
Copy link

Hey @pglombardo

Thanks for the feedback. My impression is that there's no perfect solution here - so it's a matter of choosing which one is the 'best' (which, well, can also be subjective ;) )

Was wondering if I can quote (or copy & paste) your comment for opentracing/opentracing-python#89 ? ;)

@pglombardo
Copy link
Contributor

Was wondering if I can quote (or copy & paste) your comment for opentracing/opentracing-python#89 ? ;)

If it's helpful, by all means...

@mwear
Copy link
Member

mwear commented Jun 30, 2018

I've seen this conversation in progress. Thanks for taking on this update @indrekj.

@carlosalberto @tedsuo Is this decision baked enough that it's ready to go into an official release, or is there a debate ongoing? I would like to merge this if the decision is reasonably final. If not, we could make this available in a separate branch for the time being.

@palazzem
Copy link
Member

palazzem commented Jul 3, 2018

Hello everyone! I have some thoughts about this change:

  • in general, I think that our end goal must be removing the concept of Scopes from our basic usage, unless something advanced is required. For instance, the Datadog API was tracer.trace(operation_name) (trace in the sense of the verb; we can argue it's also a noun). It was simple, it used in-process context propagation via "ScopeManagers" (slightly different in our first implementation) and it returned a Span. We never had complains about that API, I guess mostly because it was easy-to-use
  • a change in the start_span method, may be that API (lot to discuss here but just to give a pointer)
  • we agreed that having the Scope was necessary for some advanced use-cases (especially for Java). In that case, I'm OK in having an API that uses / creates a Scope.
  • start_active_span or start_active_scope? I agree with @pglombardo that start_active_scope is better because it's clear what the method is doing. It starts a new Scope, then you know what a Scope is and you know it creates also a Span. But I agree that it could just create confusion. The basic usage is using in-process context propagation, so it means creating a Scope all the time. We're working on this for a very long time, but a newcomer may really have the question "do I need a Scope or a Span for my use case?"
  • I definitely think this is a last hour change. I don't think it's wise to rush and change a public API with a week of conversation prior a 2.0 release for Python. It also introduces a deprecation warning in other libraries that is not great if this is not a stable change. It will happen again and it's costly to ask our users to change the instrumentation when their entire codebase is instrumented via OpenTracing. It will reduce the trust in the project I think

My suggestions are:

  • start_active_scope() should be the right name, but not at this stage
  • I think we should not merge this change with a discussion happening in a PR. I'd advocate writing an RFC that includes all possibilities to write a stable API with the goal "hide the Scope concept from basic usage"
  • keep the implementation as we have now (start_active_span), because it's probably not great but it would close a big pending topic that is here from a year. We don't change anything, we ship the Python 2.0 release, we re-open the debate in an RFC so that we have time to take in consideration all the possibilities

@carlosalberto
Copy link

Hey @palazzem thanks for the feedback.

Regarding the RFC, I think it could be part of the ongoing https://github.com/opentracing/specification/blob/master/rfc/scope_manager.md - as we will be moving it to TEST stage, we can start defining specific requirements or strong suggestions for the names.

in general, I think that our end goal must be removing the concept of Scopes from our basic usag

There was some talk around this from @tedsuo on providing a hight level API (on top of the current API we all use) that wouldn't be exposing so much details. This could go in that direction.

@tedsuo
Copy link
Member

tedsuo commented Jul 5, 2018

Thanks for the feedback. There is general agreement that a simpler solution is needed for common use cases. However, it's an open question as to how to implement it.

I think this is a cross-language issue which will take some time to resolve. I'd prefer not to hold up the release of this version of the Python API, as Scopes are useful and this version is ready to go. Given that start_active_span is what we call it in Ruby and Java (SpanBulder#startActive() which is basically the same thing) I am leaning towards sticking to that convention for now.

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.

6 participants