trace.WithAttributes does not satisfy SpanEndOption

[petenewcomb]

Description

The documentation of trace.WithAttributes states:

These attributes are used to describe the work a Span represents when this option is provided to a Span's start or end events.

However, it returns a SpanStartEventOption instead of a SpanEventOption and SpanStartEventOption satisfies only SpanStartOption and EventOption, not SpanEndOption.

Either the code or the documentation is incorrect. I expect that it's the code that's incorrect, because it's often the case that useful span attribute values are not known before a span's operation has started. This can still be done using Span.SetAttributes, but the text for trace.WithAttributes quoted above implies that the option it produces can be passed to Span.End. The wording is also a bit confusing because it refers to what must be calls to Tracer.Start and Span.End as "events", using that word in a different sense than the "span events" for which this function can be used to supply attributes.

Environment

• OS: N/A
• Architecture: N/A
• Go Version: N/A
• opentelemetry-go version: 1.31

Steps To Reproduce

func foo(ctx context.Context, tracer trace.Tracer) {
	ctx, span := tracer.Start(ctx, "myop")
	span.End(trace.WithAttributes())
}

./prog.go:13:11: cannot use trace.WithAttributes() (value of type trace.SpanStartEventOption) as trace.SpanEndOption value in argument to span.End: trace.SpanStartEventOption does not implement trace.SpanEndOption (missing method applySpanEnd)

(Complete example at https://go.dev/play/p/QpkKDVYmnxp)

Expected behavior

The above code should compile cleanly and behave as already described, or the documentation for trace.WithAttributes should not imply that it can be used with Span.End. In the former case, it would also be good to include text that follows the instructions in the specification:

The API documentation MUST state that adding attributes at span creation is preferred to calling SetAttribute later, as samplers can only consider information already present during span creation.

[dmathieu]

The specification only allows setting the end timestamp, not attributes when calling End().
So this rather looks like a documentation issue.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#end

[petenewcomb]

My read of the specification was that one can set the end timestamp when calling End, not that it allows only the timestamp to be set at End. If one takes the narrower view, then at least on the surface, the Go implementation should not also allow WithStackTrace to be passed to End, instead forcing it to be used only with Span.AddEvent or Span.RecordError.

Digging in a bit further, I can see that the reason that WithStackTrace is accepted by Span.End is to enable or disable capturing a stack trace from a panic. This behavior is not called out by the documentation, and it's also surprising to me that passing WithStackTrace(true) to Span.End will record a stack trace only if the goroutine is currently panicking -- and assuming, of course, that it was called from defer. Usage of defer is suggested by the examples, but like panicking to begin with, it is never noted as being required for this functionality.

Also surprising is that passing WithStackTrace to Span.AddEvent is a no-op. Perhaps there should be an "ErrorOption" interface that WithStackTrace returns, that is accepted only by RecordError and End? Or, to avoid breaking the existing API, Span.AddEvent could simply add a stack trace based on the WithStackTrace flag as would be expected, re-using the capturing and recording mechanisms employed by Span.RecordError and Span.End.

With the possible exception of the issues around AddEvent, I'm fine with just fixing the documentation. I acknowledge that the API as it stands appears to be close to minimal in the sense that once a span has been created, one must use the methods on the created span to do anything other than setting the end timestamp or trigger the panic-related functionality.

Once there's alignment on a general path forward I'll be happy to take a stab at implementing any changes (likely mostly documentation) and raise a PR for further review.

[dmathieu]

I don't know whether we should allow setting attributes while we call End. Setting attributes isn't really related to finishing a span.

Setting the end timestamp is related to finishing the span, since that value is set during that call.
I agree that the stacktrace option is a grey area.

It looks to me like the specification needs to be clarified before we make changes to the API.

[MrAlias]

span.SetAttributes(/*...*/)
span.End()

This is how you set attributes at span end.

[petenewcomb]

Agreed, and as I stated above I'm fine with that. Mostly I'm trying to make sure the community of developers I support are able to understand and use this API correctly and effectively, hence the desire to make the documentation unambiguous and consistent with the code. If you feel the specification should also be more explicit in this area, I'm happy to help with that too.

Meanwhile, making the documentation for the existing API and its behavior more complete and correct seems like an orthogonal issue to improving the specification. If the specification ends up changing such that it requires modifications to the API, that should be taken on separately.

BTW, the documentation for the implementation of Span.End does mention the panic behavior:

// The only SpanOption currently supported is WithTimestamp which will set the
// end time for a Span's life-cycle.
//
// If this method is called while panicking an error event is added to the
// Span before ending it and the panic is continued.

This of course doesn't get propagated to the published documentation.
It also doesn't mention WithStackTrace, does not call out the requirement to use defer, and should reference SpanEndOption rather than SpanOption.