Add a new AddLink() operation to Span.

[carlosalberto]

Fixes #454

Related to #3337

(Updated on Sep 18th based on feedback) As the Messaging SIG merged its last OTEP 222, we will be adding operations that require Links after Span creation, taking a direct route with AddLink(), albeit without any of the new members suggested in #3337, e.g. timestamp (to be discussed in a separate issue).

AddLink(spanContext, attributes /* optional */)

cc @open-telemetry/proto-go-maintainers @open-telemetry/cpp-maintainers

[yurishkuro]

few nits, otherwise lgtm

[carlosalberto]

Hey @open-telemetry/specs-approvers @open-telemetry/go-approvers

I've updated this PR to mention SIGs can alternatively implement this operation using AddEvent(), as discussed in the past Spec call. Let me know whether that works for you.

On a related note, specifically for the AddEvent() route, what will be name mapped to when adding links? Maybe @jmacd had something in mind already? Depending on the output of this, I will amend the PR to include the Link's proto addition of event_name (besides timestamp).

[pyohannes]

On a related note, specifically for the AddEvent() route, what will be name mapped to when adding links? Maybe @jmacd had something in mind already? Depending on the output of this, I will amend the PR to include the Link's proto addition of event_name (besides timestamp).

Given that the AddEvent implementation route will likely be exceptional (maybe only Go), I'd refrain from doing any related proto changes.

Event specific information and attributes (name and timestamp) could be put as attributes on the resulting link, however, I think we gain some flexibility (and don't loose any critical functionality) by not leaving that unspecified and don't put any related requirements into the spec.

[carlosalberto]

Event specific information and attributes (name and timestamp) could be put as attributes on the resulting link

That could be interesting indeed. Later we could 'promote' them to be actual proto fields if/as needed.

[tigrannajaryan]

I've updated this PR to mention SIGs can alternatively implement this operation using AddEvent(), as discussed in the past Spec call. Let me know whether that works for you.

Does this discussion about link addition via AddAvent() need to happen in this PR? It seems this PR is about the ability to add links after span creation using the AddLink() operation. I am not sure why we need to have these 2 changes in one PR.

[carlosalberto]

Also from the Spec call, regarding event name (at least as specified through the AddEvent() path) - do we either:

a) Drop event_name if provided?
b) Put it in the attributes(by also defining some semconv names, e.g. link_referer or link_referee) as optional field? What about SIGs going the AddLink() route for this case?

Personally I'd go with a) for simplicity and unblocking reasons - we could add additional semantics later on, etc.

[jmacd]

Discussion in the Spec SIG today, asking why we may need to know the timestamp or link-name to distinguish links created at start time vs after start. Here is a discussion about how samplers can be extended to record spans based on links that happen after-the-fact. #2918

[Oberon00]

I think late links should be marked also in OTLP. IMHO it can be considered a semantic difference.

[carlosalberto]

As per the feedback made on the last Spec call (last week), we will be adding this operation without the timestamp or event-name (for the AddEvent() route) parameters for now. We will iterate on those ones in a separate issue.

[carlosalberto]

@MrAlias A last look perhaps?

[carlosalberto]

Ping @reyang @MrAlias

[carlosalberto]

Merging this one as we have released 1.26, and thus have time to discuss details or adjustments till we release 1.27.0 (in November)

[schickling]

Exciting! Is there a tracking issue for the JS implementation already? 👀