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.

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

Are invalid SpanContexts allowed as Links? Needs spec clarification. #2176

Closed
Tracked by #659
Oberon00 opened this issue Nov 30, 2021 · 10 comments · Fixed by #3928
Closed
Tracked by #659

Are invalid SpanContexts allowed as Links? Needs spec clarification. #2176

Oberon00 opened this issue Nov 30, 2021 · 10 comments · Fixed by #3928
Assignees
Labels
area:api Cross language API specification issue area:semantic-conventions Related to semantic conventions area:span-relationships Related to span relationships semconv:messaging spec:trace Related to the specification/trace directory

Comments

@Oberon00
Copy link
Member

Oberon00 commented Nov 30, 2021

I was trying to create a link to an invalid SpanContext in opentelemetry-java, but there was no link created. It seems that the Java implementation ignores attempts to add links with invalid SpanContexts. I think the spec is unclear here if this should be allowed. I think it should be allowed to avoid creating edge cases.
E.g. one might want to put certain attributes describing an incoming call on a link (e.g. a batch receive for messaging). For some incoming messages, there might be no information about the parent context, but other link attributes might still be available. In that case, it could be useful to add a link to the invalid SpanContext with additional attributes.

Tagging this with messaging because that was the use case I came up with, feel free to remove if you feel this does not apply.

EDIT: A bit related is the general definition of an "invalid" SpanContext which is rather broad and can lead to information loss: #753

@Oberon00 Oberon00 added area:api Cross language API specification issue area:semantic-conventions Related to semantic conventions area:span-relationships Related to span relationships spec:trace Related to the specification/trace directory semconv:messaging labels Nov 30, 2021
@yurishkuro
Copy link
Member

I'd say it should not be allowed, because it creates a dangling reference that cannot be followed, causing more special case handling elsewhere. Since this is already a special case and is not actually pointing to another trace/span, the information can be captured using other constructs, such as span event.

@pyohannes
Copy link
Contributor

It seems to me the spec is deliberately unclear in that regard:

Implementations MAY ignore links with an invalid SpanContext.

The use case is a valid one, but I'm a bit skeptical about using dangling links as a tool to model it. As mentioned above, it will also require adding special logic to indexers and visualization tools.

@carlosalberto
Copy link
Contributor

I'd also be up for completely ignoring them. Btw, the wording came from #1492 regarding API/SDK separation. Any opinion on this @bogdandrutu ?

@Oberon00
Copy link
Member Author

Oberon00 commented Dec 1, 2021

The link would not be dangling (pointing to a non-existent span) but a null-link (pointing to the all-zero trace+span ID).

@Oberon00
Copy link
Member Author

Oberon00 commented Dec 1, 2021

I did not see that null-links were already mentioned, but I think this is a case where leaving it undefined is harmful.

@carlosalberto
Copy link
Contributor

Ping @bogdandrutu (who may know more about the related context)

@Oberon00
Copy link
Member Author

Oberon00 commented Mar 21, 2023

Ping @lmolkova, just stumbled upon this, maybe the messaging SIG is interested in reviving this issue (sorry if it's already on your radar anyways -- nothing urgent for me)

@lmolkova
Copy link
Contributor

lmolkova commented Mar 21, 2023

@Oberon00 definitely! thanks for bringing it up.

The use-case for span links with invalid context is attributes.

e.g. I publish or receive a batch of two messages and want to record some essential things about each message:

  • number of times it was delivered
  • message id
  • time it was published
  • etc

Even if the message does not have trace-context, recording these attributes is still important. If links can't be used if there is no context, the alternative is an event/log.
We can't do it conditionally depending if there is a context to keep instrumentation behavior stable and predictable,. If we don't allow link with invalid context, we'd have to add a link for each message with context and an event for each message.

This brings us back to links vs events discussion, but regardless of underlying concept, capturing links without context should be allowed.

@carlosalberto
Copy link
Contributor

Notes from today's Spec call:

  • There was general, initial agreement on allowing this, given the value of having attributes, even without a valid SpanContext.
  • As a complementary feature, we want to allow opting-out and instead drop Links with invalid SpanContext, in case the backends don't support invalid SpanContext instances well . This could be explored as either a SDK configuration option or a SpanProcessor, for example.

@lmolkova to follow up with a proposal.

@Oberon00
Copy link
Member Author

Oberon00 commented May 9, 2023

Since I just found that one again: A bit related is the general definition of an "invalid" SpanContext which is rather broad and can lead to information loss: #753

carlosalberto added a commit that referenced this issue May 7, 2024
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).

---------

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this issue Oct 31, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue area:semantic-conventions Related to semantic conventions area:span-relationships Related to span relationships semconv:messaging spec:trace Related to the specification/trace directory
Development

Successfully merging a pull request may close this issue.

5 participants