Trace SDK: Provide definitions for readable and read/write span.

[Oberon00]

Resolves #663 "Allow for global processing of spans in SpanProcessor".
Required for #158 "Describe proto.Span class".

• OnStart must be read/write.
• OnEnd must be readable , may additionally be writable.

Also clean up related wording for span exporters that couldn't decide whether they want to work for different kinds of telemetry data or only spans (they work only for spans).

[Oberon00]

linkcheck repeatedly failed with HTTP 429 for github.com, markdown should be clean though.

[anuraaga]

Thanks! The change looks good and clears up what read, vs read/write span are well for me.

[anuraaga]

I went ahead and spiked a PoC for opentelemetry-java, changing both onStart and onEnd to ReadWriteSpan.

https://github.com/open-telemetry/opentelemetry-java/compare/master...anuraaga:span-processors-can-process-more?expand=1#diff-b4c9e6de11a1250eb4d22d36ddd32ec9R171

[Oberon00]

Sounds great to me, but I'd like also to ask @bogdandrutu for his opinion on this ;)

-- #669 (comment)

@carlosalberto @bogdandrutu Anything I should do here?

[Oberon00]

Note that this PR is now linked to #158 which is required-for-ga.

[Oberon00]

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers This PR is waiting for reviews.

[bogdandrutu]

Is this really needed? This may make user to do hacks. Imagine we have this interface in the OnStart, they know that the object we return implements both, OnEnd we return only read, then they will guess is the same interface and may do a cast to the ReadWrite interface.

My suggestion is that Span is the write interface and ReadableSpan is the read interface, if access required to both we should pass both interfaces. Maybe I am to "evil" and think of possible hacks and abuses, but I've seen all of these happening in my previous life :)

[Oberon00]

You could pass a wrapper object that only implements the readable interface to protect against that.

I think we can mention passing the read-only and write-only interface separately as an allowed implementation option for read/write span though.

[anuraaga]

@Oberon00 suggested I do that hack when I need to mutate the span in OnEnd :) I think there is a need to be able to mutate spans globally before export (usually to use other tags as signals to user-defined domain-specific tags) so hope we can find a way. The issue we were worried about is the ordering between OnEnd and export

[Oberon00]

More importantly, spans are already ended at that point and can't be modified anyway. #669 (comment)

[Oberon00]

@bogdandrutu I incorporated the "read/write implemented as two separate objects" option in 0b9709d.

[anuraaga]

@Oberon00 suggested I do that hack when I need to mutate the span in OnEnd :) I think there is a need to be able to mutate spans globally before export (usually to use other tags as signals to user-defined domain-specific tags) so hope we can find a way. The issue we were worried about is the ordering between OnEnd and export

[iNikem]

Pinging @open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers as I am also interested in this PR getting merged.

[Oberon00]

@iNikem It would be cool if you had time to submit a review yourself. Grey checkmarks also mean something 😃

[iNikem]

Thinking some more about the original intention of @anuraaga in #663 , @bogdandrutu comment about ReadableSpan vs SpanData about this use-case and how SpanProcessor is currently described here , I think the actual problem is different.

What Anuraag and I want is a composable, external function Read/Write SpanData-> Read/Write SpanData. Which can read all the existing info and add some new info. SpanProcessor is on the critical path of the application. One would argue that doing arbitrary data enrichment on critical path is not a good idea.

Exporters, on the other hand, are off the critical path. If we update spec to make it explicit that there can be a chain/pipeline of exporters, and that they can be a place for data enrichment, that would solve the underlying issue in #663

[Oberon00]

Yeah, the SpanProcessor is not well-suited for computing additional attributes. @anuraaga said he'd create a new issue/PR for this (#669 (comment)). What is more, OnEnd can only get a read-only span (a ended + writeable span is not a thing #669 (comment)).

I think OnStart should be able to write to the Span, so that the SpanProcessor can capture some things from the current context. E.g. a thread name, a current request ID (many web frameworks offer a way to access the current web request via a global function), etc.

[anuraaga]

Yeah I've been waiting to confirm the direction of this one before filing another issue about enriching spans, whether using processor or exporter to avoid confusion as they are related topics. So hoping for a merge :-)

[Oberon00]

I also agree that it is better to get this basic readable vs read+write definitions sorted out before making follow-up changes. I'm also hoping for a merge. 😃

@carlosalberto if you approve of this PR, I think it is ready to be merged. I don't think we need to wait for @bogdandrutu anymore as I think I resolved all his comments and he hasn't responded for 10 days.

I will also post a request for reviews on gitter.

[carlosalberto]

@Oberon00 I think the section describing the option to have a Read/Write Span as (two) separate objects might not be clear enough for newcomers ;) But we can work on it if/as/when needed, so I'm ok with the current PR.

Lets wait till EOD to give a last opportunity for @bogdandrutu to comment.

[Oberon00]

I agree that this section has become rather convoluted. I have an idea in mind where we would not list parameters for callbacks one by one but rather just say which operations they must be able to do. But I'd prefer to do that in a follow up if possible.