Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Links with invalid SpanContext are recorded. #3928

Merged
merged 10 commits into from
May 7, 2024
Merged

Links with invalid SpanContext are recorded. #3928

merged 10 commits into from
May 7, 2024

Conversation

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

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
specification/trace/api.md Outdated Show resolved Hide resolved
trask
trask reviewed Mar 7, 2024
View reviewed changes
specification/trace/sdk.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor Author

@jmacd @trask Applied feedback. Please review.

cc @bogdandrutu

trask
trask approved these changes Mar 14, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@carlosalberto @trask
Update CHANGELOG.md
1b85580 
Co-authored-by: Trask Stalnaker <[email protected]>
@carlosalberto carlosalberto changed the title Links with invalid SpanContext are recorded by default. Links with invalid SpanContext are recorded. Mar 14, 2024
@github-actions GitHub Actions
Copy link

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
trask
trask reviewed Mar 29, 2024
View reviewed changes
specification/trace/api.md Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

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
specification/trace/api.md Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

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
6 of 7 checks passed
@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 <[email protected]>
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

@jmacd jmacd jmacd left review comments

@trask trask trask approved these changes

@pyohannes pyohannes pyohannes approved these changes

@Oberon00 Oberon00 Oberon00 approved these changes

@jsuereth jsuereth jsuereth approved these changes

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