Runs the processors before finishing the span

[pantuza]

Hi 👋🏼

This Pull Request simply moves the processors call to the beginning of the finish() method.

Why?

Otherwise, you can't modify the span getting finished. Processors are run AFTER the span @ended variable
gets updated to true. Thus, the span is locked for changes.

Real use case

Suppose you want to add extra attributes to you spans on the way out the door.

Therefore, you can create a custom processor by implementing a child class of
OpenTelemetry::SDK::Trace::SpanProcessor and overriding the method on_finish().
This method receives the span getting finished and you can then add new attributes, right?

Not exactly, cause by calling add_attributes you receive an error: Calling add_attributes on an ended Span.
Whit that, my personal assumption is that you can not do anything useful inside on_finish() method
of a custom Processor cause the span will always be @ended when this method is called.

The changes

This PR makes the span finish() method to iterate calling every Processor BEFORE actually ending
the span. It will allow processors to modify spans before actually finishing it.

Other argument: on_start method

If you look at the way this Span class calls on_start method, you will see it being the last thing in the
initialize() method. That makes sense, we first create the span, then we call each processor for that particular span.

The on_finish() method should do the opposite: run processors FIRST, then finish the span. Does it make sense?

Considerations

Please, let me know in case you folks think I should do something else for this PR.

[dmathieu]

Heavy 👎 (don't feel discouraged by it though, the need is indeed there)

This change does not conform to the specification. Modifying the span in the onEnd/on_finish method is not permitted.
See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#onendspan

There is a new OnEnding option that SDKs can implement which runs before OnEnd, and which allows spans to be modified. This is what should be implemented in the Ruby SDK to achieve your need.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#onending

[ericmustin]

Just quickly chiming in, tldr this is a limitation of the spec. You bring up valid practical considerations (which I agree with the spirit of!) but we are beholden to the specification that states spans have to be ended before being passed into onend

https://opentelemetry.io/docs/specs/otel/trace/sdk/#onendspan

Looks like there's the concept of "onending" that sortve is what you're looking for, but it's in "development" within the spec https://opentelemetry.io/docs/specs/otel/trace/sdk/#onending
, so I'm not sure what the path forward is there for Ruby.

Some of the more knowledgable folks and maintainers here can provide better context with the precise language on why this is working as intended, and suggested workarounds (monkeypatch the sdk itself, some sort of cloning of spans, etc) or path forward.

[dmathieu]

"in development" means the spec is brand new and may still change, in which case the SDKs would have to change too.
Nothing prevents ruby from adopting this part of the spec right now.

[pantuza]

Thank you @dmathieu for all the clarifications. I totally agree with everything you have mentioned.
And thankfully, the new Spec seems to solve the issue I have tried to report.

Do you folks mind if I try to implement the new Spec? Is there anyone working in such thing already?

[kaylareopelle]

Hi @pantuza! We'd love to have you work on it! I created #1695 to track this work. Please comment on the issue so I can assign it to you.

We've talked about this new feature in the SIG, but no one is working on it AFAIK. Let us know if you need any help 🙂

[pantuza]

I am closing this PR since, based on the suggestion made here, I have opened another Pull Request (#1713) addressing the Open Telemetry Spec.