fix: set context in in_span #551
Conversation
| span = start_span(name, attributes: attributes, links: links, start_timestamp: start_timestamp, kind: kind, with_parent: with_parent) | ||
| Trace.with_span(span) { |s, c| yield s, c } | ||
| if with_parent.nil? | ||
| span = start_span(name, attributes: attributes, links: links, start_timestamp: start_timestamp, kind: kind, with_parent: with_parent) |
There was a problem hiding this comment.
If with_parent is nil, why pass it here?
| span = start_span(name, attributes: attributes, links: links, start_timestamp: start_timestamp, kind: kind, with_parent: with_parent) | ||
| Trace.with_span(span) { |s, c| yield s, c } | ||
| else | ||
| OpenTelemetry::Context.with_current(with_parent) do |
There was a problem hiding this comment.
Another way to code this would be to make OpenTelemetry::Context.with_current yield and return if with_parent is nil. That would eliminate the awkward if statement in this method.
|
I think that we probably want this behavior. I mentioned this on #630, but without this change our Rack instrumentation does not automatically pick up baggage. We could treat this as a bug in the Rack instrumentation, and not use |
|
I think we should fix the Rack instrumentation, and probably other instrumentation that propagates trace context (Sidekiq?). My concern with changing behaviour of |
|
Replaced by #729 |
THIS IS NOT INTENDED TO BE MERGED AS IS - IT IS INTENDED FOR DISCUSSION OF SEMANTICS ONLY
#549 shows two code examples that behave differently. At first glance, it seems like they should behave the same way, and this PR attempts to "fix" the
Tracer.in_spanhelper to make them behave the same way.I want to make it clear that, after close reading of the spec, I do NOT think we should merge this. I think the current behaviour is correct, even if it isn't immediately intuitive.
in_spanis a helper that we've added. The spec states:This is what
in_spanimplements. The spec says nothing about this optional operation setting an outer Context, and I don't think it should. It should simply behave as described: create a span as ifstart_spanwere called with the same parameters and then make it active for the duration of the provided block, usingTrace.with_span.As discussed in today's call, the behaviour we're trying to achieve in #549 doesn't seem like a common case, and largely falls under the heading of "Kafka is hard".
cc @Asafb26