SpanProcessor API - Storing a Span specific state

[sfriberg]

As part of doing a PoC for JFR for OT I realized that I need to keep a state in the SpanProcessor so I can start the JFR event for the Span and end it when the span is ended. For the Context this is rather straight forward with a ThreadLocal as it is (should) always be started/ended in the same thread, however Spans do not have that guarantee and thus I need to currently have a shared data structure in the processor for handling the state for different spans.

In the current implementation I did a simple ConcurrentHashMap which works, but has the potential for some so contention for applications with many parallel threads and spans.

I did a basic implementation having the onStart function return the onEnd function as a callback, which looks OK for the most part, but does increase allocation in handling and in particular the MultiSpanProcessor, also do not know how to make it work with the Batch/Async processor as the callback wouldn't be returned immediately.

public interface SpanProcessor {
    @Nullable
    Consumer<ReadableSpan> onStart(ReadableSpan span);
    void shutdown();
    void forceFlush();
}

Another option could be to extend the ability to add a SpanProcessor state to the ReadableSpan object. Could be stored in a map and hashed on SpanProcessor instance.

Any other thoughts or ideas @jkwatson @bogdandrutu? Be happy to try and do some test implementations.

[carlosalberto]

Another option could be to extend the ability to add a SpanProcessor state to the ReadableSpan object.

Just for the record, I had wanted to add something like this to Span when writing the OT Shim, for basically the same reasons (potential contention with many threads). It's not beautiful for sure, but it would be handy ;)

[jkwatson]

Having some proof-of-concept implementation options would be great, @sfriberg

[Oberon00]

IMHO, the span processor should be able to write to the span too. And indeed in the spec only Span (not ReadableSpan) is specified thus consequently we are violating the spec by not allowing SetAttribute in the span processor. Not that this would solve the problem at hand.

Tangentially related, it would also be nice if something could be stored in the SpanContext (so that it is propagated to children), see open-telemetry/opentelemetry-specification#525 (comment)

[jkwatson]

IMHO, the span processor should be able to write to the span too. And indeed in the spec only Span (not ReadableSpan) is specified thus consequently we are violating the spec by not allowing SetAttribute in the span processor. Not that this would solve the problem at hand.

Looks like it's supposed to be a readable span from the details in here: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#onstartspan

[Oberon00]

Hmm, it says readable not read-only though, so I guess at least the clarity could be improved there. And then there is still the theoretical "thread name tagging SpanProcessor" use case that would require a writable interface. EDIT: See also open-telemetry/opentelemetry-specification#205 (comment)

[carlosalberto]

IIRC yes, SpanProcessor should be allowed to modify the Span instances.

Hmm, it says readable not read-only though, so I guess at least the clarity could be improved there.

Agreed. Shall we fill an issue for this?:

[bogdandrutu]

@sfriberg I would like to see a small prototype of this idea. I think overall is a good addition to the API.

In order maybe to keep span read-only we can simply just return an object from start and pass that object back to the end call.

[jkwatson]

I think the spec and the code have both been updated to provide a ReadWriteSpan to the onStart. Is that good enough for this, or do we still need some other functionality?

[Oberon00]

I don't think this solves the issue in the description which would require storing an arbitrary object that should not be exported

[iNikem]

Should SpanProcessor accept the whole Context and not just a Span?

[Oberon00]

Actually it should receive the full parent Context in OnStart according to the spec. But I'm not sure how helpful that is for this use case either. A Span has no single associated context that would make sense for End, though you can access Context.current of course. Maybe you can do some custom context handling with that, but you can't create a new parent context in OnStart. Maybe the sampler could be extended to support this? It's not related to sampling though...

[sfriberg]

Just updated the PR, #963, with the new SpanProcessor API and the ReadWrite span. As mentioned it doesn't seem to allow me to add something like a transient value to it, and also the context seems to be the parent context so unclear what I can add there and how I would get access to it in the onEnd as I get no context in that call.