Indicate if a span's parent or link is remote using 2 bit flag as described in OTEP 0182

[estolfo]

This adds parent_span_is_remote field to Span message introduced in OTEP 0182: https://github.com/open-telemetry/oteps/blob/main/text/0182-otlp-remote-parent.md

Update: The PR now uses 2 bits from flags to represent the 3 states of parent or link is remote: unknown, not remote, is remote.

[tigrannajaryan]

I am blocking this until we make 1.0 release. Ideally we don't want any changes other than bug fixes between the RC that we just did and the final release.

[estolfo]

Hi @tigrannajaryan @Oberon00 @bogdandrutu can we reopen this discussion now that v1.0 is out?

v1.0 is out, no reason to block this PR further.

[carlosalberto]

(added it to the Spec call agenda next week)

[carolabadeer]

Hi @tigrannajaryan @Oberon00 @carlosalberto, any updates on this discussion?

[Oberon00]

Hi @carolabadeer, what do you mean by "this discussion"? If you mean the one at #484 (comment) then it would be better to reply there, as the top-level PR comments have no threading functionality and it gets confusing soon.

[carlosalberto]

Ping @tigrannajaryan

[tigrannajaryan]

Sorry for slow response, I was out for a couple weeks.

Re-reading the thread I don't see any new arguments, so I think we should go ahead with a tristate enum.

[jsuereth]

Recommendation:

Can you add documentation for Unset denoting that older clients may not set this field, in which case how would one figure out whether ParentSpanIsRemote?

Something like:

"In this case, ParentSpanIsRemote can be derived by looking at the parent span and determining if it has a different Resource. This value is expected from older clients."

[tigrannajaryan]

Another approach is to represent this as a 2-bitfield of a new int32 field, part of which will be also used by #503

[Oberon00]

@jsuereth

Can you add documentation for Unset denoting that older clients may not set this field, in which case how would one figure out whether ParentSpanIsRemote?

You can't find out. That's kind of the point of the field.

[Oberon00]

@tigrannajaryan I think we should not block this PR with this, but consider this refactoring before the next release, which should probably wait until after both this PR and the flags PR(s) have been merged.

[tigrannajaryan]

@tigrannajaryan I think we should not block this PR with this, but consider this refactoring before the next release, which should probably wait until after both this PR and the flags PR(s) have been merged.

I am not sure it is a good idea. This is a repository that is declared stable. We should minimize any churn here. An accidental release in an intermediate incorrect state can become a huge problem. It may be worth looking into using branches to prepare work for next releases instead of doing it on main branch.

[Oberon00]

An accidental release in an intermediate incorrect state can become a huge problem. It may be worth looking into using branches to prepare work for next releases instead of doing it on main branch.

The state would not be incorrect, unless you view that optimization/refactoring as vital.
Stable applies only to releases, not any intermediate main branch state, right?

[tigrannajaryan]

Stable applies only to releases, not any intermediate main branch state, right?

This has not been discussed.

Formally, it is likely that you are right, we only need stability guarantees to apply to releases.

However we don't currently have a good process that ensures main branch is in a state that satisfies stability guarantees before we make the release. We can work on adding such a process, but absent that I want to be careful and for now require that "main" branch state must also meet the stability guarantees. We can lift off that requirement when there are checks in place that prevent mistakes like releasing intermediary states that are not supposed to be released.

This is a high-stakes repo, we need to be extra careful here.

Again, a possible approach for this repo would be to do a branch-based development where all work for the next release (1.1 in this case) is done on a separate branch, not on "main", to make sure the release process is very explicit and there is no easy way to accidentally release from an intermediary "main" state that is not actually ready to be released. This would require merging PRs to the branch and let the changes bake for a while and then once we are happy with accumulated changes we would cut the next release by merging the branch to "main" and then making a release from "main".

There are likely other processes we can use, I am open for suggestions.

[tigrannajaryan]

Blocking to wait for #503 and use the introduced bit field.

[carlosalberto]

Now that #503 has been merged, we can discuss this PR again. IIRC @tigrannajaryan wanted this information to exist in the newly added flags field in the mentioned PR, rather than exist as an independent parent_is_remote field. Could you confirm/clarify this please?

[tigrannajaryan]

Now that #503 has been merged, we can discuss this PR again. IIRC @tigrannajaryan wanted this information to exist in the newly added flags field in the mentioned PR, rather than exist as an independent parent_is_remote field. Could you confirm/clarify this please?

Yes, that was my suggestion. We can take 2 more bits from flags to represent the 3 states we need for parent_is_remote.

[AlexanderWert]

@open-telemetry/specs-approvers I think this is ready to be merged!

[tigrannajaryan]

@open-telemetry/specs-approvers I think this is ready to be merged!

Needs at least 2 business days since last change before merging.

[estolfo]

@open-telemetry/specs-approvers can we merge this please?

[tigrannajaryan]

@estolfo looks like we forgot one more thing: an entry in the CHANGELOG in the Unreleased section. Please add and we can merge.

[estolfo]

@tigrannajaryan ok done

[tigrannajaryan]

Thank you @estolfo and everyone for patience!