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 SpanContext to SpanReference. #1075

Merged

Conversation

carlosalberto
Copy link
Contributor

Renamed SpanContext to SpanReference. This is done in order to prevent the overuse of the Context term.

Inspired by the discussion happening in #1033 - see #1033 (comment)

@carlosalberto carlosalberto requested a review from a team October 8, 2020 16:54
@carlosalberto carlosalberto added area:api Cross language API specification issue spec:trace Related to the specification/trace directory labels Oct 8, 2020
spec-compliance-matrix.md Outdated Show resolved Hide resolved
@@ -76,7 +76,7 @@ Injects the value into a carrier. For example, into the headers of an HTTP reque
Required arguments:

- A `Context`. The Propagator MUST retrieve the appropriate value from the `Context` first, such as
`SpanContext`, `Baggage` or another cross-cutting concern context.
`SpanReference`, `Baggage` or another cross-cutting concern context.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we only put Span in Context, should we use Span for this and the next few files?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Oberon00 Perhaps this is appropriate for #1079?

Copy link
Member

@Oberon00 Oberon00 Oct 9, 2020

Choose a reason for hiding this comment

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

I think this would more be a follow-up of #994. I suggested related changes right in this PR as it neatly highlights all usages, but I think you are right that this could be in a separate PR. I don't think it belongs into #1079 though.

specification/metrics/api.md Outdated Show resolved Hide resolved
specification/metrics/api.md Outdated Show resolved Hide resolved
specification/metrics/api.md Outdated Show resolved Hide resolved
specification/metrics/api.md Outdated Show resolved Hide resolved
specification/metrics/api.md Outdated Show resolved Hide resolved
specification/overview.md Outdated Show resolved Hide resolved

### SpanContext
### SpanReference

Represents all the information that identifies **Span** in the **Trace** and
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion:

Suggested change
Represents all the information that identifies **Span** in the **Trace** and
Represents all the information that is required to reference a **Span** in the **Trace** unambiguously and efficiently and

E.g. in Dynatrace we could reference the span with just span+trace ID but require some routing info in the trace state to do so efficiently. Not sure if this is generally applicable enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely something that can be done in a another PR, specially because this was not even touched in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think this one is more related to that PR, as the name change reflects a change in the understanding of that type. But I'm fine with doing this in a follow-up too (I already approved)


Represents all the information that identifies **Span** in the **Trace** and
MUST be propagated to child Spans and across process boundaries. A
**SpanContext** contains the tracing identifiers and the options that are
**SpanReference** contains the tracing identifiers and the options that are
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**SpanReference** contains the tracing identifiers and the options that are
**SpanReference** contains the tracing identifiers, tracestate and the options that are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, the lack of trace state is not related to this PR.

specification/overview.md Outdated Show resolved Hide resolved
specification/overview.md Outdated Show resolved Hide resolved
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.

Generally I'm in favor of this change.

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.

I'm also in favor of this change. Thanks @carlosalberto.

spec-compliance-matrix.md Show resolved Hide resolved
carlosalberto and others added 7 commits October 11, 2020 02:46
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
@carlosalberto
Copy link
Contributor Author

carlosalberto commented Oct 11, 2020

Applied some feedback - but please, let's avoid scope creep and do a few things in follow ups (for example, the tracetstate missing part). I applied the SpanContext -> Span (Propagated Span) suggestions to speed up things, but that should have been a different PR.

@carlosalberto
Copy link
Contributor Author

@open-telemetry/specs-approvers I will merge this PR by EOD - we have enough approvals, and was already discussed, but want to give some extra time for further reviews ;)

@bogdandrutu
Copy link
Member

@carlosalberto I think we are good, 5 approvals more than enough

@jmacd
Copy link
Contributor

jmacd commented Oct 21, 2020

This was discussed in the Spec SIG (APAC edition) today, and there is a will to revert this change to the specification. AI: @carlosalberto ?

@tigrannajaryan
Copy link
Member

@jmacd it would be great to post a summary of the discussion with the reasoning. I am not against reverting but I am interested in knowing the reasons.

@Oberon00
Copy link
Member

I can live with both, but I think SpanContext has the real drawback that it is not the Context of a span.

carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 21, 2020
carlosalberto added a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
* Rename SpanContext to SpanReference.

* More updates.

* Update CHANGELOG

* Update specification/metrics/api.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

* Update specification/metrics/api.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

* Update specification/metrics/api.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

* Update specification/metrics/api.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

* Update specification/overview.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

* Update specification/overview.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

* Update specification/metrics/api.md

Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>

* Add entry to the compliance matrix.

* Update specification/overview.md

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>

Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Armin Ruech <armin.ruech@dynatrace.com>
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
area:api Cross language API specification issue spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants