[Trace SDK] Input of SpanProcessor::OnEnd should be a readable span

[luyor]

According to the specification, input of SpanProcessor::OnEnd should be a readable span.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#onendspan

#### OnEnd(Span)

`OnEnd` is called after a span is ended (i.e., the end timestamp is already set).
This method MUST be called synchronously within the [`Span.End()` API](api.md#end),
therefore it should not block or throw an exception.

**Parameters:**

* `Span` - a [readable span object](#additional-span-interfaces) for the ended span.
  Note: Even if the passed Span may be technically writable,
  since it's already ended at this point, modifying it is not allowed.

**Returns:** `Void`

Current input of SpanProcessor::OnEnd is a Recordable,

opentelemetry-cpp/sdk/include/opentelemetry/sdk/trace/processor.h

Line 47 in 9efb7a1

virtual void OnEnd(std::unique_ptr<Recordable> &&span) noexcept = 0;

but Recordable only contains setters for span attributes.

https://github.com/open-telemetry/opentelemetry-cpp/blob/main/sdk/include/opentelemetry/sdk/trace/recordable.h

I want to filter out some spans by attributes in SpanProcessor::OnEnd, but it's not doable if span attributes cannot be read.

[lalitb]

Recordable interface allows for the more optimized implementation of trace pipeline by avoiding intermediate copying of data to immutable/readable Span. The implementation of Recordable is provided by the exporter, and the span data is directly serialized into the format required by the exporter.

Just curious - Are you trying to create a custom span-processor with filtration support? Instead of trying to do this on the processor, you can create a custom Sampler which takes this decision based on the initial set of attributes of the Span. Have a look at the default samplers provided by the SDK - https://github.com/open-telemetry/opentelemetry-cpp/tree/main/sdk/src/trace/samplers

[luyor]

My use case is to decide whether span needs to be collected based on status code. It can't be done in Sampler, since sampler is called when span starts, but status code is available only when span ends.

I think SpanProcessor.OnEnd is a valid place to do filtering based on status code. The specification also mentions this:

SDK MUST allow users to implement and configure custom processors and decorate built-in processors for advanced scenarios such as tagging or filtering.

Avoid copying Recordable data to immutable span is reasonable. Maybe readonly interface can be provided by Recordable, and exporter can implement the way to read Span data from serialized format.

[lalitb]

Maybe readonly interface can be provided by Recordable, and exporter can implement the way to read Span data from serialized format.

That looks to be the fair ask, though it would be a breaking change for any external exporters (not part of this repo) and needs to be communicated ahead of time. @pyohannes - Just in case you have any comments, as I think you were involved in the initial recordable design :) One concern is it would increase the effort for devs to implement exporters, as now they need to provide both getters and setters for the span.

[ThomsonTan]

Sounds like we need pass read only span to OnEnd instead of the current recordable? Like it is unexpected to change/set span data in OnEnd?

[pyohannes]

I think there's no general solution for this. For some implementations of Recordable it might be impossible to implement setters at all.

One possibility I see is extending the Recordable interface by a method like this:

virtual explicit operator SpanData*() const {
    return nullptr;
}

Then Recordables can implement casting to readable SpanData where possible. This can then be used by processors. Recordables for which reading data is not possible would return nullptr, in this case the processor can't access the data.