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

Revert "Rename SpanContext to SpanReference. (#1075)" #1127

Merged

Conversation

carlosalberto
Copy link
Contributor

Fixes #1126

I'd like to get the eyes of as many people as possible. We will also reserve a few days before (hopefully) merging this.

@carlosalberto carlosalberto requested review from a team October 21, 2020 18:52
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion, approving to indicate I saw this and fine with the change.
I advise all @open-telemetry/specs-approvers and language maintainers to also indicate that they saw this even if you don't have any comments.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dyladan
Copy link
Member

dyladan commented Oct 21, 2020

Seems fine to me. I think we've used the term "context" in far too many places. The cost of changing now is high, but what is the point of alpha/beta cycles if not to make expensive changes before GA?

@tsloughter
Copy link
Member

Put my reasoning for approval in the issue #1126 (comment)

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still slightly favor SpanReference, but probably because it's more idiomatic in Java than other languages since everything's a reference anyways. For the record, this is the sort of code that's unfortunately bemuddled with two contexts, but I understand the motivation to not make a big change at this point

Span.fromContext(context).getSpanContext()

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving because I agree the cost of renaming may be too high. I do think that SpanReference is the better name for that concept though.

Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as @Oberon00.
@carlosalberto you might want to indicate in the changelog that the change was reverted.

@carlosalberto
Copy link
Contributor Author

@arminru Removed the original entry in the CHANGELOG regarding the rename to SpanReference ;) Hope that works!

@bogdandrutu
Copy link
Member

Wow only 10 approvals :))

@carlosalberto
Copy link
Contributor Author

Merging this as we have 1) enough approvals (7 approvers and 3 language SIG approvers), and 2) let it soak for a pair of days already.

carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename SpanReference to SpanContext (revert #1075)