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

Usefulness of Tracer.getCurrentSpan #168

Closed
GregBowyer opened this issue Jun 30, 2019 · 5 comments
Closed

Usefulness of Tracer.getCurrentSpan #168

GregBowyer opened this issue Jun 30, 2019 · 5 comments
Labels
area:api Cross language API specification issue
Milestone

Comments

@GregBowyer
Copy link

Hi there.

So in experimenting with a rust implementation of opentelemetry the usefulness of the method getCurrentSpan on Tracer is a bit questionable.

It feels very thread local in nature, indeed the java api defers this task to grpc.io Context which under the hood apears to use a thread local.

This is fine for java, but gives some more interesting issues for Rust and I would think C++ and Golang.

In Rust it would be desirable to bind a open telemetry Span to the various I/O systems as well as Tokio (especially as a shim between Tokio-trace and open telemetry).

A TLS implementation limits what these frameworks can do to support a current span method, so the natural extension is to add a context argument to the Tracer method, like so

pub trait Tracer {
    fn get_current_span(&self, context: &dyn Context) -> &dyn Span;
}

The natural implementation of this would then be

impl Tracer {
    fn get_current_span(&self, context: &dyn Context) -> &dyn Span {
       context.current_span()
    }
}

Which leads to the question where did the context come from and why we dont get current_span from Context.

If Tracer was not a global, then it wouldnt matter, but that seems to break the independence suggested by the API.

@jmacd
Copy link
Contributor

jmacd commented Jul 1, 2019

It's fine and appropriate to use a Context object in languages with explicit context propagation. We do in Golang. I suspect C++ will adopt a thread-local for this.

We've realized we need to change the way the specification to be less tied to Java, more about requirements and suggested names. See issue 165.

@jmacd
Copy link
Contributor

jmacd commented Jul 1, 2019

(Although, I think we could debate over whether this is a Tracer method or a static method. We admit that there are more than one tracer in the process, but I haven't thought through the implications of allowing more than one active span.)

@carlosalberto
Copy link
Contributor

In Python we have used this approach (it was even used since OpenTracing), as different asynchronous frameworks used their own context (which is getting unified with the new contextvar effort, btw).

As @jmacd said, for languages using an explicit Context object it's fine to simply rely on it - else, keeping a reference to the current Trace has been quite useful.

@tedsuo tedsuo added the area:api Cross language API specification issue label Jul 11, 2019
@bogdandrutu bogdandrutu added this to the Alpha v0.3 milestone Sep 30, 2019
@pauldraper
Copy link

The long-term plan is to separate propagation from metrics and traces.

Rather than Tracer.getCurrentSpan(), there would be ContextStore.current().get(spanKey)

#237

@carlosalberto
Copy link
Contributor

Hey @GregBowyer I'm going to close it as the Rust API/SDK have changed a lot. Please re-open if it needed (or create a new ticket).

TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this issue Oct 15, 2020
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 21, 2024
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 23, 2024
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
Projects
None yet
Development

No branches or pull requests

7 participants