context.withSpan - proposition

[obecny]

In many places we will have now a repeatable code

context.with(setSpan(context.active(), span), ()=> { ....

Could we somehow simplify it by adding an extra helper function into something like

context.withSpan(span,()=> { .....  

This is just proposition, to make it easier and to be more intuitive.

[Flarna]

So mostly the same as tracer.withSpan which was removed recently.
Putting it on context is clearly a better choice as states that global context instance is used. For Tracer this was not clear as it could be created from a non global TracerProvider using a different ContextManager.

Not sure if Spec allows to add more methods to global API objects. If not a free function could be used instead, similar as the existing setSpan,...

[obecny]

The spec doesn't really define functions like setSpan etc. it is up to us to name them etc. The new helpers will be nothing more then just a shortcut sitting in the same "namespace"

[jonaskello]

I found the below helper function useful but it goes a bit further and auto-ends the span. Not sure if this is good practice or not. It has worked OK for me :-). Before I had problems with spans not getting closed in some cases because developers forgot to call span.end().

/** Function I want to trace */
async function readFileWithRefs(fileName: string): Promise<FileWithRefs> {
  return await withSpan("readFileWithRefs", async (span) => {
    span.setAttribute("filename", fileName);
    return JSON.parse(await fsp.readFile(fileName, "utf8"));
  });
}

/**
 * Helper function for running code within a span.
 * Create a new span, make it the active span on a new context and execute the provided
 * function within that context
 */
export function withSpan<T extends (span: Span) => ReturnType<T>>(operationName: string, fn: T): ReturnType<T> {
  const tracer = api.trace.getTracer("default");
  const span = tracer.startSpan(operationName);
  const ctx = api.setActiveSpan(api.context.active(), span);
  const fn2 = (): ReturnType<T> => {
    const result = fn(span);
    // The result might be a promise but there is no safe way to tell,
    // instead convert the value to something that we know for sure is a promise
    const promise = Promise.resolve(result);
    promise.then(() => {
      // End the span when promise resolves
      // In case we wrapped a value it will be resolved directly
      if (span.isRecording()) {
        span.end();
      }
    });
    return result;
  };
  return api.context.with(ctx, fn2);
}

[obecny]

@jonaskello the helper function you posted is something different then what we are talking here, it will be just a shortcut for the code snippet I posted, nothing more

[jonaskello]

Yes, it does a lot more work than being a shortcut. I'm new to all this so I'm curious where/how the proposed shortcut helper would be used and if I am somehow overdoing it with my own helper.

[obecny]

we haven't yet decided if we add it or not, if we add this it will sit in api context, feel free to add your thoughts if you think such a shortcut will be useful, thx

[dyladan]

Other languages I have seen have a method that starts a span and sets it active on the context. ruby and python have these mechanisms. @mwear how has that worked out for ruby? do you expect it would be a useful addition here?

[Flarna]

Other languages I have seen have a method that starts a span and sets it active on the context.

The current JS API to use with(fn) has the advantage that setting the context is scoped and works also if done nested. If a user can set a context on the current async operation it's responsible to unset it also - in the right order.
Consider following code:

const rootSpan = tracer.startSpan('root')
const scopeRoot = contextSetActiveSpan(rootSpan)
const childSpan = tracer.startSpan('child')
context.current = setSpan(context.active(), childSpan)
const scopeChild = contextSetActiveSpan(childSpan)
//... other code and possible child spans of childSpan

scopeRoot.done() // <= which context is active after this call?
scopeChild.done() // <= which context is active after this call?
// what if the sequence is exchanged?

if you transform this to use context.with() it's clear and not error prone.

Besides that this would require that the underlying context manager allows to modify the current context. This is supported by Node AsyncLocalStorage.enterWith() but it's controversial (see nodejs/node#35286 (comment)).

[dyladan]

Other languages I have seen have a method that starts a span and sets it active on the context.

The current JS API to use with(fn) has the advantage that setting the context is scoped and works also if done nested. If a user can set a context on the current async operation it's responsible to unset it also - in the right order.
Consider following code:

const rootSpan = tracer.startSpan('root')
const scopeRoot = contextSetActiveSpan(rootSpan)
const childSpan = tracer.startSpan('child')
context.current = setSpan(context.active(), childSpan)
const scopeChild = contextSetActiveSpan(childSpan)
//... other code and possible child spans of childSpan

scopeRoot.done() // <= which context is active after this call?
scopeChild.done() // <= which context is active after this call?
// what if the sequence is exchanged?

if you transform this to use context.with() it's clear and not error prone.

Besides that this would require that the underlying context manager allows to modify the current context. This is supported by Node AsyncLocalStorage.enterWith() but it's controversial (see nodejs/node#35286 (comment)).

I would not recommend the enterWith approach, but rather some function like this:

tracer.startActiveSpan(name, opts, ctx, () => {
  // this is just a thin wrapper around api.context.with
});

[tedsuo]

Just dropping in to say please, please add shortcuts for these things. api.context.with(api.setSpan(api.context.active(), span))? Seriously? This is impossible to teach users.

[mwear]

Other languages I have seen have a method that starts a span and sets it active on the context. ruby and python have these mechanisms. @mwear how has that worked out for ruby? do you expect it would be a useful addition here?

Ruby has two methods that work well for us Tracer#in_span and Trace.with_span.

Tracer#in_span will start a new span, make it active, and execute a block of code. It will reset the active span after the block executes: See https://github.com/open-telemetry/opentelemetry-ruby/blob/0bbcbab686ac679fd6e2759df3103d0028abb50a/api/lib/opentelemetry/trace/tracer.rb#L11-L37. This is pretty heavily used in instrumentation.

Trace.with_span takes a span, makes it active and executes a block of code, then deactivates the span. See: https://github.com/open-telemetry/opentelemetry-ruby/blob/0bbcbab686ac679fd6e2759df3103d0028abb50a/api/lib/opentelemetry/trace.rb#L74-L84.

I think either one of these would be a good addition.

[Flarna]

tracer.startActiveSpan(name, opts, ctx, () => {
  // this is just a thin wrapper around api.context.with
});

As long as we support more ContextManagers/TracerProviders operations on Tracer are problematic, see #1932

Besides that I would love to see shortcuts writen in spec to get some consistency between languages.

[dyladan]

Just dropping in to say please, please add shortcuts for these things. api.context.with(api.setSpan(api.context.active(), span))? Seriously? This is impossible to teach users.

Context in general is difficult to teach to users, but so is having multiple ways to create and activate spans (not to mention the nomenclature trouble between started and "active"). FWIW most users use the auto instrumentation, and those that use manual tracing don't seem to be confused (or are at least not telling us if they are).

Ruby has two methods that work well for us Tracer#start_span and Trace.with_span.

We used to have withSpan which worked much the same way and took a callback (I believe it was came from opencensus and was actually required by spec at one point). It was removed because it was removed from the spec. I'm not necessarily against adding it back, but it is just additional API surface for us to maintain. If these are something all languages should be implementing then why are they not spec'd?

[dyladan]

It was my understanding that the difference between a started span and a span that was the active span on the context was an important part of the context distinction and was done in the spec quite intentionally. If that distinction is important in the spec then why would it be encouraged to obfuscate it in the client implementations?

[mwear]

We used to have withSpan which worked much the same way and took a callback (I believe it was came from opencensus and was actually required by spec at one point). It was removed because it was removed from the spec.

One subtle difference between the JS implementation and the Ruby implementation is that withSpan was an instance method on a tracer in JS and the Ruby equivalent is a method on the Trace module (not tied to a specific tracer instance). I agree that withSpan doesn't make sense on the tracer, but we could export a helper function from the API that does basically the same thing.

[dyladan]

We used to have withSpan which worked much the same way and took a callback (I believe it was came from opencensus and was actually required by spec at one point). It was removed because it was removed from the spec.

One subtle difference between the JS implementation and the Ruby implementation is that withSpan was an instance method on a tracer in JS and the Ruby equivalent is a method on the Trace module (not tied to a specific tracer instance). I agree that withSpan doesn't make sense on the tracer, but we could export a helper function from the API that does basically the same thing.

This changes api.context.with(api.setSpan(api.context.active(), tracer.startSpan())) to api.trace.withSpan(tracer.startSpan(), api.context.active())? It might be a little more convenient to type, but it doesn't change the fundamental conceptual difficulty. I don't think a user confused by the first would be any less confused by the second.

[obecny]

@dyladan not api.trace.withSpan(tracer.startSpan(), api.context.active()) but

api.trace.withSpan(tracer.startSpan()()=> {
  // create a new span will be a parent of previously started span, exactly as it was with removed function
})
or 
context.withSpan(tracer.startSpan()()=> {
  // create a new span will be a parent of previously started span, exactly as it was with removed function
})

//etc.

[dyladan]

I still think it isn't any less confusing, just a little more convenient to type. It still requires the user to understand the concept of the background context.

[mwear]

My vote would be for api.trace.withSpan(span, () => {} ) as it'd match the Ruby implementation.

[dyladan]

Does this assume the context.active() always? Or is it an optional third parameter with context.active() the default value?

[Flarna]

My reading of the spec is that only static methods are allowed so the method we removed on Tracer seems to be a nogo.

I think the best thing we can do is to add

// in API, uses global context manager
api.context.withSpan(span, fn, thisArg, ...args)`
// and on context manager (to allow use of non global context managers)
contextManager.withSpan(span, fn, thisArg, ...args)`

Maybe also .withSpanContext(spanContext,...) and .withBaggage(baggage,..).

Another option would be to add methods to the context object as this avoids any defaulting to context.active(). So one could write ROOT_CONTEXT.withSpan(span, () => {}, thisArg, ...args).

I would not put it on Tracer as this binds it to the global context manager (except we go for global only).

[mwear]

Does this assume the context.active() always? Or is it an optional third parameter with context.active() the default value?

It assumes context.active(). An optional context parameter could add some additional flexibility though.

[obecny]

to avoid confusion and explanation where this sits I would keep it simple

api.withSpan(span, ()=> {
  //
})

All other edge / advance cases are already covered with current functionality. I want to simplify things for 80% the most simple common scenarios without writing a big documentation and adding more params etc. The rest 20% of cases can be covered with what we have

[tedsuo]

@Flarna just to clarify, there is no reason from a spec perspective that you can't add high level convenience methods. I recommend having these functions sit directly in the api package, like @obecny suggests. Ideally, application developers can stick to this single package, since their needs are not complex. This makes it easy to get started with OpenTelemetry, and then progressively learn the details later.

I really do want to stress that this is an important feature. The lower level objects are well factored, not questioning that. But I train a lot of people and this kind of simplified UX makes a huge difference.

[dyladan]

to avoid confusion and explanation where this sits I would keep it simple

api.withSpan(span, ()=> {
  //
})

All other edge / advance cases are already covered with current functionality. I want to simplify things for 80% the most simple common scenarios without writing a big documentation and adding more params etc. The rest 20% of cases can be covered with what we have

I think this works except I would suggest api.trace.withSpan just to keep it better separated.

[tedsuo]

@dyladan as long as it is in the same package I can live, but users do have a clear preference for a unified API. It's shorter to type and makes discovery easier for them.

From my experience, users would be happiest with having it all at their fingertips. Not just otel.withSpan(), but also otel.setAttribute(), otel.addEvent(), etc. The more succinct the more they like it. Since we already have it well-factored under the covers, we can go all the way with the convenience layer. Please consider something like this.

[dyladan]

@tedsuo I think everything we've talked about has been in the API package which is only one package (except metrics)

[dyladan]

Not just otel.withSpan(), but also otel.setAttribute(), otel.addEvent(), etc.

I'm 100% with you on withSpan modulo my previous complaints that I wish this was specified.

setAttribute and addEvent are a little less obvious because the question quickly becomes "what am i adding an event to?" or "set attribute on what?" As the JS maintainer I know the answers to these questions, but they may not be obvious to new users. Especially when we have such similar things as attributes (trace) and labels (metrics) with very subtle semantic differences and seemingly synonymous names.

My biggest gripe here is with the "etc." part. Without specification, how can I know what we should implement here? What happens if the spec adds some of these methods later that have subtly different implementations than what we implement?

[tedsuo]

Hmmmm, I'm not sure that complexity would be helpful for users. You can't break them once you add them, and if you ended up creating an api.withSpan and an api.h.withSpan that were subtly different, it would be confusing.

Personally, I think defining helpers like withSpan to do whatever is most helpful in python is the correct way to go. We can add suggestions to the spec, but the point is to make helpers helpful.

The main reason that I'm not as worried about deviation in this area is because these are higher level functions. As long as they just represent what a combination of lower-level functions would do, there's no risk that somehow the design of these helpers would be wrong in the sense that python would suddenly become incompatible with ruby, or break an important edge case.

[dyladan]

@dyladan I think the request to have this specified is totally reasonable. I would want to emphasize that implementations would not need to exactly follow that portion of the spec if it was not actually helpful, as "convenience" can often mean "idiomatic for a given language." But there's no reason we should not provide guidance here.

Let's see if we can get that moving quickly in the spec; I don't want to leave v1.0 implementations in a position where users are trying it out without these helpers, as they really improve the experience for new users.

Maybe the spec can probably have a "suggested helpers" section or something where helpers and name suggestions are listed but are not required to be spec compliant.

Suggested Helpers

Methods in this section are not required for a client to be specification compliant, but are encouraged in cases where it makes sense for the language.

Start a span and make it the active span on the context

Name Suggestion: WithSpan

Functionally equivalent code:

api.withSpan(span, () => {
	// do something here
});

api.context.with(api.setSpan(span), () => {
	// do something here
});

Set an attribute to the currently active span

NameSuggestion: SetSpanAttribute

Functionally equivalent code:

tracer.setSpanAttribute(attributeName, attributeValue);

tracer.getCurrentSpan().setAttribute(attributeName, attributeValue);

etc.

[Flarna]

see #open-telemetry/opentelemetry-js-api#7

[vmarchaud]

Well with a quick look on other implementations:

• Go expose them on trace namespace: https://pkg.go.dev/go.opentelemetry.io/[email protected]/api/trace#ContextWithSpan
• Python doesn't any any, they use with keyword: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/tests/test_implementation.py#L38
• Java doesn't provide any top level helper: https://github.com/open-telemetry/opentelemetry-java/blob/main/api/all/src/test/java/io/opentelemetry/api/trace/DefaultTracerTest.java#L68

I believe we should be namespacing helper, as i said in #7, i prefer using api.context.with (like proposed by @Flarna in his PR).

[obecny]

@vmarchaud but you are aware then we end up with having more helpers in many different namespace so the user will still have hard times with understanding which namespace he should be using, whereas keeping it on global api will simply allow them to use it without necessity of understanding first where this helper should belong to ?
And with any todays editor you can do api. and it will show you all available methods already

[vmarchaud]

but you are aware [...]

Yes i am, i believe that if the user wants to instrument his code he should be able to read a documentation or understand how our 3 api (trace, context and baggage) works. In the context of the issue, the user want to set a span as active in the context so the helper is available in the context namespace.
Having all helpers shows up on the global export doesn't help the user at all understand that we have different API for different purposes.

[obecny]

@vmarchaud
if you need to read the whole documentation before you can do a very simple thing that means your api is not intuitive and something is not right. If you are entering a new car you can expect that user should be able to drive it without reading whole manual. Things should be easy to understand etc. You can't expect a user to have knowledge how to change the oil or if you want to open a trunk you are doing this intuitively, you are not checking manual for that. It should be the same here. User will give it a try and if he cannot figure out easily something after 3 minutes the frustration comes. You are already expecting a user to know what is a context and that we named this a context - previously it was a scope, some different instrumentation library might name this even differently so how the user can do it without gaining an expert knowledge first is really what makes many things successful.

[vmarchaud]

You can't expect a user to have knowledge how to change the oil or if you want to open a trunk you are doing this intuitively, you are not checking manual for that

I expect that he knows that he need to turn his steering wheel to turn his car.

Generally speaking i will stand my point that we need to namespace the helpers, as i linked major implementation didn't export helpers on the global namespace so i believe we should be coherent. We can discuss with its best to expose it in context or trace though

[tedsuo]

vmarchaud we'll be focusing on improving our approach to having a convenience API across all languages over the next month, so we can have more of a discussion there.

I do think you're being kind of rough by saying that users should just RTFM. Independent of whether a namespace is appropriate or not, we should strive to make the user have as intuitive an experience as we can. I would encourage the JS maintainers to actively seek user feedback on the convenience issue.

[dyladan]

I also think a namespace is appropriate in general. For withSpan maybe it is obvious, but for other things like setAttribute it will become much less obvious as there are many things which may have attributes (spans, resources, metrics, etc).

Namespaces can cause issues with code minifiers, which is an issue in JS that other languages likely don't really care about, but we may be able to get around that by having very descriptive top-level function names like setCurrentSpanAttribute or something similar.

[vmarchaud]

@tedsuo

we'll be focusing on improving our approach to having a convenience API across all languages over the next month, so we can have more of a discussion there.

In this case can't we wait for a decision at the spec level and publish a minor release after RC ? It seems a better idea to me.

I do think you're being kind of rough by saying that users should just RTFM. Independent of whether a namespace is appropriate or not, we should strive to make the user have as intuitive an experience as we can

I understand how you could see my point as RTFM but it was in the context of using namespacing or not. I believe there a difference between explaining that we have different APIs, so they should find helper in the correct API instead of throwing helpers as top level because they will have completion for it.

Furthermore, i don't think an user that doesn't know much about otel will be better able to use a withSpan helper because its exported as top level, specially since how the context behave in JS runtimes where its applied in the callback or in any async context that might be created while running that said callback. This is a much more complex issue that i believe is better solved with correct documentation than helpers (though i accept that not everyone agree with this).

I would encourage the JS maintainers to actively seek user feedback on the convenience issue.

I remember seeing this document circulating about user research on gitter, is there any result about the JS API/SDK about this ? I don't know if the result are openly accessible.

[austinlparker]

As a short-term solution, might I suggest someone take it upon themselves to write up a concise explanation of how to use the low-level API for the docs site?

[dyladan]

I am working on that now for our API repo. The docs site can use the same documentatio

[austinlparker]

Fantastic! If you tag me in the PR I'll copy it over.

[naseemkullah]

open-telemetry/opentelemetry-js-api#14 opened but waiting on: should it be api.context.withSpan, api.trace.withSpan or api.withSpan ?

[vmarchaud]

@naseemkullah I think this will be decided at the spec level

[aabmass]

Are we still waiting for a convenience spec before implementing this? My understanding is the convenience spec will be language agnostic and might not answer all of the questions above anyway. It would be really great to have this for the 1.0 release, I think most newcomers are expecting new spans to automatically become children and then can't figure out how to do this at all. I see the question "how do I set the parent span" all the time.

• Python doesn't any any, they use with keyword: https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/tests/test_implementation.py#L38

Python uses the with keyword for automatically ending a span, but to set the span in context automatically, there is with tracer.start_as_current_span() as span: ... (source).

Has adding with() on span been considered?

tracer.spartSpan('span name').with((span: Span) => {
  //...
});

[obecny]

to mimic what python does

tracer. spartSpanAsCurrentSpan(...

I'm all for such convenience self explanatory method.

[naseemkullah]

Both namespace vs top level have pros and cons, abstracting a little less to leak the mechanics in a good way vs abstracting more in the name of user friendliness, and there is no obvious choice or current spec recommendation. However the time has come to make a choice, JS has 3 maintainers making a stalemate impossible if this were voted on. How about a vote to settle this?

api.context.withSpan vs api.withSpan? I think @obecny and @vmarchaud have voiced theirs already, @dyladan could you break the tie and we move forward?

[naseemkullah]

What is throwing me off as well is that getSpan an setSpan are at the top level (but there's probably a good reason for that)

[naseemkullah]

Maybe we could expose two layers of abstraction and maintain both tracer.startSpanAsCurrent() which would wrap api.context.withSpan() ? A little more work but every user wins

[dyladan]

Not sure what makes you think a choice needs to be forced now since these convenience methods could easily be added later and there is an ongoing spec effort to add these types of convenience methods.

If forced into a decision now, I would prefer to keep the context API free from signal-specific functions. Keeping the semantic of "signals use context, not the other way around" will prevent us from accidentally introducing circular logic. I think something like tracer.startSpanAsActive, keeping "active" word instead of "current" since it is already used other places, would be a reasonable course of action.

[naseemkullah]

Not sure what makes you think a choice needs to be forced now since these convenience methods could easily be added later and there is an ongoing spec effort to add these types of convenience methods.

"Bias for action" as they say 😄

[naseemkullah]

closed via open-telemetry/opentelemetry-js-api#54 which adds tracer.startActiveSpan(name, opts?, context?, fn(span)) 🎉

[naseemkullah]

follow up to implement in Tracer created: #2210