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

Enable static functions to work with active span #924

Closed
toumorokoshi opened this issue Sep 7, 2020 · 11 comments · Fixed by #923
Closed

Enable static functions to work with active span #924

toumorokoshi opened this issue Sep 7, 2020 · 11 comments · Fixed by #923
Assignees
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory

Comments

@toumorokoshi
Copy link
Member

toumorokoshi commented Sep 7, 2020

What are you trying to achieve?

In languages such as Python, there exist static functions that can live at the package / module level. It would improve the user experience if consumers did not have to go through the work of creating an object to retrieve the current span:

  • no need for a Tracer instantiation to retrieve a span
  • no confusion to the user regarding what values in the instantiated Tracer impact active span setter / getter behavior

Work to simplify this has already been implemented in Python as an experiment, and has worked quite well in terms of reducing boilerplate and confusion around the behavior of context.

Before:

from opentelemetry import trace

# despite no information about the tracer being used, the user must pass it in anyway. # It also can raise the question to the consumer about whether tracers share active spans.
tracer = trace.get_tracer(__name__)  
current_span = tracer.get_current_span()

After:

from opentelemetry import trace

# no mention of tracer name, clear it's global to some level
current_span = trace.get_current_span()

Under the hood, this simplification is possible because:

What did you expect to see?

The changes to the spec needed to enable a function at the module level are:

  • added documentation around a "Trace Package", with (at least optional) methods to get and set the active span
  • added (at least optional) methods to the TracerProvider to get and set the active span.

Additional context.

What about languages that don't allow static methods at the package level?

The optional nature of these changes allows languages where it is convenient to add span manipulation to the tracer directly to continue to work the same way.

However:

Languages such as Java may choose to make the active span behavior completely static and create static methods instead, avoiding creating an instance.

Related issues

@toumorokoshi toumorokoshi added the spec:trace Related to the specification/trace directory label Sep 7, 2020
@Oberon00 Oberon00 added area:api Cross language API specification issue spec:context Related to the specification/context directory labels Sep 7, 2020
@Oberon00
Copy link
Member

Oberon00 commented Sep 7, 2020

One question. What are the scenarios in which you typically call get_current_span? What do you do with the returned Span?

@toumorokoshi
Copy link
Member Author

One question. What are the scenarios in which you typically call get_current_span? What do you do with the returned Span?

The common scenario is adding new attributes or events to the active span. It could also be arguable that a new span should be created, at which point you would have a handle and no longer need to use get_current_span.

But I have seen a strong preference for some individuals to add more attributes to an existing span, with reasoning being reducing the number of spans visible in a trace (less visual clutter).

Another use case is those using an instrumentation library. You won't have access to the span directly, so you call get_current_span in a place where the current instrumentation has already set the span as active.

@Oberon00
Copy link
Member

Oberon00 commented Sep 8, 2020

One original motivation for supplying an instrumentation library name was to stop one by name from emitting telemetry. If the current span is used to do that, it would be worth considering if using the tracer for that by default has merits.
As no one seems to really use the instrumentation library name for that (yet?), it's not a strong objection though.

@andrewhsu andrewhsu added release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p1 Highest priority level labels Sep 8, 2020
@toumorokoshi
Copy link
Member Author

toumorokoshi commented Sep 9, 2020

This enters the subjective realm (although this whole issue is subjective), but I would find such methods a bit less intuitive, primarily since retrieving and object and modifying it is a common object-oriented pattern. I'm not sure if the intuition follows to retrieve a tracer, and modify the spans via indirect methods.

That said, the approach would be about the same number of operations:

trace.get_current_span().add_attribute("key", "value")

vs

trace.get_tracer(__name__).add_attribute_to_current_span("key", "value")

@iNikem
Copy link
Contributor

iNikem commented Sep 9, 2020

My 2c: I think one should not need to provide instrumentation library name to get current existing span. It does not make any sense to require one.

@Oberon00
Copy link
Member

Oberon00 commented Sep 9, 2020

I think that opinion is valid, and I don't necessarily disagree, except for the "any sense" part: Consider an instrumentation that adds lots of attributes or a large attribute to a span. For example, there might be an instrumentation that adds the exception semantic conventions (including stacktrace) for every thrown exception to the currently active span. Or an instrumentation that adds an event to a span for every time an asyncronous tasks gets suspended/resumed, etc. It would then make sense to have that instrumentations use a separate tracer.

@tedsuo
Copy link
Contributor

tedsuo commented Sep 10, 2020

I would strongly vote for the simplified experience. The named tracer approach is straightforward for instrumentation plugins, but can be awkward for application code.

One approach might be to add a constant DEFAULT_TRACER_NAME = "application", and have that tracer be available to application code as a global. This would. stick with our named tracer module, but improve the experience for application developers. Obviously it wouldn't improve the experience for instrumentation plugins, but perhaps those are self contained enough that it isn't an issue?

@toumorokoshi
Copy link
Member Author

I would +1 a default tracer, although I think that's not strictly necessary for this experience.

If we went that route, I would worry about the ramifications regarding instrumentations authored outside of core opentelemetry. It would be easy for app developer to:

  1. learn to use the default tracer exclusively.
  2. author and instrumentation and use the default tracer to do so, since that's all they know

Of course it can be rectified later and many would look at other instrumentation and notice the named tracer pattern. Thus why I'm a +1

@tedsuo
Copy link
Contributor

tedsuo commented Sep 11, 2020

Yeah, I agree that someone could forget, but it's an easy fix for all the reasons you mention. Just to clarify, that would solve the issue, correct?

@toumorokoshi
Copy link
Member Author

This issue could be solved via your suggestion, but there's also #923 that has the initial suggested approach.

I still think your change has merit though, to further simplify the new user experience. But I'd like @Oberon00's input on that, as named tracers is critical to Dynatrace's needs.

@Oberon00
Copy link
Member

as named tracers is critical to Dynatrace's needs

It should be enough if new spans are created through named tracers. Having all active-span manipulations traced through named tracers would be nice-to-have, but I think the approach in #923 is fine. Feels far better to me than having a default tracer in any case 😃 Although I don't think we have to decide here (#586 is orthogonal to this IMHO)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:context Related to the specification/context directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants