Security of ending spans

[NthPortal]

Currently, all spans expose end methods, allowing even users of managed spans to end them prematurely and potentially cause problems. Additionally, if we end up exposing the current span in Tracer, arbitrary callers may be able to end spans prematurely. Should the API be changed to prevent users other than callers of startUnmanaged from calling end on a span?

[iRevive]

We tried this approach a while ago, and it was nice to have two slightly different spans with different sets of operations, but we didn't find it successful or beneficial.

While I think it's a good and secure design of the API, there are several hiccups:

• startUnmanaged does not propagate context automatically. It means you need to pass the span everywhere you need it explicitly. So it's hard to call 'startUnmanage' as an alternative to 'start' with the only difference that you need to call end manually

• Spec says we should provide the end operation

Though we can cheat here, in some sense:

Language SIGs MAY provide methods other than End in the API that also end the span to support language-specific features like with statements in Python. However, all API implementations of such methods MUST internally call the End method and be documented to do so.

• There can be scenarios when you want to end the span manually at any given time

And in the end, it's up to a user to use the library correctly. If someone wants to go yolo and do unsafe things, we cannot prevent it. The behavior and consequences are transparent, IMO.

[NthPortal]

And in the end, it's up to a user to use the library correctly. If someone wants to go yolo and do unsafe things, we cannot prevent it.

I'm not sure I agree with this; it's quite common to try to prevent both mistakes and malicious use in languages and libraries. in Java for example, Object lock = new Object(); synchronized(lock) is often used rather than synchronized(this) to prevent other users from reducing the performance of a synchronized block of code. so I don't think it would be unusual to try to guide people towards safer usage in this library as well

[iRevive]

in Java for example, Object lock = new Object(); synchronized(lock) is often used rather than synchronized(this) to prevent other users from reducing the performance of a synchronized block of code

Yes and it happens under the hood. It is the same thing with cats-effect that uses mutable data structures internally to keep on par with performance.

For example, OpenTelemetry Java allows terminating span at any point of time:

val tracer: io.opentelemetry.api.trace.Tracer = ???
val span = tracer.spanBuilder("span").startSpan()
...
span.end()

If we strip this functionality away from span, how can a user end it earlier? Yes, it's rather a rare case, but it may be viable in some non-trivial applications.

You cannot use startUnmanaged instead because it does not propagate:

for {
  span <- Tracer[IO].spanBuilder("span").build.startUnmanaged
  _ <- IO.println(span.context)
  _ <- Tracer[IO].span("child").use(childSpan => IO.println(childSpan.context))
  _ <- span.end
} yield ()

The output is:

WrappedSpanContext(ImmutableSpanContext{traceId=a5dcbbb6ed763253c707b9ec9c248ef2, spanId=cd4a3f0c5de61530, traceFlags=01, traceState=ArrayBasedTraceState{entries=[]}, remote=false, valid=true})
WrappedSpanContext(ImmutableSpanContext{traceId=95733b28962074dceaedb3b45adc0480, spanId=430308498d64b634, traceFlags=01, traceState=ArrayBasedTraceState{entries=[]}, remote=false, valid=true})

So, we have two root spans there.

We can allow making any span 'current', e.g. Tracer[F].makeCurrent(span). But it will go against this advice:

it's quite common to try to prevent both mistakes and malicious use in languages and libraries

Because we provide a way to overrule the default propagation of a span at any point in time. Which is as bad as ending the span earlier.

Someone can always find a way to break even a perfectly designed library.

[iRevive]

@NthPortal I don't think we should entirely give up on this design. I agree that the lifecycle should be encapsulated and fully managed by the internals.

If we find the right option of making 'startUnmanaged' propagatable, we can consider changing the encoding and stripping 'end' from the managed span.

Also, 'startUnmanaged' could be the exact solution to the tracing within the resource and stream. So it is worth exploring.