-
Notifications
You must be signed in to change notification settings - Fork 624
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
Comments
I don't like the name |
I have no problem with the current names, but for uniformity purposes there is motivation to change them ;) (and actually this could also impact |
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:
But personally I tend to put simplicity at the highest priority, unless there is a show blocker. |
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 But having the |
Related issue in the specification: open-telemetry/opentelemetry-specification#238 |
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. |
By the way, I was thinking about removing the ability to 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 |
Currently
start_span()
in different languages has different activation semantics: in Java and Ruby, it does not set the newly createdSpan
as the active instance, while Python does (Go seems to offer to do it if you pass aContext
).In OpenTracing, for this purpose, we had
start_span()
andstart_active_span()
.Hence I'd suggest renaming the related methods:
This will most likely impact the Specification itself at some degree, but wanted to ask here first about your opinion ;)
The text was updated successfully, but these errors were encountered: