Skip to content

Links with invalid SpanContext are recorded.#3928

Merged
carlosalberto merged 10 commits intoopen-telemetry:mainfrom
carlosalberto:keep-invalid-links
May 7, 2024
Merged

Links with invalid SpanContext are recorded.#3928
carlosalberto merged 10 commits intoopen-telemetry:mainfrom
carlosalberto:keep-invalid-links

Conversation

@carlosalberto
Copy link
Copy Markdown
Contributor

@carlosalberto carlosalberto commented Mar 7, 2024 •
edited
Loading

As Links with invalid SpanContext may contain attributes describing the operation (specially useful and occurrent for messaging systems), we still want to collect them. Fixes #2176 (in the issue extensive discussion about took place and we came, back in the day, with an initial agreement).

@carlosalberto
Links with invalid SpanContext are recorded by default.
9df5bf2 
As Links with invalid SpanContext may contain attributes
describing the operation (specially useful and occurrent
for messaging systems), we still want to collect them,
also allowing the user to drop them via a SDK configuration
option.
@carlosalberto carlosalberto requested review from a team March 7, 2024 14:09
@pyohannes
Copy link
Copy Markdown
Contributor

pyohannes commented Mar 7, 2024

Thanks @carlosalberto for getting this going.

We currently have a "Implementations MAY ignore links with an invalid SpanContext", and I'm changing it to "Implementations SHOULD record links with an invalid SpanContext", which doesn't sound like a breaking change, but I'd like to hear (practical) opinions.

I think this (practically) depends on what implementations are currently doing. If all implementations are currently recording links to invalid span contexts, then this change wouldn't be a breaking change. If there is an implementation that drops links to invalid span contexts without good reason, then this change might be considered breaking for that implementation.

I'd suggest:

  1. Identify if there are any implementations dropping links to invalid span contexts.
  2. Change those implementations to record links to invalid span contexts.
  3. Merge this specification change.

trask
trask reviewed Mar 7, 2024
View reviewed changes
trask
trask reviewed Mar 7, 2024
View reviewed changes
@carlosalberto
Copy link
Copy Markdown
Contributor Author

carlosalberto commented Mar 14, 2024

@jmacd @trask Applied feedback. Please review.

cc @bogdandrutu

trask
trask approved these changes Mar 14, 2024
View reviewed changes
@carlosalberto @trask
Update CHANGELOG.md
1b85580 
Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@carlosalberto carlosalberto changed the title Links with invalid SpanContext are recorded by default. Links with invalid SpanContext are recorded. Mar 14, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 22, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 22, 2024
Oberon00
Oberon00 reviewed Mar 22, 2024
View reviewed changes
trask
trask reviewed Mar 29, 2024
View reviewed changes
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 10, 2024
Oberon00
Oberon00 approved these changes Apr 18, 2024
View reviewed changes
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 26, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 26, 2024
@trask trask removed the Stale label Apr 30, 2024
@carlosalberto carlosalberto merged commit ca0ce36 into open-telemetry:main May 7, 2024
@carlosalberto carlosalberto deleted the keep-invalid-links branch May 7, 2024 13:32
@carlosalberto carlosalberto mentioned this pull request May 7, 2024
Merged
@MrAlias MrAlias mentioned this pull request May 7, 2024
Closed
carlosalberto added a commit that referenced this pull request May 9, 2024
@carlosalberto
Release 1.33.0 (#4034)
a7bb647 
May 2024 release.

Important changes:

* Mark exemplars as stable (#3870)
* Mark synchronous gauge as stable (#4019)
* Record Links with empty/invalid SpanContext (#3928)
@lzchen lzchen mentioned this pull request May 9, 2024
Closed
carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@carlosalberto @trask
Links with invalid SpanContext are recorded. (open-telemetry#3928)
2a990ac 
As Links with invalid SpanContext may contain attributes describing the
operation (specially useful and occurrent for messaging systems), we
still want to collect them. Fixes open-telemetry#2176 (in the issue extensive
discussion about took place and we came, back in the day, with an
initial agreement).

---------

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
@carlosalberto
Release 1.33.0 (open-telemetry#4034)
3fe53ed 
May 2024 release.

Important changes:

* Mark exemplars as stable (open-telemetry#3870)
* Mark synchronous gauge as stable (open-telemetry#4019)
* Record Links with empty/invalid SpanContext (open-telemetry#3928)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@trask trask trask approved these changes

@jmacd jmacd jmacd left review comments

@jsuereth jsuereth jsuereth approved these changes

@Oberon00 Oberon00 Oberon00 approved these changes

+1 more reviewer

@pyohannes pyohannes pyohannes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmacd jmacd

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Are invalid SpanContexts allowed as Links? Needs spec clarification.

6 participants

@carlosalberto @pyohannes @jsuereth @trask @Oberon00 @jmacd