Add RecordLink for Span. #3240

carlosalberto · 2023-02-21T16:44:41Z

Alternative to #3186

A new optional RecordLink(SpanContext, Attributes) operation is added, very similar to RecordException, which means we use AddEvent behind the scenes, storing the actual data as Attributes:

Event name is link.
trace and span ids are stored as hex strings.
Trace State remains a string.
The Attributes associated to a new Link are passed through to the newly created Event.

This effectively means these are soft Links. Other details:

As this is an optional operation, not all languages would have to add this (C++, Go, etc). However, this will require instrumentation authors to manually set the Attributes passed to AddEvent as shown above.
(At least) OTLP consumers will have to process standard Links plus check any event with the link name and containing the traceid, spanid and tracestate triplet.

arminru · 2023-02-21T17:01:32Z

specification/trace/semantic_conventions/links.md

@@ -0,0 +1,45 @@
+# Semantic Conventions for Links


Please add this new file to the TOC in README.md

arminru · 2023-02-21T17:03:47Z

specification/trace/api.md

@@ -691,6 +692,26 @@ Note: `RecordException` may be seen as a variant of `AddEvent` with
 additional exception-specific parameters and all other parameters being optional
 (because they have defaults from the exception semantic convention).

+#### Record Link


Probably too early to spark a naming discussion but I think span.RecordLink(ctx, attrs) would be very misleading and should rather be called RecordLinkAs(Span)Event or the like, since the outcome will not be a Span Link as one might expect from the name.

I think if we go down the path of recording links as events after span starts, we'd eventually replace original links with events too. If that happens, we'd regret naming it as RecordLinkAs(Span)Event.

yurishkuro

I am strongly against this approach. A Link is a dedicated entity in the data model that is designed to capture causality between spans. Reusing another entity from the data model (Event) for the same purpose is a very bad design.

tsloughter · 2023-02-21T18:13:59Z

I agree with @yurishkuro.

There seem to be 2 use cases, one that is due to limitations of particular instrumentations (so don't conceptionally need the link added after start) and those like #454 that describe non-causal relationships.

In the latter example I'd argue those shouldn't be Links in the first place. In which case maybe a definition for adding span contexts as Events to a Span makes sense, just not "Links".

So Links can stay as they are, and Events are used for non-causal relationships -- "at time T this span did something based on a message created during Span X".

yurishkuro · 2023-02-21T19:06:39Z

"at time T this span did something based on a message created during Span X".

why isn't this causal / happens-before?

tsloughter · 2023-02-21T20:05:24Z

@yurishkuro just basing it on the example given in #454 and generally considering the idea that a span could cause an Event instead of a Span. I can't think of an example that describes the real world data/events that would need to be modeled like this.

lmolkova · 2023-02-21T21:54:34Z

I have no strong opinion on the link/event terminology. FWIW limiting links to causal-only relationships leaves us without a way to represent non-causal relationships and perhaps we should be looking into broadening links definition.

Anyway, representing the thing that describes that two spans are related as event seems optimal:

events (at least in scope of Event API) can happen outside of span lifetime, i.e. if I discover that two spans are related after they're both over, I don't need to create an artificial span just to record relationships between them. (this was a real case in some Azure service)
they have timestamp, i.e. if I receive multiple messages in scope of one operation, I can record when each of them was received
they don't affect sampling decisions
they are flexible and don't imply specific relationships between spans.

lmolkova · 2023-02-21T22:02:19Z

semantic_conventions/trace/link.yaml

+          [retrieved](../api.md#retrieving-the-traceid-and-spanid)
+          by `SpanContext`.
+        examples: 'af9d5aa4-a685-4c5f-a22b-444f80b3cc28'
+      - id: tracestate


trace-flags? otherwise we don't know if it was recorded on the other service

Sounds useful, although I think you could make the same argument for our normal parent-child relationships (maybe we should actually add a parent_flags to the OTLP span?)

MrAlias · 2023-02-23T20:01:58Z

I am strongly against this approach. A Link is a dedicated entity in the data model that is designed to capture causality between spans. Reusing another entity from the data model (Event) for the same purpose is a very bad design.

Can you link to the data model definition?

pyohannes · 2023-02-23T22:54:55Z

Can you link to the data model definition?

I guess one could see OTLP as a data model definition? There links are a different entity than events.

One of the main drawbacks of this approach is, that it forces telemetry consumers (who want to properly support links) to look at two different places for them: as links and as events. Proper support for links created after span creation would then require modifications in a variety of backends and exporters.

yurishkuro · 2023-02-23T23:12:48Z

Can you link to the data model definition?

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#specifying-links
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/overview.md#links-between-spans

These two define the semantics and the data model of the links. The definition is not as strong as Lamport's happens-before relationship, it only says "causally related", but the point is that we have a distinct model entity defined for that, so as @pyohannes said above, using a different entity like Event, which is defined semantically to mean a very different thing, is a poor design choice, and a poor user experience (both production and consumption).

lmolkova · 2023-02-24T00:06:16Z

One of the main drawbacks of this approach is, that it forces telemetry consumers (who want to properly support links) to look at two different places for them: as links and as events. Proper support for links created after span creation would then require modifications in a variety of backends and exporters.

we have a precedent unifying events and logs. If we do links-are-represented-as-events in OTLP, the original links would probably go away.

using a different entity like Event, which is defined semantically to mean a very different thing, is a poor design choice, and a poor user experience (both production and consumption).

some people say that everything (including span) is semantically an event. In any case, user experience and over-the-wire data representation are not strictly related and I'd love to see some explanation on what's bad about user experience.

yurishkuro · 2023-02-24T02:35:27Z

I'd love to see some explanation on what's bad about user experience.

Cognitive overhead, ambiguity. Production: do I record this as link or event? Consumption: do I read this from links or events? Production: I am capturing a relationship between two spans, what is the timestamp doing here?

some people say that everything (including span) is semantically an event

we can also say that everything is a sequence of bytes, why bother with semantics of the logical data model at all?

lmolkova · 2023-02-24T18:50:37Z

Production: do I record this as link or event?

You record it as link since that's what on the API and in the documentation without bothering much about over-the-wire format.

Production: I am capturing a relationship between two spans, what is the timestamp doing here?

Counter: When I'm receiving multiple messages with a timeout and some of them are prefetched and other come at different points in time, how do I record when the message came without creating two things - link AND event?

Timestamp is there to say when the relationship was discovered and should also help answer questions like: why this link was not considered when I made sampling decision.

Consumption: do I read this from links or events?

Users don't read it from over-the-wire format but from their backends. Cognitive load of a backend developer getting list of links from the span and transforming them is no different than extracting exceptions from events.

Counter: as a user, when I want to see if two spans are related and my backend stores links inside spans, I need to write a query like span.links.filter(l.trace_id == another_trace_id), how is this a good experience?

Having events that exist outside of span lifetime would at least require them to be stored separately improving the experience instead.

lmolkova · 2023-02-24T18:54:48Z

we can also say that everything is a sequence of bytes, why bother with semantics of the logical data model at all?

what I'm saying is that event is a base type for different things and I argue that links is one of these things

yurishkuro · 2023-02-24T19:46:00Z

@lmolkova you argue that relationships can be recorded via events. I don't dispute that. A link is just a structured representation of the relationship that can be just as well encoded via semantic conventions. What I don't agree with is having two representations of links in the data model, as this PR is proposing. I can get behind the proposal to deprecate the concept of Links in favor of span events with specific semantic convention for capturing span/trace reference.

github-actions · 2023-03-04T03:17:13Z

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

MrAlias · 2023-03-07T16:23:38Z

I'm also having a hard time understanding why a link cannot be added to an event. It sounds like a preference to not add a field to the event for links or update the definition of an event to contain similar information. Is there any compatibility issues with this?

Given this is being looked at as an alternative to recording a link in other ways that all have explicit limitations (sampling, API compatibility, logical inclusion of trace IDs) this solution seems to be the only one without explicit limitation, instead there is a subjective limitation.

Oberon00 · 2023-03-08T15:21:01Z

Long-term, didn't we want to deprecate the Span events APIs in favor of the global events /logs?

github-actions · 2023-03-16T03:17:10Z

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

lmolkova · 2023-03-21T17:47:04Z

One more reason why link is a log record + remote trace context is #2176 (comment)

Essentially, link as it's defined today

  message Link {
    // A unique identifier of a trace that this linked span is part of. The ID is a
    // 16-byte array.
    bytes trace_id = 1;

    // A unique identifier for the linked span. The ID is an 8-byte array.
    bytes span_id = 2;

    // The trace_state associated with the link.
    string trace_state = 3;

    // attributes is a collection of attribute key/value pairs on the link.
    // Attribute keys MUST be unique (it is not allowed to have more than one
    // attribute with the same key).
    repeated opentelemetry.proto.common.v1.KeyValue attributes = 4;

    // dropped_attributes_count is the number of dropped attributes. If the value is 0,
    // then no attributes were dropped.
    uint32 dropped_attributes_count = 5;
  }

mixes two concepts together: remote context and attributes.

I think limiting link to

  message Link {
    // A unique identifier of a trace that this linked span is part of. The ID is a
    // 16-byte array.
    bytes trace_id = 1;

    // A unique identifier for the linked span. The ID is an 8-byte array.
    bytes span_id = 2;

    // flags?

    // The trace_state associated with the link.
    string trace_state = 3;
  }

and adding link property (or list of them?) to LogRecord would preserve two different concepts.

github-actions · 2023-03-29T03:16:56Z

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

github-actions · 2023-04-06T03:16:40Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Add a RecordLink for Span.

b940ec9

carlosalberto requested review from a team February 21, 2023 16:44

carlosalberto changed the title ~~Add a RecordLink for Span.~~ Add RecordLink for Span. Feb 21, 2023

github-actions bot assigned jmacd Feb 21, 2023

arminru added area:api Cross language API specification issue area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Feb 21, 2023

arminru reviewed Feb 21, 2023

View reviewed changes

arminru requested review from a team February 21, 2023 17:04

pyohannes mentioned this pull request Feb 21, 2023

Allow adding links after span creation #3186

Closed

yurishkuro requested changes Feb 21, 2023

View reviewed changes

lmolkova reviewed Feb 21, 2023

View reviewed changes

github-actions bot added the Stale label Mar 4, 2023

MrAlias removed the Stale label Mar 7, 2023

pyohannes linked an issue Mar 10, 2023 that may be closed by this pull request

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

Closed

github-actions bot added the Stale label Mar 16, 2023

lmolkova mentioned this pull request Mar 21, 2023

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

Closed

github-actions bot removed the Stale label Mar 22, 2023

jmacd mentioned this pull request Mar 23, 2023

Do not add AddLink(), use option to AddEvent() instead #3337

Closed

github-actions bot added the Stale label Mar 29, 2023

github-actions bot closed this Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RecordLink for Span. #3240

Add RecordLink for Span. #3240

carlosalberto commented Feb 21, 2023 •

edited

Loading

arminru Feb 21, 2023

arminru Feb 21, 2023

lmolkova Feb 21, 2023

yurishkuro left a comment

tsloughter commented Feb 21, 2023

yurishkuro commented Feb 21, 2023

tsloughter commented Feb 21, 2023

lmolkova commented Feb 21, 2023 •

edited

Loading

lmolkova Feb 21, 2023

Oberon00 Feb 22, 2023

MrAlias commented Feb 23, 2023

pyohannes commented Feb 23, 2023

yurishkuro commented Feb 23, 2023 •

edited

Loading

lmolkova commented Feb 24, 2023 •

edited

Loading

yurishkuro commented Feb 24, 2023 •

edited

Loading

lmolkova commented Feb 24, 2023 •

edited

Loading

lmolkova commented Feb 24, 2023 •

edited

Loading

yurishkuro commented Feb 24, 2023

github-actions bot commented Mar 4, 2023

MrAlias commented Mar 7, 2023 •

edited

Loading

Oberon00 commented Mar 8, 2023 •

edited

Loading

github-actions bot commented Mar 16, 2023

lmolkova commented Mar 21, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Apr 6, 2023

Add RecordLink for Span. #3240

Add RecordLink for Span. #3240

Conversation

carlosalberto commented Feb 21, 2023 • edited Loading

arminru Feb 21, 2023

Choose a reason for hiding this comment

arminru Feb 21, 2023

Choose a reason for hiding this comment

lmolkova Feb 21, 2023

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

tsloughter commented Feb 21, 2023

yurishkuro commented Feb 21, 2023

tsloughter commented Feb 21, 2023

lmolkova commented Feb 21, 2023 • edited Loading

lmolkova Feb 21, 2023

Choose a reason for hiding this comment

Oberon00 Feb 22, 2023

Choose a reason for hiding this comment

MrAlias commented Feb 23, 2023

pyohannes commented Feb 23, 2023

yurishkuro commented Feb 23, 2023 • edited Loading

lmolkova commented Feb 24, 2023 • edited Loading

yurishkuro commented Feb 24, 2023 • edited Loading

lmolkova commented Feb 24, 2023 • edited Loading

lmolkova commented Feb 24, 2023 • edited Loading

yurishkuro commented Feb 24, 2023

github-actions bot commented Mar 4, 2023

MrAlias commented Mar 7, 2023 • edited Loading

Oberon00 commented Mar 8, 2023 • edited Loading

github-actions bot commented Mar 16, 2023

lmolkova commented Mar 21, 2023

github-actions bot commented Mar 29, 2023

github-actions bot commented Apr 6, 2023

carlosalberto commented Feb 21, 2023 •

edited

Loading

lmolkova commented Feb 21, 2023 •

edited

Loading

yurishkuro commented Feb 23, 2023 •

edited

Loading

lmolkova commented Feb 24, 2023 •

edited

Loading

yurishkuro commented Feb 24, 2023 •

edited

Loading

lmolkova commented Feb 24, 2023 •

edited

Loading

lmolkova commented Feb 24, 2023 •

edited

Loading

MrAlias commented Mar 7, 2023 •

edited

Loading

Oberon00 commented Mar 8, 2023 •

edited

Loading