Skip to content

Add option to disable span activation #1187

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.

Sign up for GitHub

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

Closed
mwisnicki wants to merge 6 commits into from
Closed

Add option to disable span activation #1187

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d1b5118
Activate child spans just like root spans
mwisnicki Jun 23, 2025
125dc51
Add option to disable span activation
mwisnicki Jun 23, 2025
63a194a
Attempt to fix mima
mwisnicki Jun 23, 2025
a133129
Use copy
mwisnicki Jun 25, 2025
9069ce6
Rename to shouldActivateSpan
mwisnicki Jun 25, 2025
69e9c39
Move scaladoc to setter where it's more useful and expand
mwisnicki Jun 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import com.typesafe.tools.mima.core._

ThisBuild / tlBaseVersion := "0.3"
ThisBuild / tlBaseVersion := "0.4"
Copy link
Member

@bpholt bpholt Jun 23, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mwisnicki mwisnicki Jun 25, 2025

Choose a reason for hiding this comment

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

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.


val scala212Version = "2.12.20"
val scala213Version = "2.13.16"
Expand Down
22 changes: 15 additions & 7 deletions modules/core/shared/src/main/scala/Span.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

@kubukoz kubukoz Jun 23, 2025

Choose a reason for hiding this comment

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

nitpick: shouldActivateSpan, perhaps? The verb form gives me a little ick.

Or spanActivationPolicy, that'd be more consistent with the existing spanCreationPolicy param

Copy link
Contributor Author

@mwisnicki mwisnicki Jun 25, 2025

Choose a reason for hiding this comment

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

renamed to shouldActivateSpan, adding enum for binary option seemed overkill


def withParentKernel(kernel: Kernel): Options
def withoutParentKernel: Options
def withSpanCreationPolicy(p: Options.SpanCreationPolicy): Options

def withSpanKind(spanKind: SpanKind): Options

def withLink(kernel: Kernel): Options

def withActivateSpan(activateSpan: Boolean): Options
}

object Options {
Expand All @@ -202,22 +207,25 @@ object Span {
parentKernel: Option[Kernel],
spanCreationPolicy: SpanCreationPolicy,
spanKind: SpanKind,
links: Chain[Kernel]
links: Chain[Kernel],
activateSpan: Boolean
) extends Options {
override def withParentKernel(kernel: Kernel): Options =
OptionsImpl(Some(kernel), spanCreationPolicy, spanKind, links)
OptionsImpl(Some(kernel), spanCreationPolicy, spanKind, links, activateSpan)
Copy link
Contributor Author

@mwisnicki mwisnicki Jun 23, 2025

Choose a reason for hiding this comment

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

why not copy?

Copy link
Member

@kubukoz kubukoz Jun 23, 2025

Choose a reason for hiding this comment

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

oversight? :)

Copy link
Contributor Author

@mwisnicki mwisnicki Jun 25, 2025

Choose a reason for hiding this comment

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

changed to use copy

override def withoutParentKernel: Options =
OptionsImpl(None, spanCreationPolicy, spanKind, links)
OptionsImpl(None, spanCreationPolicy, spanKind, links, activateSpan)
override def withSpanCreationPolicy(p: SpanCreationPolicy): Options =
OptionsImpl(parentKernel, p, spanKind, links)
OptionsImpl(parentKernel, p, spanKind, links, activateSpan)
override def withSpanKind(spanKind: SpanKind): Options =
OptionsImpl(parentKernel, spanCreationPolicy, spanKind, links)
OptionsImpl(parentKernel, spanCreationPolicy, spanKind, links, activateSpan)
override def withLink(kernel: Kernel): Options =
OptionsImpl(parentKernel, spanCreationPolicy, spanKind, links.append(kernel))
OptionsImpl(parentKernel, spanCreationPolicy, spanKind, links.append(kernel), activateSpan)
override def withActivateSpan(activateSpan: Boolean): Options =
OptionsImpl(parentKernel, spanCreationPolicy, spanKind, links, activateSpan)
}

val Defaults: Options =
OptionsImpl(None, SpanCreationPolicy.Default, SpanKind.Internal, Chain.empty)
OptionsImpl(None, SpanCreationPolicy.Default, SpanKind.Internal, Chain.empty, true)
Copy link
Member

@kubukoz kubukoz Jun 23, 2025

Choose a reason for hiding this comment

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

nitpick: I suggest using named params

Copy link
Contributor Author

@mwisnicki mwisnicki Jun 25, 2025

Choose a reason for hiding this comment

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

done

val Suppress: Options = Defaults.withSpanCreationPolicy(SpanCreationPolicy.Suppress)
val Coalesce: Options = Defaults.withSpanCreationPolicy(SpanCreationPolicy.Coalesce)

Expand Down
8 changes: 6 additions & 2 deletions modules/datadog/src/main/scala/DDEntryPoint.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ final class DDEntryPoint[F[_]: Sync](tracer: ot.Tracer, uriPrefix: Option[URI])
Resource
.make(initSpan())(s => Sync[F].delay(s.finish()))
.flatTap(span =>
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)
)
.map(DDSpan(tracer, _, uriPrefix, options))
}
Expand All @@ -52,7 +54,9 @@ final class DDEntryPoint[F[_]: Sync](tracer: ot.Tracer, uriPrefix: Option[URI])
Resource
.make(initSpan())(s => Sync[F].delay(s.finish()))
.flatTap(span =>
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)
Copy link
Member

@kubukoz kubukoz Jun 23, 2025

Choose a reason for hiding this comment

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

question: any other entrypoints/spans that should have this?

Copy link
Contributor Author

@mwisnicki mwisnicki Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mwisnicki mwisnicki Jun 29, 2025

Choose a reason for hiding this comment

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

Found a better solution to java-interop: #1189

Copy link
Contributor Author

@mwisnicki mwisnicki Jun 29, 2025

Choose a reason for hiding this comment

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

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.

)
.map(DDSpan(tracer, _, uriPrefix, options))
}
Expand Down
5 changes: 5 additions & 0 deletions modules/datadog/src/main/scala/DDSpan.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ final case class DDSpan[F[_]: Sync](
case (span, ExitCase.Errored(e)) => Sync[F].delay(span.log(e.toString).finish())
case (span, _) => Sync[F].delay(span.finish())
}
.flatTap(span =>
Resource
.make(Sync[F].delay(tracer.activateSpan(span)))(s => Sync[F].delay(s.close()))
.whenA(options.activateSpan)
)
.map(DDSpan(tracer, _, uriPrefix, options))
)

Expand Down