Opentelemetry-go: Align span interfaces with the spec

[johananl]

Following the discussion here, we should take another look at the way we use export.SpanData in sdk/trace.

The spec mentions two types of interfaces to a span at the SDK level:

• A read-only span
• A read/writable span

Right now we use export.SpanData in the SDK as a "container" for any information about a span which may need to be read, for example for the sake of exporting a span. We store a pointer to a SpanData struct in a span's data field and we make a copy of the SpanData using makeSpanData() when we want to prevent modifications to an ended span:

https://github.com/open-telemetry/opentelemetry-go/blob/27aa1f601124468cde6c796935460435b6723700/sdk/trace/span.go#L52

https://github.com/open-telemetry/opentelemetry-go/blob/27aa1f601124468cde6c796935460435b6723700/sdk/trace/span.go#L146

We should see if there is a cleaner, Go-idiomatic way to express the two "behaviors" of the span inside the SDK and refactor accordingly.

[johananl]

@MrAlias please feel free to assign me.

[johananl]

After reading the relevant parts of the spec and looking at the way things are done at the Java library, I'm thinking about the following design:

• Keep trace.Span at the API as the write-only span type used for instrumentation.
• Add a ReadableSpan interface to the SDK which would allow reading all data from the internal state of the (unexported) span type. This interface returns data directly from the original span created by the tracer, not from a snapshot of the span, meaning it will keep reflecting changes to the original span if relevant.
  
  
  
  • This interface shall be used by span processors in OnEnd.
  
  
  • Note: Need to figure out if it's better to have a granular list of methods - one for each span field - or have one method which returns a big struct with all the data.
  
  
• Add a ReadWriteSpan interface to the SDK which would embed trace.Span and ReadableSpan to provide the union of the methods from these two interfaces.
  
  
  
  • This interface shall be used by span processors in OnStart as required by the spec. This means span processors will be able to _modify spans_.
  
  
• Store all span data as fields directly in the SDK span type. Any write operations currently done on a SpanData instance should be changed so that they are performed on the span.
• Change SpanData type to be an immutable snapshot of a span's internal state.
  
  
  
  • This type shall be used by exporters only.
  
  
  • Note: Need to figure out the best way to keep span and SpanData DRY while maintaining an abstraction which makes sense.
  
  

@MrAlias @krnowak - happy to hear your thoughts. To save time I'm starting to work in this direction and will add comments if I encounter any problems.

[MrAlias]

This sounds like a good design to me.

[johananl]

Note: Need to figure out if it's better to have a granular list of methods - one for each span field - or have one method which returns a big struct with all the data.

Talked to @krnowak offline. He suggests using getter methods which "mirror" the setters we already have in trace.Span. Might want to expose only fields which are actually needed right now and not every single trace.Span field.

[johananl]

Also - we might want to improve the naming of the new interfaces. We could maybe do ReadOnlySpan and ReadWriteSpan which is more consistent language-wise.

[johananl]

~WIP is here: https://github.com/johananl/opentelemetry-go/tree/johananl/refactor-spandata~

TODO:

• [x] Store all span data directly under span.
• [x] Add ReadOnlySpan interface and use in the appropriate places instead of export.SpanData.
• [x] Add ReadWriteSpan interface and use in the appropriate places instead of export.SpanData.
• [x] Ensure export.SpanData is immutable and used only for exporting purposes (the docs should be clear about this).
• [x] Allow getting a span's Resource via ReadOnlySpan.
• [x] Increase test coverage for ReadOnlySpan and ReadWriteSpan.