Clarify behavior of Span.End, interaction with IsRecording

[Oberon00]

Fixes #1010

Changes

• A Span MUST always be ended by the user.
• Ending a Span MUST NOT change it's activeness in any Context and it MUST be possible to use it as a parent even after it was ended.
• After ending a Span it SHOULD become non-recording.

[carlosalberto]

Overall good, thanks for adding this, LGTM.

[Oberon00]

@tsloughter Since you commented on this PR quite a bit, it would be great if you could "formally" review it, and ideally approve if you think the change is good.

[tsloughter]

So I think I'm ok with this because its in the API. My main concern at this point was "MAY leak memory" and "responsibility of the user".

I was going to suggest including something about how implementations can optionally include cleanup code to handle leak cases, but I guess that belongs in the SDK?

In the Erlang SDK we have 2 ways to deal with un-End'ed Spans. One is the "sweeper", this is optionally enabled and configured to look for Spans that have been started and not ended for a configurable amount of time, it can then mark them with an attribute like "ended by sweeper" and End them.

The other is an optional process that monitors processes with started Spans and each Span stores the PID of the process it was started in -- still haven't merged this one as I haven't fixed it to work with Spans started in one process and attached in another -- and if the process crashes the monitor handles cleanup of all the Spans in that process, End'ing them and marking them in some way as having crashed.

Since this is SDK stuff I'm not sure anything needs to be put in the API spec about it.

[Oberon00]

I was going to suggest including something about how implementations can optionally include cleanup code to handle leak cases, but I guess that belongs in the SDK?
In the Erlang SDK we have 2 ways to deal with un-End'ed Spans. One is the "sweeper", [...] the other is an optional process that monitors processes with started Spans

I think it may make sense to add at least the first one to the SDK spec as an optionally available span processor. Also, in Java there is an issue (open-telemetry/opentelemetry-java#1084) about a SpanProcessor that ends warns about spans that are no longer referenced, that could be applicable to other languages with GC.

We could consider adding something to the SDK spec that the default SDK "SHOULD NOT" leak resources even for un-ended spans if it's possible (e.g. in most garbage collected languages, this should be the natural default; only if you grab a (strong) reference in OnStart, then you may cause a leak).

However, I think all this belongs in a follow-up issue. Do you want to create one?

EDIT: And thank you for the review! 😊

[tsloughter]

@Oberon00 yea, I can create another issue or a PR to track that.

[carlosalberto]

@open-telemetry/specs-approvers Feel free to review this PR. We reached agreement on a missing detail regarding recording after Span.end() on the last Spec SIG call, so this shouldn't be polemical. Otherwise, I will merge tomorrow.

[Oberon00]

@carlosalberto or @open-telemetry/technical-committee: I think we can merge this now.

[Oberon00]

Oh, wait a minute, I just got reminded by @arminru to add an entry to the compliance matrix for the IsRecording change. Adding it now...

[Oberon00]

Updated compliance matrix & build is green. So now this should really be ready to merge 😃 @carlosalberto