all: always check if opentracing.SpanFromContext is nil#42027
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 4ba015f...5e90b3f.
|
|
TBH it feels like |
camdencheek
left a comment
There was a problem hiding this comment.
How do we feel about making SpanFromContext always return a valid value? In the case when there is no active span, its methods would just be no-ops. That way consumers of the tracing API don't need to care whether or not tracing is enabled. We'd only care if tracing is active when initializing a trace, probably at the API layer.
In OpenTelemetry, I believe you can continue on as if a span is always returned - OpenTelemetry will provide a no-op span that can be used with no effect.
We can't in this case, because |
policy.ShouldTrace does not guarantee that span has been started within a context, just that one should be created if anyone asks. opentracing.SpanFromContext documents that it can return nil if no span is started, which may cause a panic when used. To mitigate this, this we update the docstring of policy.ShouldTrace and make sure usages of opentracing.SpanFromContext always check for the nil-ness of the span. We do this even if a trace is started right before, just in case. The OpenTelemetry equivalent, trace.SpanFromContext, returns a zero-value no-op span instead (though there are no usages of this function yet).
policy.ShouldTrace does not guarantee that span has been started within a context, just that one should be created if anyone asks. opentracing.SpanFromContext documents that it can return nil if no span is started, which may cause a panic when used. To mitigate this, this we update the docstring of policy.ShouldTrace and make sure usages of opentracing.SpanFromContext always check for the nil-ness of the span. We do this even if a trace is started right before, just in case. The OpenTelemetry equivalent, trace.SpanFromContext, returns a zero-value no-op span instead (though there are no usages of this function yet).
policy.ShouldTracedoes not guarantee that span has been started within a context, just that one should be created if anyone asks.opentracing.SpanFromContextdocuments that it can return nil if no span is started, which may cause a panic when used (https://github.com/sourcegraph/sourcegraph/pull/42003). To mitigate this, this we update the docstring ofpolicy.ShouldTraceand make sure usages ofopentracing.SpanFromContextalways check for the nil-ness of the span. We do this even if a trace is started right before, just in case.The OpenTelemetry equivalent,
trace.SpanFromContext, returns a zero-value no-op span instead (though there are no usages of this function yet).Test plan
tests pass still