Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename SpanReference to SpanContext (revert #1075) #1126

Closed
carlosalberto opened this issue Oct 21, 2020 · 6 comments · Fixed by #1127
Closed

Rename SpanReference to SpanContext (revert #1075) #1126

carlosalberto opened this issue Oct 21, 2020 · 6 comments · Fixed by #1127
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory

Comments

@carlosalberto
Copy link
Contributor

carlosalberto commented Oct 21, 2020

What are you trying to achieve?

Revert #1075 after it was badly received in the actual implementations. Overall, the reasons behind going back to SpanContext are:

  • SpanContext is an already very well known term.
  • SpanReference may make people think of pointers/references.
  • SpanReference includes additional information to Spans, such as tracestate.
  • Too late in the game to make this change overall, as well as the lack of a true perfect solution.

For this change, we will actually expect more than the usual reviews, in order to verify with SIG maintainers/approvers this is a good thing.

@carlosalberto carlosalberto added area:api Cross language API specification issue spec:trace Related to the specification/trace directory release:required-for-ga Must be resolved before GA release, or nice to have before GA priority:p1 Highest priority level labels Oct 21, 2020
@jmacd
Copy link
Contributor

jmacd commented Oct 21, 2020

This was discussed extensively in yesterday's Spec SIG (APAC) call. We found among the group general consensus that SpanContext is not a perfect term, but that the cost of changing from SpanContext (which is well established) is too high to justify the change.

@marcingrzejszczak
Copy link

I wasn't there at the meeting and I don't know if the discussion is closed or not but in Brave the term is TraceContext and it's also well established 🤷

@tsloughter
Copy link
Member

As I mentioned in the meeting, while I think SpanReference (or similar so it isn't confusing to those users of languages with references) would be an improvement, the use of SpanContext is everywhere, code and docs and blogs, it is just simpler to not change unless there is a really really good reason.

Also a good point that it isn't a reference to a Span, it contains trace state too. But it also "isn't" a TraceContext since it is the necessary identities for a particular Span plus the Trace state.

So defining the term really well is the best way forward, even if a term could be come up with that perfectly states what it is -- like TracePropagatationIdentifiers :)

@reyang
Copy link
Member

reyang commented Oct 22, 2020

I vote for keeping SpanContext. SpanReference is a very confusing term for both C# (.NET) and C++.

@Oberon00
Copy link
Member

Oberon00 commented Oct 22, 2020

SpanContext is also a very confusing term because it is not the Context of a Span. I think reference is less so (e.g. there is also https://docs.microsoft.com/en-us/dotnet/api/system.windows.markup.reference or https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.xml.reference). The only argument for reverting that I buy is that SpanContext is already well-known. If we introduced the concept today, I would strongly object against SpanContext.

@yurishkuro
Copy link
Member

yurishkuro commented Oct 22, 2020

+1 to go back to SpanContext.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants