Clarify behavior of Span.End, interaction with IsRecording (#1011)

* Clarify behavior of Span.End.

* Add CHANGELOG.

* Update specification/trace/api.md

Co-authored-by: Yuri Shkuro <[email protected]>

* Polish wording for end/timestamp more.

* End is kinda the only way to end a span.

* Tweak wording of IsRecording after SIG mtg.

* Add to compliance matrix.

Co-authored-by: Yuri Shkuro <[email protected]>

CHANGELOG.md

@@ -107,6 +107,8 @@ Updates:
   ([#995](https://github.com/open-telemetry/opentelemetry-specification/pull/995))
 - Remove custom header name for Baggage, use official header
   ([#993](https://github.com/open-telemetry/opentelemetry-specification/pull/993))
+- Trace API: Clarifications for `Span.End`, e.g. IsRecording becomes false after End
+  ([#1011](https://github.com/open-telemetry/opentelemetry-specification/pull/1011))
 
 ## v0.6.0 (07-01-2020)
 

spec-compliance-matrix.md

@@ -38,6 +38,7 @@ status of the feature is not known.
 |End                                           | + | +  | + | +    | +  | +    | + | +  | + | +  |
 |End with timestamp                            | + | +  | + | +    | +  | +    | + | -  | + | +  |
 |IsRecording                                   | + | +  | + | +    | +  | +    | + |    | + | +  |
+|IsRecording becomes false after End           |   |    |   |      |    |      |   |    |   |    |
 |Set status                                    | + | +  | + | +    | +  | +    | + | +  | + | +  |
 |Safe for concurrent calls                     | + | +  | + | [-](https://github.com/open-telemetry/opentelemetry-python/issues/1157)    | +  | +    | + | +  | + | +  |
 |[Span attributes](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes)|

specification/trace/api.md

@@ -359,6 +359,12 @@ created in another process. Each propagators' deserialization must set
 `IsRemote` to true on a parent `SpanContext` so `Span` creation knows if the
 parent is remote.
 
+Any span that is created MUST also be ended.
+This is the responsibility of the user.
+API implementations MAY leak memory or other resources
+(including, for example, CPU time for periodic work that iterates all spans)
+if the user forgot to end the span.
+
 #### Determining the Parent Span from a Context
 
 When a new `Span` is created from a `Context`, the `Context` may contain a `Span`
@@ -411,7 +417,12 @@ Returns true if this `Span` is recording information like events with the
 `AddEvent` operation, attributes using `SetAttributes`, status with `SetStatus`,
 etc.
 
-There should be no parameter.
+After a `Span` is ended, it usually becomes non-recording and thus
+`IsRecording` SHOULD consequently return false for ended Spans.
+Note: Streaming implementations, where it is not known if a span is ended,
+are one expected case where `IsRecording` cannot change after ending a Span.
+
+`IsRecording` SHOULD NOT take any parameters.
 
 This flag SHOULD be used to avoid expensive computations of a Span attributes or
 events in case when a Span is definitely not recorded. Note that any child
@@ -558,17 +569,31 @@ Required parameters:
 
 #### End
 
-Finish the `Span`. This call will take the current timestamp to set as `Span`'s
-end time. Implementations MUST ignore all subsequent calls to `End` (there might
-be exceptions when Tracer is streaming event and has no mutable state associated
-with the `Span`).
+Signals that the operation described by this span has
+now (or at the time optionally specified) ended.
+
+Implementations SHOULD ignore all subsequent calls to `End` and any other Span methods,
+i.e. the Span becomes non-recording by being ended
+(there might be exceptions when Tracer is streaming events
+and has no mutable state associated with the `Span`).
+
+Language SIGs MAY provide methods other than `End` in the API that also end the
+span to support language-specific features like `with` statements in Python.
+However, all API implementations of such methods MUST internally call the `End`
+method and be documented to do so.
+
+`End` MUST NOT have any effects on child spans.
+Those may still be running and can be ended later.
 
-Call to `End` of a `Span` MUST not have any effects on child spans. Those may
-still be running and can be ended later.
+`End` MUST NOT inactivate the `Span` in any `Context` it is active in.
+It MUST still be possible to use an ended span as parent via a Context it is
+contained in. Also, any mechanisms for putting the Span into a Context MUST
+still work after the Span was ended.
 
 Parameters:
 
-- (Optional) Timestamp to explicitly set the end timestamp
+- (Optional) Timestamp to explicitly set the end timestamp.
+  If omitted, this MUST be treated equivalent to passing the current time.
 
 This API MUST be non-blocking.
 
@@ -621,6 +646,8 @@ The behavior is defined as follows:
   are not being recorded, i.e. they are being dropped.
 
 The remaining functionality of `Span` MUST be defined as no-op operations.
+Note: This includes `End`, so as an exception from the general rule,
+it is not required (or even helpful) to end a Propagated Span.
 
 This functionality MUST be fully implemented in the API, and SHOULD NOT be overridable.