Tracing POC

[nomisRev]

Discussing some ideas for new features with @raulraja he mentioned tracing for Effect, so I was playing around with some ideas and come up with a small POC.

Since raise already relies on CancellationException to co-operate with the coroutine system we currently pay a performance penalty for raise and the stack trace creation that comes with CancellationException by default. This penalty is not needed, and is stack trace creation is by default also disabled in KotlinX Coroutines.

This PR solves the issue of debugging/tracing, and the performance penalty we currently pay for raise.
We can now disable the stack trace creation, and we can ad-hoc enable it for any Effect you compose traced upon or any Raise program you run within a traced block.

public fun main() {
  val error = effect<String, Int> { raise("error") }
  error.traced { traced -> traced.printStackTrace() }
    .fold({ require(it == "error") }, { error("impossible") })
}

arrow.core.continuations.RaiseCancellationException: Raised Continuation
  at arrow.core.continuations.DefaultRaise.raise(Fold.kt:77)
  at MainKtKt$main$error$1.invoke(MainKt.kt:6)
  at MainKtKt$main$error$1.invoke(MainKt.kt:6)
  at arrow.core.continuations.Raise$DefaultImpls.bind(Shift.kt:22)
  at arrow.core.continuations.DefaultRaise.bind(Fold.kt:74)
  at arrow.core.continuations.Effect__TracingKt$traced$2.invoke(Tracing.kt:46)
  at arrow.core.continuations.Effect__TracingKt$traced$2.invoke(Tracing.kt:46)
  at arrow.core.continuations.Effect__FoldKt.fold(Fold.kt:92)
  at arrow.core.continuations.Effect.fold(Unknown Source)
  at MainKtKt.main(MainKt.kt:8)
  at MainKtKt.main(MainKt.kt)

Notes

• Since the Effect runtime is shared amongst DSL such as either, result, option, ior, etc the traced operator can be added, or used for all these DSLs, and types. Including any of the DSL methods that result in shifting such as ensure, ensureNotNull, bind, etc.

• Currently only JVM is disabling stack-trace generation. Not sure if we also need to disable it for JS, or Native. (KotlinX Coroutines only disables it for JVM)

• What API would we want on Traced<R>? Currently we include mentions to ShiftCancellationException and the dynamic call to DefaultShift#shift which is not part of the user stack trace. Should we slightly change the API such that this is improved?

[github-actions]

Kover Report

File	Coverage [57.49%]
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Builders.kt	94.74%
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Fold.kt	97.92%
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/RaiseAccumulate.kt	24.07%
arrow-libs/core/arrow-core/src/commonMain/kotlin/arrow/core/raise/Trace.kt	25.00%
arrow-libs/core/arrow-core/src/jsMain/kotlin/arrow/core/raise/CancellationExceptionNoTrace.kt	100.00%
arrow-libs/core/arrow-core/src/jvmMain/kotlin/arrow/core/raise/CancellationExceptionNoTrace.kt	100.00%
arrow-libs/core/arrow-core/src/nativeMain/kotlin/arrow/core/raise/CancellationExceptionNoTrace.kt	100.00%

Total Project Coverage	42.88%

[kyay10]

Is there a reason to distinguish between RaiseCancellationExceptionNoTrace and RaiseCancellationException on a type level? it seems to me that a val isTraced might be sufficient, although I see that this might be an unnecessary overhead. Instead, would an interface for RaiseCancellationException make sense? That is, to remove the duplication in the RaiseCancellationException.raised and RaiseCancellationException.raise properties.

Also, is there a reason as to why Traced needs to exist as a class? It seems sufficient to just pass the needed information to the lambda, and Traced seems to just be delegating its methods to its properties anyways.

[nomisRev]

Thanks for your review and feedback @kyay10 🙏

Is there a reason to distinguish between RaiseCancellationExceptionNoTrace and RaiseCancellationException on a type level? it seems to me that a val isTraced might be sufficient, although I see that this might be an unnecessary overhead.

Okay, so this was an attempt to hide RaiseCancellationExceptionXXX from the public API, but in turn CancellationExceptionNoTrace is public so not sure if that's really better 🤔 Sadly expect/actual cannot be private 😞

Also, is there a reason as to why Traced needs to exist as a class? It seems sufficient to just pass the needed information to the lambda, and Traced seems to just be delegating its methods to its properties anyways.

The reason for Traced was to have a bit more flexibility. Having Trace as an @JvmInline value class is perhaps better. The reason I modelled it like this is because I wanted to encapsulate the Throwable that gives more flexibility for the future.

I would've really liked to sanitise the trace, but I'm not sure if there is a decent way to do this for Native / JS. We could do it on minimum-effort for JVM only, it seems that is also what KotlinX Coroutines is doing.

I've played around with the changes, and made a commit applying your suggestions @kyay10. WDYT? To my surprise RaiseCancellationException didn't become public in the api file.

[kyay10]

LGTM, perhaps just fix the (very minor) typo I commented above.

Should there also be changes to all the comprehension blocks (is that what we still call them btw?) to add a traced block for each e.g. option.traced? Or is that not useful?

[nomisRev]

LGTM, perhaps just fix the (very minor) typo I commented above.

Oh yes, missed that in the PR 👍

Should there also be changes to all the comprehension blocks (is that what we still call them btw?) to add a traced block for each e.g. option.traced? Or is that not useful?

This shouldn't be necessary you can use traced({ option.bind() }) { } and get a trace from it since bind short-circuit through raise. The biggest issue, or downside, is that it traces Raise and has no way of figuring out where the Option was created. So compared to regular exceptions the trace will point to the throws call and not the Throwable#init.

[nomisRev]

@kyay10 actually I think this change is impossible without having two types 🤔 Since fillInStackTrace is called in the init method of Throwable it will always be uninitialised, hence false. There is no way to influence this so it requires two separate classes.

[kyay10]

@nomisRev Ah that's peculiar! Could RaiseCancellationException become an internal sealed interface with tracing and non-tracing impls? Perhaps even with a companion object { operator fun invoke(..., isTraced: Boolean): RaiseCancellationException } that returns the correct impl based on isTraced.
If that's too complex, honestly I'm fine with the minor code duplication in this case. It's a deep implementation detail that likely won't be modified often, so it might be okay to ignore DRY.

[nomisRev]

Ah that's peculiar!

It's standard behavior for the JVM. Initialisation order of https://stackoverflow.com/questions/32490714/order-of-initialization-instantiation-of-class-variables-of-derived-class-and-in.

It's a deep implementation detail that likely won't be modified often, so it might be okay to ignore DRY.

I think it's fine, especially in a library like Arrow. Optimise for the user facing API > optimise for less code in library.

[kyay10]

Maybe Trace could be made a value class? I think the other commit (the one that failed the tests) had it as such.

[serras]

Looks good to me :)

[serras]

Suggested change

	this is RaiseCancellationExceptionNoTrace && this.raise === raise -> raised as R
	this is RaiseCancellationException && this.raise === raise -> raised as R
	this.raise === raise && (this is RaiseCancellationExceptionNoTrace || this is RaiseCancellationException) -> raised as R

[franciscodr]

This change introduces a compilation error because raise and raised properties don't exist in CancellationException. Those properties belong to RaiseCancellationExceptionNoTrace and RaiseCancellationException, so we need the smart cast here.

[raulraja]

❤️

[nomisRev]

Thanks for the great suggestions and review @serras 🙏

[nomisRev]

Thanks for fixing the build @franciscodr

[nomisRev]

Wow, binary says BOOM due to value class 🤯 Never realised this before