Move the Event type from the API to the SDK

[MrAlias]

Currently the API exports an Event type:

opentelemetry-go/trace/trace.go

Lines 542 to 556 in ca130e5

	// Event is a thing that happened during a Span's lifetime.
	type Event struct {
	// Name is the name of this event
	Name string
	// Attributes describe the aspects of the event.
	Attributes []attribute.KeyValue
	// DroppedAttributeCount is the number of attributes that were not
	// recorded due to configured limits being reached.
	DroppedAttributeCount int
	// Time at which this event was recorded.
	Time time.Time
	}

This type is not used in the API, it is used in the trace SDK and those values of this type that it creates are passed down the processing pipeline. It does not make sense to have this type exported from the API. It should be included in the package that actually uses it, the trace SDK.

What about the API asymmetry between the Link and Event?

The following is mostly added to capture the though process that went into the decision to not move the Link type as well and is included for transparency and future understanding of the decision.

It seems wrong to move this type without also moving the Link type also declared in the API. This cannot be done though because the Link type is directly used in the API by being passed to the WithLinks SpanOption.

opentelemetry-go/trace/config.go

Lines 160 to 164 in ca130e5

	// WithLinks adds links to a Span. The links are added to the existing Span
	// links, i.e. this does not overwrite.
	func WithLinks(links ...Link) SpanOption {
	return linksSpanOption(links)
	}

This exposes an underlying asymmetry issue between the argument structure of functions in the API. The methods that create a span event accept the component parts of the event as arguments, but the function to create a span link accepts the composite struct type itself. This is the only part of the API where we use an underlying struct type to pass configuration state as an argument instead of functional options.

Possible Solutions

1) Move Event and Link type to SDK and update WithLinks SpanOption

1. Move the Event type to the otel/sdk/trace package.
2. Move the Link type to the otel/sdk/trace package.
3. Remove the WithLinks SpanOption.
4. Add a new WithLink SpanOption to the otel/trace package. E.g.

   func WithLinks(sc SpanContext, attr ...attribute.KeyValue) SpanOption
   

Pros:

• Resolves the API asymmetry
• Unifies the API function argument pattern
  • This might be an anti-pattern given it is uniformity driving design without considering what is appropriate for the situation.

Cons:

• The span link API cannot be extended. If we want to add additional annotations to a link besides attributes it would require a breaking change to the function signature.

2) Move the Event type to SDK, but leave the Link type

1. Move the Event type to the otel/sdk/trace package.
2. Make no other API changes and leave the asymmetry in place.

Pros:

• Allows links to be extended in the future in a backwards compatible way.

Cons:

• Preserves the API asymmetry

3) Update the AddEvent method of the "otel/trace".Span interface to accept the Event type instead of EventOptions.

1. Update the "otel/trace".Span interface AddEvent method to accept the Event type instead of EventOptions
2. Make no other API changes.

Pros:

• Resolves the API asymmetry between the Event and Link functions

Cons:

• Splits the API further into accepting functional options in some places and config structs in others
• It is not clear how the RecordError method, which creates an event, should be updated (or not) to support this

Proposed Solution

Move the Event type to SDK, but leave the Link type.

[paivagustavo]

What do you think about creating a LinkOption and changing the WithLinks(...Link) to WithLink(SpanContext, ...LinkOption)?

That seems to resolve both problems.

[MrAlias]

What do you think about creating a LinkOption and changing the WithLinks(...Link) to WithLink(SpanContext, ...LinkOption)?

That seems to resolve both problems.

Ah, yeah, thanks for pointing that out. I forgot to include it.

I had initially discarded this solution because it would mean we would have an option for an option. This seemed excessive to me, but I'm open to hear if that is a shared sentiment.

[paivagustavo]

it would mean we would have an option for an option.

Could you explain more about that?

Do you mean that we would end up writing WithLink(WithAttributes(key, value), WithSpanContext(...))?

[MrAlias]

Could you explain more about that?

So WithLink would still be an option passed to a Span's start or end method. Meaning that if it itself accepted options you will have calls like:

tracer.Start("name", WithLink(spancontext, WithAttributes(/* ... */)), WithAttributes(/* ... */))

[Aneurysm9]

Could you explain more about that?

So WithLink would still be an option passed to a Span's start or end method. Meaning that if it itself accepted options you will have calls like:

tracer.Start("name", WithLink(spancontext, WithAttributes(/* ... */)), WithAttributes(/* ... */))

Would we even need a LinkOption type? The only things that can be included in a link are a single SpanContext and zero or more Attributes, so this signature would seem fine:

func WithLink(sc SpanContext, attrs attribute.KeyValue...) linkSpanOption {...}

The invocation at Start/End would still look something like this, but avoid the extra WithAttributes():

tracer.Start("name", WithLink(spancontext,  attrs...)), WithAttributes(/* ... */))

[MrAlias]

Would we even need a LinkOption type? The only things that can be included in a link are a single SpanContext and zero or more Attributes, so this signature would seem fine:

func WithLink(sc SpanContext, attrs attribute.KeyValue...) linkSpanOption {...}

The invocation at Start/End would still look something like this, but avoid the extra WithAttributes():

tracer.Start("name", WithLink(spancontext,  attrs...)), WithAttributes(/* ... */))

One of the things I was thinking with this approach is if the specification wanted to add something like a "type" to track the specific types of links it would be a breaking change to try and include something like that.

[Aneurysm9]

We could work around that with a new WithTypedLink() option or something like that, but I take the point. I would expect most anything else that might be added to a Link to be covered through the use of Attributes, but if we want to maintain the flexibility to extend the current API rather than adding new surface we'd need to use options and not just attrs as the varargs.

[MrAlias]

We could work around that with a new WithTypedLink() option or something like that, but I take the point. I would expect most anything else that might be added to a Link to be covered through the use of Attributes, but if we want to maintain the flexibility to extend the current API rather than adding new surface we'd need to use options and not just attrs as the varargs.

That seems like a valid approach if we need to extend in the future, I'm not opposed to this approach either.

[MrAlias]

Decision from the Go SIG meeting is to go with replacing WithLinks with:

func WithLink(sc SpanContext, attrs ...attribute.KeyValue) SpanOption {...}

[MrAlias]

One thing I missed is the Link type is used in the SpanConfig:

opentelemetry-go/trace/config.go

Line 56 in 08f4c27

Links []Link

I'm second guessing if we can move this type completely out of the API without just replacing it with an anonymous struct.

[Aneurysm9]

I'm second guessing if we can move this type completely out of the API without just replacing it with an anonymous struct.

Hmm, can we even do that? Wouldn't an SDK need to be able to use the Links field from the SpanConfig? Maybe simply moving the SpanContext from embedded to a named field in the Link struct is the way to go.

[MrAlias]

I'm second guessing if we can move this type completely out of the API without just replacing it with an anonymous struct.

Hmm, can we even do that? Wouldn't an SDK need to be able to use the Links field from the SpanConfig? Maybe simply moving the SpanContext from embedded to a named field in the Link struct is the way to go.

Agreed.