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

Rename start_span/create_span #97

Closed
carlosalberto opened this issue Aug 21, 2019 · 8 comments
Closed

Rename start_span/create_span #97

carlosalberto opened this issue Aug 21, 2019 · 8 comments
Labels
api Affects the API package.
Milestone

Comments

@carlosalberto
Copy link
Contributor

Currently start_span() in different languages has different activation semantics: in Java and Ruby, it does not set the newly created Span as the active instance, while Python does (Go seems to offer to do it if you pass a Context).

In OpenTracing, for this purpose, we had start_span() and start_active_span().

Hence I'd suggest renaming the related methods:

  • create_span() -> start_span()
  • start_span() -> start_current_span() or similar

This will most likely impact the Specification itself at some degree, but wanted to ask here first about your opinion ;)

@Oberon00
Copy link
Member

I don't like the name start_current_span, it sounds like it does get_active_span().start(). Maybe start_as_current_span? But to be honest, I think the current names aren't so bad either.

@carlosalberto
Copy link
Contributor Author

I have no problem with the current names, but for uniformity purposes there is motivation to change them ;)

(and actually this could also impact Span.start(), as it would be removed to have a similar API with the other languages)

@Oberon00
Copy link
Member

Hmm, removing the ability to have an unstarted span would be a welcome simplification.

@reyang
Copy link
Member

reyang commented Aug 22, 2019

Hmm, removing the ability to have an unstarted span would be a welcome simplification.

Having unstarted span could be an important feature, I remember in OpenCensus there was scenario like this:

create_span
add attributes
if condition 1:
    do something
    start_span
elif condition 2:
    do something else
    add attributes
    start_span
else:
    do not start the span at all
...

But personally I tend to put simplicity at the highest priority, unless there is a show blocker.

@carlosalberto
Copy link
Contributor Author

I don't think I've ever seen need for a pattern like this, to be honest. But even so, we could have that as an extra method (to have it remain as create_span()).

But having the Span be active or not from Start Span depending on language feels... weird ;)

@Oberon00 Oberon00 added the api Affects the API package. label Aug 23, 2019
@carlosalberto
Copy link
Contributor Author

Related issue in the specification: open-telemetry/opentelemetry-specification#238

@bogdandrutu
Copy link
Member

I would strongly suggest to make python consistent with other languages and remove the "just create".

@reyang For attributes we do have an option to add attributes during the span creation time.

Also there were some concerns about having "delayed" start span option open-telemetry/oteps#24 and if this capability is needed you need to have an otep to justify why we should support it in all the languages.

@Oberon00
Copy link
Member

Oberon00 commented Sep 23, 2019

By the way, I was thinking about removing the ability to end_on_exit in #154 and instead make Span a context manager (in a separate PR). Thinking about it, I came up with the following code example:

In [2]: class SpanActivationManager:
   ...:     def __enter__(self, span):
   ...:         # set active
   ...:         return span
   ...:     def __exit__(self, *args):
   ...:         pass
   ...:

In [3]: class Span:
   ...:     def __enter__(self):
   ...:         return self
   ...:     def __exit__(self, *args):
   ...:         self.end()
   ...:

Indeed, one can activate and automatically end the Span with that by doing

with span, SpanActivationManager(span):
 pass

But that seems a bit arcane to me. What do you think?

EDIT: Of course, in real code, instead of SpanActivationManager(span), one would write tracer.use_span(span)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the API package.
Projects
None yet
Development

No branches or pull requests

5 participants