Spec Conformance Review: Span End

[MrAlias]

• Review End operation for a Span and identify all normative requirements it contains.
• Validate our implementation of the specification to conform or not. If it conforms, document the conformity here. If it does not open an Issue or PR track the work.

[MrAlias]

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).

We gate span methods that should have no effect after End is called with the IsRecording method. This method, currently, checks if end time is set:

opentelemetry-go/sdk/trace/span.go

Line 168 in 90bd4ab

return s.endTime.IsZero()

This is set to non-zero when a span is ended:

opentelemetry-go/sdk/trace/span.go

Lines 244 to 249 in 90bd4ab

	// Setting endTime to non-zero marks the span as ended and not recording.
	if config.Timestamp.IsZero() {
	s.endTime = et
	} else {
	s.endTime = config.Timestamp
	}

Note: there is somewhat related bug, but it is more related to starting a span and the IsRecording method itself.

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.

We do not provide such functions currently.

End MUST NOT have any effects on child spans.
Those may still be running and can be ended later.

It does not. Spans to not, currently, keep reference to any child spans other than a running total of how many are created.

End MUST NOT inactivate the Span in any Context it is active in.

There is a concept of active Span in the project. There is no process to make a span (active or) inactivate.

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.

There are no restrictions in place. The parent span is stored, but no check is in place for if it has ended in the creation of a new span:

opentelemetry-go/sdk/trace/span.go

Lines 521 to 590 in 90bd4ab

	func startSpanInternal(ctx context.Context, tr *tracer, name string, parent trace.SpanContext, remoteParent bool, o *trace.SpanConfig) *span {
	span := &span{}
	cfg := tr.provider.config.Load().(*Config)
	var tid trace.TraceID
	var sid trace.SpanID
	if hasEmptySpanContext(parent) {
	// Generate both TraceID and SpanID
	tid, sid = cfg.IDGenerator.NewIDs(ctx)
	} else {
	// TraceID already exists, just generate a SpanID
	tid = parent.TraceID()
	sid = cfg.IDGenerator.NewSpanID(ctx, tid)
	}
	span.spanContext = trace.NewSpanContext(trace.SpanContextConfig{
	TraceID: tid,
	SpanID: sid,
	TraceFlags: parent.TraceFlags(),
	TraceState: parent.TraceState(),
	})
	span.attributes = newAttributesMap(cfg.SpanLimits.AttributeCountLimit)
	span.messageEvents = newEvictedQueue(cfg.SpanLimits.EventCountLimit)
	span.links = newEvictedQueue(cfg.SpanLimits.LinkCountLimit)
	span.spanLimits = cfg.SpanLimits
	data := samplingData{
	noParent: hasEmptySpanContext(parent),
	remoteParent: remoteParent,
	parent: parent,
	name: name,
	cfg: cfg,
	span: span,
	attributes: o.Attributes,
	links: o.Links,
	kind: o.SpanKind,
	}
	samplingResult := makeSamplingDecision(data)
	if isSampled(samplingResult) {
	span.spanContext = span.spanContext.WithTraceFlags(span.spanContext.TraceFlags() | trace.FlagsSampled)
	} else {
	span.spanContext = span.spanContext.WithTraceFlags(span.spanContext.TraceFlags() &^ trace.FlagsSampled)
	}
	span.spanContext = span.spanContext.WithTraceState(samplingResult.Tracestate)
	if !isRecording(samplingResult) {
	return span
	}
	startTime := o.Timestamp
	if startTime.IsZero() {
	startTime = time.Now()
	}
	span.startTime = startTime
	span.spanKind = trace.ValidateSpanKind(o.SpanKind)
	span.name = name
	span.hasRemoteParent = remoteParent
	span.resource = cfg.Resource
	span.instrumentationLibrary = tr.instrumentationLibrary
	span.SetAttributes(samplingResult.Attributes...)
	span.parent = parent
	return span
	}

and

opentelemetry-go/sdk/trace/tracer.go

Lines 40 to 80 in 90bd4ab

	func (tr *tracer) Start(ctx context.Context, name string, options ...trace.SpanOption) (context.Context, trace.Span) {
	config := trace.NewSpanConfig(options...)
	parentSpanContext, remoteParent, links := parent.GetSpanContextAndLinks(ctx, config.NewRoot)
	if p := trace.SpanFromContext(ctx); p != nil {
	if sdkSpan, ok := p.(*span); ok {
	sdkSpan.addChild()
	}
	}
	span := startSpanInternal(ctx, tr, name, parentSpanContext, remoteParent, config)
	for _, l := range links {
	span.addLink(l)
	}
	for _, l := range config.Links {
	span.addLink(l)
	}
	span.SetAttributes(config.Attributes...)
	span.tracer = tr
	if span.IsRecording() {
	sps, _ := tr.provider.spanProcessors.Load().(spanProcessorStates)
	for _, sp := range sps {
	sp.sp.OnStart(ctx, span)
	}
	}
	ctx, span.executionTracerTaskEnd = func(ctx context.Context) (context.Context, func()) {
	if !rt.IsEnabled() {
	// Avoid additional overhead if
	// runtime/trace is not enabled.
	return ctx, func() {}
	}
	nctx, task := rt.NewTask(ctx, name)
	return nctx, task.End
	}(ctx)
	return trace.ContextWithSpan(ctx, span), span
	}

Parameters:
(Optional) Timestamp to explicitly set the end timestamp.

opentelemetry-go/trace/config.go

Lines 149 to 153 in 90bd4ab

	// WithTimestamp sets the time of a Span life-cycle moment (e.g. started,
	// stopped, errored).
	func WithTimestamp(t time.Time) LifeCycleOption {
	return timestampSpanOption(t)
	}

If omitted, this MUST be treated equivalent to passing the current time.

opentelemetry-go/sdk/trace/span.go

Lines 245 to 249 in 90bd4ab

	if config.Timestamp.IsZero() {
	s.endTime = et
	} else {
	s.endTime = config.Timestamp
	}

This API MUST be non-blocking.

This method needs to acquire a lock to set the end time so it does not have a race with any other calls to the IsRecording method of a span. This is a requirement from the fact that Span methods need to be concurrent safe according to the specification. From other discussions, this non-blocking requirment usually means "does not take excessive time to complete", which it should not based on the simple flow of IsRecording and this method.

I'm opening this to see about clarifying this requirement.