Proposal: remove Span

[yurishkuro]

This is obviously controversial, but it would be nice if we removed the concept of the Span from tracing API, and replace it with methods on the Tracer, such as:

tracer.SetSpanAttribute(ctx, key, value)
tracer.RecordSpanEvent(ctx, event)

There are two reasons for that:

1. When Context API is moved into an independent layer below all other layers, the way extractors might work is like this: ctx = extractor.extract(ctx, request). Because extract() cannot start a new span, it must store span context in the returned ctx. With this proposal, it will always keep the span context only in the context, never the Span.
2. Not giving users references to Span objects simplifies memory management. In OpenTracing it was pretty difficult to pool span objects because user can keep a reference to the span even after calling span.finish(). The tracer can keep a buffer of active spans indexed by immutable span contexts that are kept by the user code. When span is finished the tracer can reuse the span object for another trace. if a later call from user code comes with span context not in the table, trace can either ignore the call, or capture that data by other means (if the backend supports merging of spans).

[Oberon00]

First, I think this issue belongs more to the spec repository than the OTEP repository (even though an OTEP may come out of it).

Second, as I see it the main change here is that for each Span operation you now have the Tracer as an additional input (you could just implement Span as a reference to SpanContext and a Tracer and have the public methods of Span delegate to Tracer methods of the form you suggested). Is that correct?

This has implications for the SpanContext too, because we probably would want some additional property like int activeSpanIndex otherwise we force something like a HashMap access for each Span operation. Then the question is who has (read or write) access to this property? Should it be defined in the SDK or API? If in the SDK, we are back to #58. And so on.

[jmacd]

This feels like a Go-specific proposal to me, and it's one that's been discussed in the past I'm sure many times. The question is, even if every operation on a Span is equivalent to some Tracer operation on a context, should we still have a Span type? I can be convinced not, in this case, but there's a specific line in the Golang Context documentation that causes some concern:

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

I personally disagree with the advice not to store Contexts, but as a compromise to some of my co-workers, whenever I've done so I embed it in another struct and clearly document the behavior of the stored context, i.e., never store a bare Context, only store contexts in derived Context structures with a clear intent. Then, we can remove the Span type.

[yurishkuro]

@Oberon00

you could just implement Span as a reference to SpanContext

Yes, but that's the point - if it can be done, why have the Span object in the API?

I agree that we'd need to revisit #58.

@jmacd I didn't follow your train of though. How is this related to storing Context anywhere? Context is a propagation mechanism, which is independent of Span vs. SpanContext duality. The code examples I had are Go specific, but in another language you can just replace ctx with spanCtx.

[carlosalberto]

I remember that we mentioned back in OpenTracing having an abstraction layer (working on top of the OT api) that effectively would hide these details, so you would always work with the current Span, as depicted in your example. I feel this approach would be great, as it would simplify things for 'end users', but wouldn't probably help with optimizations like the Span pooling you mentioned ;)

[tedsuo]

@yurishkuro Josh means that some gophers refuse to store a reference to a Context. So if you take away their Span handle, they have nothing to store but the Context, and they are sad. This comes from a literal reading of the following advice from https://golang.org/pkg/context/

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx

I won't go into it here, but there are enough edge cases that a literal interpretation of this advice is silly. Occasionally, you have to store a context for use later.

@carlosalberto this is the old example from OpenTracing Java. It's easy to see how this change would make the instrumentation more readable, in just about every language: https://docs.google.com/presentation/d/1FKnzs19iuJyDIy0cHffgC8H1B4y_U4sXDw7N_YswnQw/edit

[jmacd]

I fully support this proposal, also I'm prepared to stand before any and all gophers and state that storing Contexts is the right thing to do. I appreciated the related discussion here: https://github.com/open-telemetry/oteps/pull/66/files#r347198758

[bogdandrutu]

There may be cases where it is necessary to have a Span object, for example an interceptor pattern like this:

Interceptor {
  onStart(Headers);

  Context beforeExecutedHandler(Context);

  onEnd();
}

The Interceptor implementation needs to create a Span in the onStart and install the Span in the Context later. With the proposed API this is not possible because it is not easy to "merge" contexts in the beforeExecutedHandler.

So I would definitely not rush a decision like this before v1.

[lmolkova]

Is this proposal specific for Go and if not, how it could work for other languages?

It seems to imply that all spans have to be active and that language allows to rely on current spans in all cases. I don't think this is the case for Java or C++. Even for languages with full async support, it's not always possible to use the current span reliably.

[tsloughter]

I'm a +1 on this. It at least applies to Erlang/Elixir, so not only Go.

It is also basically how the Erlang impl already works. We provide an additional interface that sits on top of the official API, a module named otel.

We also don't allow modifications to a span after it has been finished. A finished span is moved to a new buffer and out of the active spans buffer. I'm guessing this is related to what @yurishkuro is talking about with simplifying memory management.

Actually, yes:

The tracer can keep a buffer of active spans indexed by immutable span contexts that are kept by the user code.

This is exactly how the Erlang impl works. And when end_span is called the span is removed from the active spans and moved to a separate buffer that is used by the batch processor (assuming the batch processor has been added as a span processor).

[jmacd]

There was some discussion about this in a call about Context Propagation today. I would be satisfied with a specification update stating that Span objects are logical constructs which refer to a span context. The notion of "CurrentSpan" returning a span is really just semantic sugar for a type which encapsulates a span context. I believe this addresses your concern, without needing to actually remove Span from the specification. It would allow a valid implementation to be stateless, it allows a meaningful interpretation for span events that happen after Spans finish, and so on.

[tedsuo]

@lmolkova I believe it implies that we can simply when adding a context object to every language. Even if this context object is automatically propagated most of the time, there are cases where the context object must also be instantiated. So, the proposal is that all operations work directly on this context object, rather than first extracting a span from the context.

A pseudocode example, with no automated context propagation:

// operate on the context object directly
SetSpanAttribute(context, "foo", "bar")

// rather than first grab a span handle
span = GetSpan(context)
span.SetAttribute("foo", "bar")

When automated context propagation is available, then the context object is implied.

// use the context directly
SetCurrentSpanAttribute("foo", "bar")

// rather than grab a span handle
span = GetCurrentSpan()
span.SetAttribute("foo", "bar")

Is that a helpful description? I'm not proposing it would look exactly like this in C#, but I do think the result would be a more declarative API, that hides the span object as an implementation detail. I think this leaves more room for improvement in the future, as the API now exposes fewer details. I believe .Net uses activities as the equivalent of "context" in this case.

BTW, it is this OTEP (#66) proposing a separate context layer which is bringing this discussion up. I agree with @bogdandrutu that we have to confirm that edge cases like moving contexts still work. I actually do not think it is complicated to move/merge a span from one context to another, but we would need to create tests and examples for all of these cases first. So I would not want to rush a big change. Perhaps in some cases, this is just sugar to make the API more declarative and take up fewer lines of code.

[lmolkova]

So my understanding is that this simply eliminates Span interface:

1. we have some kind of handler for the span.
2. we keep all the Span interface APIs to enrich spans, but expose them through different class (Tracer or whatever)
3. We keep readable Span in some shape for exporters

With .NET team we are entertaining the idea of providing all Span capabilities in the platform (which simplifies dependency management enormously). So this is definitely an interesting proposal that helps with this idea.

At the same time, it seems we need something in the SDK to store all active spans (which is not trivial to do right) AND we still ask users to carry handler around AND we keep all the same methods on the different interface.

If we want to do this in v1, I hope to see some real benefits for this choice. I believe there are some, but I'm not sure what is ROI. But after v1, Span interface would need to stay for a while and it might not make sense to do anything about it.

[tedsuo]

@lmolkova yes I really like the way that .NET is going with this, I hope other language runtimes eventually catch up. :)

I'm not convinced that this would be a major change to the current implementations in other languages, even though it changes the API significantly.

For each language, it would need to be done as a very complete PR, with the edge cases handled and benefit shown; and not dribbled out. I would not want to block the main line of SDK work with this.

[jmacd]

I will vote to keep the Span type but change the specification for Span to define it as semantic sugar, making it in every way equivalent to the proposal in this issue but without removing the Span interface. The original Go "streaming" SDK was implemented this way. A Span was effectively just a context and all operations recorded an event with the context and operation details.

https://github.com/open-telemetry/opentelemetry-go/blob/4172bdfbf9def7962191c955228b7f9e4561466c/experimental/streaming/sdk/span.go#L29

IOW I believe a valid SDK can already avoid the memory management problem -- there is no requirement for an SDK to maintain state in memory to implement a Span. I still believe the specification should be firmed up to say that Span is semantically equivalent to a context.

[jmacd]

I believe this is a very important topic. Please see the related/opposed issue #73.

I believe it's an ergonomic loss to remove the Span API. One of the points of confusion that could arise, were Span to be removed, is that it becomes easier to mis-use the API by recording a single Span to multiple Tracers (i.e., you could use one Tracer for the Start event, one for the End event).

[lmolkova]

basically it requires to expose Span APIs on the span context/handler, not the tracer.

There is already enough confusion around current span on the tracer open-telemetry/opentelemetry-specification#108 (is it current for this tracer or current globally).

[tsloughter]

There is already enough confusion around current span on the tracer open-telemetry/opentelemetry-specification#108 (is it current for this tracer or current globally).

And with this change it is current for the current context, right?

I'd never heard of current span being tied to a tracer or globally, this may be why I've always been so confused in discussions about current/active spans?

I wanted to reiterate that I'm a big +1 on this and really hope the spec goes this way. It is already basically how the Erlang/Elixir implementation works, only basically because to keep the API's for tracer and span modules roughly the same as the spec we have an additional module that acts as a layer above them to combine their APIs.

[Oberon00]

I think this proposal should receive some serious consideration again now that OTEP 66 (Context OTEP) is merged. Although now, Span should arguably not be replaced by SpanContext (we can remove that too, #73), but OTEP 66's Context + operations on the Tracer.

[Oberon00]

While we would have had the chance to do this with OTEP66, I think it's too late now. Can we close this?