Add option to disable span activation #1187
Conversation
Activation binds span to current thread via ThreadLocal so it will become incorrect when IO/Fiber switches threads but at least it's an improvement and consistent with root spans. Ideally there would be a Span option to disable activation entirely.
For now implemented only for Datadog.
| ) extends Options { | ||
| override def withParentKernel(kernel: Kernel): Options = | ||
| OptionsImpl(Some(kernel), spanCreationPolicy, spanKind, links) | ||
| OptionsImpl(Some(kernel), spanCreationPolicy, spanKind, links, activateSpan) |
There was a problem hiding this comment.
changed to use copy
| @@ -175,13 +175,18 @@ object Span { | |||
| def spanKind: Span.SpanKind | |||
| def links: Chain[Kernel] | |||
|
|
|||
| /** Whether to activate span in underlying tracing framework. This typically binds span to a ThreadLocal. */ | |||
| def activateSpan: Boolean | |||
There was a problem hiding this comment.
nitpick: shouldActivateSpan, perhaps? The verb form gives me a little ick.
Or spanActivationPolicy, that'd be more consistent with the existing spanCreationPolicy param
There was a problem hiding this comment.
renamed to shouldActivateSpan, adding enum for binary option seemed overkill
| } | ||
|
|
||
| val Defaults: Options = | ||
| OptionsImpl(None, SpanCreationPolicy.Default, SpanKind.Internal, Chain.empty) | ||
| OptionsImpl(None, SpanCreationPolicy.Default, SpanKind.Internal, Chain.empty, true) |
There was a problem hiding this comment.
nitpick: I suggest using named params
| Resource.make(Sync[F].delay(tracer.activateSpan(span)))(s => Sync[F].delay(s.close())) | ||
| Resource | ||
| .make(Sync[F].delay(tracer.activateSpan(span)))(s => Sync[F].delay(s.close())) | ||
| .whenA(options.activateSpan) |
There was a problem hiding this comment.
question: any other entrypoints/spans that should have this?
There was a problem hiding this comment.
Presumably other backends could use it but I'm not familiar with them and don't know if anyone uses them.
ThreadLocal based span activation on start is error prone so I'm not even sure if it's worth adding this option except it's the default behavior of datadog backend and I want ability to selectively disable it.
With opentelemetry a better solution would be similar to typelevel/otel4s#214
In general there really ought to be some kind of Java-interop method like in otel4s.
There was a problem hiding this comment.
Found a better solution to java-interop: #1189
There was a problem hiding this comment.
Although disabling automatic span activation is still the right thing to do.
It seems other backends don't activate by default so maybe a better option is to do the opposite of #1186 and just disable it completely.
| @@ -1,6 +1,6 @@ | |||
| import com.typesafe.tools.mima.core._ | |||
|
|
|||
| ThisBuild / tlBaseVersion := "0.3" | |||
| ThisBuild / tlBaseVersion := "0.4" | |||
There was a problem hiding this comment.
I haven't had a chance to fully look at this PR yet, but is there any way to achieve this without an early-semver-major version bump?
There was a problem hiding this comment.
I guess I would have to provide default noop implementations in the trait?
I.e. def withActivateSpan(b: Boolean) = this. Not sure if this makes sense.
I wish I knew how to test it without waiting for PR checks.
I've tried sbt ++2.13.16 mimaReportBinaryIssues as per https://typelevel.org/cats/contributing.html but it's not working:
[info] natchez-core: mimaPreviousArtifacts is empty, not analyzing binary compatibility.
For now implemented only for Datadog.
Stacked on top of: