From ec58e4507c1966a0537804a8b81776921569b385 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20Neum=C3=BCller?= Date: Mon, 12 Oct 2020 10:41:38 +0200 Subject: [PATCH] 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 * 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 --- CHANGELOG.md | 2 ++ spec-compliance-matrix.md | 1 + specification/trace/api.md | 43 +++++++++++++++++++++++++++++++------- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb1aaa38e7d..a532bba5e0d 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/spec-compliance-matrix.md b/spec-compliance-matrix.md index da34c377c8e..e7fb73a0f50 100644 --- a/spec-compliance-matrix.md +++ b/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)| diff --git a/specification/trace/api.md b/specification/trace/api.md index d4f7f026b0b..f8f75aa58b1 100644 --- a/specification/trace/api.md +++ b/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.