Skip to content

Add a new AddLink() operation to Span.#3678

Merged
carlosalberto merged 28 commits intoopen-telemetry:mainfrom
carlosalberto:add-link
Oct 10, 2023
Merged

Add a new AddLink() operation to Span.#3678
carlosalberto merged 28 commits intoopen-telemetry:mainfrom
carlosalberto:add-link

Conversation

@carlosalberto
Copy link
Copy Markdown
Contributor

@carlosalberto carlosalberto commented Aug 29, 2023

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

@carlosalberto carlosalberto requested review from a team August 29, 2023 14:32
Comment thread specification/trace/api.md Outdated
Comment thread specification/trace/api.md
Copy link
Copy Markdown
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few nits, otherwise lgtm

Comment thread specification/trace/api.md Outdated
Comment thread specification/trace/api.md Outdated
Comment thread specification/trace/api.md
Comment thread specification/trace/api.md Outdated
@carlosalberto
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor

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.

Comment thread specification/trace/api.md
@carlosalberto
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor Author

carlosalberto commented Sep 5, 2023

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
Copy link
Copy Markdown
Contributor

jmacd commented Sep 12, 2023

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
Copy link
Copy Markdown
Member

Oberon00 commented Sep 12, 2023

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

@carlosalberto
Copy link
Copy Markdown
Contributor Author

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 carlosalberto requested a review from a team October 6, 2023 14:49
@carlosalberto
Copy link
Copy Markdown
Contributor Author

@MrAlias A last look perhaps?

@carlosalberto
Copy link
Copy Markdown
Contributor Author

Ping @reyang @MrAlias

@carlosalberto
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown

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

carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Fixes open-telemetry#454 

Related to open-telemetry#3337 

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 open-telemetry#3337, e.g. `timestamp` (to be
discussed in a separate issue).

```
AddLink(spanContext, attributes /* optional */)
```
schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025
Fixes open-telemetry#454

Related to open-telemetry#3337

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 open-telemetry#3337, e.g. `timestamp` (to be
discussed in a separate issue).

```
AddLink(spanContext, attributes /* optional */)
```
schmikei pushed a commit to schmikei/opentelemetry-specification that referenced this pull request Apr 17, 2025
Fixes open-telemetry#454

Related to open-telemetry#3337

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 open-telemetry#3337, e.g. `timestamp` (to be
discussed in a separate issue).

```
AddLink(spanContext, attributes /* optional */)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Please (re)-allow recording links after Span creation time