Export SpanContext.IsRemote in OTLP

[axw]

Proposal to update OTLP to indicate whether a span's parent is remote.

[axw]

@open-telemetry/specs-approvers can I please get some reviews on this?

[anuraaga]

This is an important piece of information and aside from some small comments this OTEP looks like a good idea to me.

I have a small preference for using a field instead of attribute since we currently don't have any data modeling attributes I think, they are all informational. Memory analysis below but tl;dr, even if there was a bigger impact it seems too small to change this above fact about our data modeling.

Note that there likely won't be any significant memory impact on SDKs since we already have SpanContext.isRemote - in Java, we just modify here

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/common/src/main/java/io/opentelemetry/exporter/otlp/internal/traces/SpanMarshaler.java#L43

to store parentContext instead of parentId and serialize based on it

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/common/src/main/java/io/opentelemetry/exporter/otlp/internal/traces/SpanMarshaler.java#L117

so there is zero impact on non-local-root spans. Where SDKs use generated protobuf objects, the relatively short-lived proto Span object will have one extra field only, but no long term storage should be needed.

For backends that receive the data, it is likely that there will be a byte or four (depending on alignment) of extra long term memory usage. Anyways it doesn't seem enough to factor into decision making.

[anuraaga]

@open-telemetry/specs-approvers Can you please review this otep?

[tigrannajaryan]

@axw sorry for late reply, this fall off my radar (and thanks for reminding). The proposal looks reasonable to me.

If we end up adding flags field to Span in OTLP as proposed here I think we can record the IsRemote as a bit in the flags (which makes it zero cost in memory and very small cost on the wire).

[tigrannajaryan]

@bogdandrutu I think you discussed this in OpenCensus too. PTAL.

[Oberon00]

If we end up adding flags field to Span in OTLP as proposed here I think we can record the IsRemote as a bit in the flags

This is a very nice idea that may lead to much trouble and head-scratching if whatever bit we choose gets a meaning assigned in W3C. Unless we make our "flags" larger than the W3C trace flags, but then it's not zero-cost (and in theory, the W3C trace flags field might have more bits an a future traceparent header version).

[tigrannajaryan]

If we end up adding flags field to Span in OTLP as proposed here I think we can record the IsRemote as a bit in the flags

This is a very nice idea that may lead to much trouble and head-scratching if whatever bit we choose gets a meaning assigned in W3C. Unless we make our "flags" larger than the W3C trace flags, but then it's not zero-cost (and in theory, the W3C trace flags field might have more bits an a future traceparent header version).

Our flags are already larger than W3C trace flags. We reserve 8 bits for W3C trace flags (of which only 1 bit is defined by W3C currently) and the remaining 24 bits in our flags are available for anything else we want to use it for. If W3C in the future decides to come up with more than 8 bits of flags they have to do it in a very explicit way, in which case we can also follow what they do.

[Oberon00]

I suggest we have the discussion regarding the naming

I strongly suggest to stay with "IsRemote" for storing what the spec already calls IsRemote in the API (where we can't rename it)

[carlosalberto]

@open-telemetry/specs-approvers We have enough approvals so I will merge in the next couple of days. Please raise your voice if you want to do a review before that.

[carlosalberto]

@axw We are ready to merge, but we need to get the lint check to pass. Can you check what's happening? https://app.circleci.com/pipelines/github/open-telemetry/oteps/1593/workflows/eadc0058-1700-47a1-b96a-60abff61c475/jobs/2588

[graphaelli]

satisfy markdown-lint

[graphaelli]

@carlosalberto would you accept the suggestions to address the markdown linting issues?

[graphaelli]

another round of markdown-lint-fix

[carlosalberto]

Merged. Please start preparing the PR(s) to include this change in the Spec + proto repositories.