oteps

oteps copied to clipboard

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

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

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

@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).

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

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).

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.

@bogdandrutu ICYMI Tigran's comment above (https://github.com/open-telemetry/oteps/pull/182#issuecomment-1132111698):

I think you discussed this in OpenCensus too. PTAL.

Could you please take a look?

Thanks for chiming in @willarmiros, and to everyone who has reviewed so far.

I'd also love to get this merged. IIANM, there's still 3 more reviews required. I suppose the grey check approvals are not code owners, and don't count towards the required "4 approving reviews".

@open-telemetry/specs-approvers please take a look. If more clarification is needed, please let me know. I'd be happy to have a meeting if it helps, bearing in mind I'm UTC+8.

@open-telemetry/specs-approvers Is anything missing in this proposal to get approvals / reviews?

There seems to be quite some interest for this and also https://github.com/open-telemetry/opentelemetry-proto/pull/384 seems to be waiting on this.

This was closed in error -- https://github.com/open-telemetry/opentelemetry-specification/pull/3257 is a prerequisite clarification, it doesn't resolve this PR overall.

Can someone with permissions please reopen? (I can't.)

It seems that the syntax "resolves https://github.com/open-telemetry/oteps/pull/182#discussion_r971857784" resolves the whole PR to which the linked comment belongs. Sorry for that.

• With https://github.com/open-telemetry/opentelemetry-specification/pull/3257 being merged, the parent SpanContext (including the parent's IsRemote information) is specified as something that must be exposed on the ReadableSpan. So, this comment should be resolved.
• Here, @Oberon00 explained very well, why SpanKind (with its "logical" remote parent-child relationship ) is not suitable for carrying information about the actual isRemoteParent information

@open-telemetry/specs-approvers as the main discussion points have been addressed, can we proceed with this proposal to expose the parent->isRemote information (as a protobuf field) on a serialized spans and merge this proposal?

Carrying on from the Zoom chat today in the Spec SIG meeting:

I found that my question was covered here https://github.com/open-telemetry/oteps/pull/182/files#r969043021 and specifically resolved by the patch to the spec here https://github.com/open-telemetry/opentelemetry-specification/pull/3257

MUST expose at least the full parent SpanContext.

So in the case of Erlang at least we simply need to update to comply with the latest spec and include a copy of the parent SpanContext in the Span and not just the SpanId.

The other discussion was around whether IsRemote really means "remote" since it could be propagated from within the same process -- example being propagating a context from C++ called through the JNI or an Erlang NIF.

But what "is remote" I don't think is seen by anyone as anything to block this OTEP, rather a discussion to be had separately that was prompted by this OTEP.

IsRemote today means IsExtracted, or "span ID not guaranteed to be generated by this TracerProvider instance"

I suggest we have the discussion regarding the naming ("is remote or not") as part of making this into the Specification (where we will have to update the proto files with the proper documentation docs anyway).

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)

@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.

@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

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

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