Skip to content
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.

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

core-trace: SpanBuilder use macro to preserve laziness #734

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Aug 21, 2024

Fixes #725.

Let's take a look at how the following code will be unwrapped:

val result = b.withSpanKind(kind).withStartTimestamp(timestamp).withParent(ctx)

Scala 2 (chained calls are optimized manually by the macro):

val result = if (b.meta.isEnabled) {
  b.modifyState(_.withSpanKind(kind).withStartTimestamp(timestamp).withParent(ctx))
} else {
  b
}

The Scala 3 chained calls aren't optimized:

val result = {
  val SpanBuilderMacro_this: org.typelevel.otel4s.trace.SpanBuilder[[A >: scala.Nothing <: scala.Any] => cats.effect.IO[A]] = b
  val `SpanBuilderMacro_this₂`: org.typelevel.otel4s.trace.SpanBuilder[[A >: scala.Nothing <: scala.Any] => cats.effect.IO[A]] = (if (SpanBuilderMacro_this.meta.isEnabled) SpanBuilderMacro_this.modifyState(((_$11: org.typelevel.otel4s.trace.SpanBuilder.State) => _$11.withSpanKind(kind))) else SpanBuilderMacro_this: org.typelevel.otel4s.trace.SpanBuilder[[A >: scala.Nothing <: scala.Any] => cats.effect.IO[A]])
  val `SpanBuilderMacro_this₃`: org.typelevel.otel4s.trace.SpanBuilder[[A >: scala.Nothing <: scala.Any] => cats.effect.IO[A]] = (if (`SpanBuilderMacro_this₂`.meta.isEnabled) `SpanBuilderMacro_this₂`.modifyState(((_$13: org.typelevel.otel4s.trace.SpanBuilder.State) => _$13.withStartTimestamp(timestamp))) else `SpanBuilderMacro_this₂`: org.typelevel.otel4s.trace.SpanBuilder[[A >: scala.Nothing <: scala.Any] => cats.effect.IO[A]])

  (if (`SpanBuilderMacro_this₃`.meta.isEnabled) `SpanBuilderMacro_this₃`.modifyState(((_$15: org.typelevel.otel4s.trace.SpanBuilder.State) => _$15.withParent(org.typelevel.otel4s.trace.SpanBuilderMacro.inline$Explicit$i1(_root_.org.typelevel.otel4s.trace.SpanBuilder.Parent).apply(ctx)))) else `SpanBuilderMacro_this₃`: org.typelevel.otel4s.trace.SpanBuilder[[A >: scala.Nothing <: scala.Any] => cats.effect.IO[A]])
}

It's not the end of the world for Scala 3, JIT compiler will inline the if statements in the runtime, but still sad that we produce non-optimized code.


Pros

  • Zero allocation builder when instrumentation is disabled

Cons

  • It's a macro, a complicated one
  • Scala 3 generated code could be improved

@iRevive iRevive added the tracing Improvements to tracing module label Aug 21, 2024
@iRevive iRevive force-pushed the core-trace/span-builder-macro branch from 1cc19cb to 61b69fd Compare August 21, 2024 16:41
Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

I love this. on the surface, it seems like we've made things much more complicated, but:

  • no more Tracer.Meta
  • substantial deduplication of SpanBuilder implementations
  • huge simplification of the Scala 3 TracerMacro


// if the optimization is not applied, the compilation will fail with
// Method too large: org/typelevel/otel4s/...
// Scala 3 compiler optimizes chained calls out of the box
Copy link
Contributor

Choose a reason for hiding this comment

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

that's very kind of them

@@ -59,7 +58,7 @@ private[otel4s] trait TracerMacro[F[_]] {
inline name: String,
inline attributes: Attribute[_]*
): SpanOps[F] =
${ TracerMacro.span('self, 'name, 'attributes) }
spanBuilder(name).addAttributes(attributes).build
Copy link
Contributor

Choose a reason for hiding this comment

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

I love how the only sense in which this is even a macro now is that everything is inline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. Scala 3 macro feels much better now.

@iRevive iRevive force-pushed the core-trace/span-builder-macro branch from d0078b6 to daae6f7 Compare August 27, 2024 08:02
@iRevive iRevive force-pushed the core-trace/span-builder-macro branch from daae6f7 to 22ff66b Compare August 27, 2024 08:04
@iRevive iRevive merged commit d87f032 into typelevel:main Aug 29, 2024
10 checks passed
@iRevive iRevive deleted the core-trace/span-builder-macro branch August 29, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracing Improvements to tracing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting the SpanKind is tedious
2 participants