fix!: rename SpanContext to SpanReference #440
Conversation
fbogsany
requested review from
dazuma,
ericmustin,
mwear and
robertlaurin
as code owners
Yeah - "someone" expressed the same sentiment to me privately 😄 . Let's hold off on merging this until next week? I don't think it's truly final yet. |
I echo these sentiments and know there are some ongoing conversations about this. It's probably a good idea to hold off merging until the dust settles. |
|
There's a spec issue to revert this: open-telemetry/opentelemetry-specification#1126 |
|
The change was reverted in the spec, this PR can be closed |
| @@ -254,34 +254,34 @@ | |||
|
|
|||
| it 'uses the context from parent if supplied' do | |||
| parent = tracer.start_root_span('root') | |||
| parent_ctx = OpenTelemetry::Trace.with_span(parent) { |_, ctx| ctx } | |||
| parent_ctx = OpenTelemetry::Trace.context_with_span(parent) | |||
There was a problem hiding this comment.
This change should be made regardless (in another PR).
| @@ -227,7 +227,7 @@ | |||
| end | |||
|
|
|||
| it 'creates a span with all supplied parameters' do | |||
| links = [OpenTelemetry::Trace::Link.new(context)] | |||
| links = [OpenTelemetry::Trace::Link.new(span_reference)] | |||
There was a problem hiding this comment.
I think we need to change this too - argument should be span_context instead of context.
| parent_span_reference = nil unless parent_span_reference.valid? | ||
| parent_span_id = parent_span_reference&.span_id | ||
| tracestate = parent_span_reference&.tracestate | ||
| trace_id = parent_span_reference&.trace_id |
There was a problem hiding this comment.
This logic should be cleaned up - I suspect the obvious change would be more efficient than the repeated "lonely operator".
| # | ||
| # @return [SpanContext] | ||
| attr_reader :context |
There was a problem hiding this comment.
I feel like this should be span_context, although we avoid that name in Span due to stuttering (but perhaps we shouldn't?).
| private_constant :INVALID_TRACE_ID, :INVALID_SPAN_ID | ||
|
|
||
| class << self | ||
| # Creates a new {TraceParent} from a supplied {Trace::SpanContext} | ||
| # @param [SpanContext] ctx The context |
There was a problem hiding this comment.
Should read span context
| # @return [TraceParent] a trace parent | ||
| def from_context(ctx) |
There was a problem hiding this comment.
Perhaps rename this from_span_context
|
|
||
| carrier | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def span_context_from(context) | ||
| OpenTelemetry::Trace.current_span(context)&.context |
There was a problem hiding this comment.
I believe we can get rid of the lonely operator here.
|
I'll close this, however I've identified a bunch of changes we should make regardless (in another PR). |
Implements open-telemetry/opentelemetry-specification#1075