Skip to content
This repository has been archived by the owner on Nov 10, 2022. It is now read-only.

Consider using SpanContext for Link #36

Closed
carlosalberto opened this issue Apr 20, 2021 · 4 comments
Closed

Consider using SpanContext for Link #36

carlosalberto opened this issue Apr 20, 2021 · 4 comments

Comments

@carlosalberto
Copy link

As an optimization only Trace/Span Ids are being used for Link, instead of SpanContext. However, this means users have to deal with another type (that also is not in the specification).

Not a strong feeling though.

@dyladan
Copy link
Member

dyladan commented Apr 20, 2021

This is done to prevent the compiler from complaining that { spanId, traceId } is not a valid SpanContext because it is missing TraceFlags. Trace flags are not needed for links so requiring the user to pass a dummy value to quiet the compiler seems more annoying than having another type. most of the time this type will be inferred by the type system and never used directly by the user.

@dyladan dyladan closed this as completed Apr 21, 2021
@blumamir
Copy link
Member

blumamir commented May 2, 2021

Current interface for LinkContext is picking only traceId and spanId from SpanContext:

export type LinkContext = Pick<SpanContext, 'traceId' | 'spanId'>;

However, protobuf interface expect to also fill trace_state for each link:

  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.
    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;
  }

The current implementation in exporter collector just ignore this property and set nothing to it in proto message.

It feels like there is an alignment issue here between js-api, opentelemetry-proto and opentelemetry API.
IMO, we should either add trace_state to LinkContext if it's part of the API, or it should be removed from proto file if it's not part of the API.

@dyladan
Copy link
Member

dyladan commented May 3, 2021

It is entirely possible to have properties in the proto and sdk which are not exported from the API. Span has many such properties. In this case though, we should have it exposed.

From the spec:

A Link is structurally defined by the following properties:

  • SpanContext of the Span to link to.
  • Zero or more Attributes further describing the link.

Seems pretty clear to me that the link should expose the whole SpanContext including the trace state.

@Flarna
Copy link
Member

Flarna commented May 3, 2021

Refs: open-telemetry/opentelemetry-js#818

My intention that time was to remove traceFlags (and isRemote) from LinkContext. I agree we should add traceState.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants