Ported improved stack traces from monix #190

WesselVS · 2020-10-31T19:19:33Z

Thanks for this library! I am really excited about the stack traces functionality in monix, so I decided to help a little bit with getting this into bio. It's based on monix/monix#1267. Couple of things:

In TaskRunLoop, augmentException is only used on throwable types. I used isInstanceOf checks to guard this.
The tracing unit tests do not produce equivalent results between monix and monix-bio. It seems that 'runToFuture' traces differ in numbers. This smells like a bug somewhere, but the traces still seem reasonable.
I copied RingBuffer, this needs to be removed when the new monix release is available.

Avasil

Thank you so much!
You are saving me a lot of work. :)

The tracing unit tests do not produce equivalent results between monix and monix-bio. It seems that 'runToFuture' traces differ in numbers. This smells like a bug somewhere, but the traces still seem reasonable.

I will play with the changes after merging this PR and before releasing but it's probably not a blocker

I copied RingBuffer, this needs to be removed when the new monix release is available.

I'm going through the release process right now but I'm doing it through GitHub Actions for the first time so it will probably take a few tries before it works properly 😅

Avasil · 2020-11-07T10:20:01Z

build.sbt

+    fork in Test := true,
+    fork in FullTracingTest := true,
+    javaOptions in Test ++= Seq(
+      "-Dmonix.bio.tracing=true",


There's no such option, I blindly copied it from Cats-Effect, will have to correct it. Maybe it should be enhancedExceptions to not rely on the default value?

Changed to enhancedExceptions

Avasil · 2020-11-07T10:26:47Z

core/jvm/src/main/scala/monix/bio/internal/TaskRunSyncUnsafe.scala

@@ -102,6 +118,12 @@ private[bio] object TaskRunSyncUnsafe {
          }

        case Error(error) =>
+          if (isStackTracing && enhancedExceptions) {


We should add it to case Termination too

Avasil · 2020-11-07T10:28:22Z

core/jvm/src/main/scala/monix/bio/internal/TaskRunSyncUnsafe.scala

@@ -102,6 +118,12 @@ private[bio] object TaskRunSyncUnsafe {
          }

        case Error(error) =>
+          if (isStackTracing && enhancedExceptions) {
+            if (tracingCtx eq null) tracingCtx = new StackTracedContext
+            if (error.isInstanceOf[Throwable]) {


E is always Throwable in runSyncUnsafe so we could get rid of this check

If I remove the check, unit tests fail with cast exceptions.

Avasil · 2020-11-07T10:31:09Z

core/shared/src/main/scala/monix/bio/IO.scala


  /** Lifts a value into the task context. Alias for [[now]]. */
  def pure[A](a: A): UIO[A] = now(a)

  /** Returns a task that on execution is always finishing in error
    * emitting the specified value in a typed error channel.
    */
-  def raiseError[E](ex: E): IO[E, Nothing] =
-    Error(ex)
+  def raiseError[E](ex: E): IO[E, Nothing] = {


I think we should also do a similar thing for Termination

Avasil · 2020-11-07T10:31:24Z

core/shared/src/main/scala/monix/bio/IO.scala

@@ -2925,8 +2975,11 @@ object IO extends TaskInstancesLevel0 {
    TaskFromFuture.deferAction(f)

  /** Alias for [[defer]]. */
-  def suspend[A](fa: => Task[A]): Task[A] =
-    Suspend(fa _)
+  def suspend[A](fa: => Task[A]): Task[A] = {


I think we should also do a similar thing for SuspendTotal

Avasil · 2020-11-07T10:41:10Z

core/shared/src/main/scala/monix/bio/internal/TaskRunLoop.scala

+import monix.bio.internal.TracingPlatform.{enhancedExceptions, isStackTracing}
+import monix.bio.tracing.{TaskEvent, TaskTrace}
+
+import scala.reflect.NameTransformer

 private[bio] object TaskRunLoop {


we have to augment exceptions in Termination case as well

Avasil · 2020-11-07T10:42:12Z

core/shared/src/main/scala/monix/bio/internal/TaskTracing.scala

+/**
+  * All Credits to https://github.com/typelevel/cats-effect and https://github.com/RaasAhsan
+  */
+private[bio] object TaskTracing {


I would rename it to IOTracing, I plan to gradually change Task names in internal classes

Avasil · 2020-11-07T10:43:28Z

core/shared/src/main/scala/monix/bio/tracing/TaskEvent.scala

+/**
+  * All Credits to https://github.com/typelevel/cats-effect and https://github.com/RaasAhsan
+  */
+sealed abstract class TaskEvent


This one is public so it should be definitely IOEvent

Avasil · 2020-11-07T10:43:42Z

core/shared/src/main/scala/monix/bio/tracing/TaskTrace.scala

+/**
+  * All Credits to https://github.com/typelevel/cats-effect and https://github.com/RaasAhsan
+  */
+final case class TaskTrace(events: List[TaskEvent], captured: Int, omitted: Int) {


Avasil · 2020-11-07T10:46:11Z

tracingTests/src/fulltracing/scala/tracing/FullStackTracingSuite.scala

+/**
+  * All Credits to https://github.com/typelevel/cats-effect and https://github.com/RaasAhsan
+  */
+object FullStackTracingSuite extends BaseTestSuite {


Once we add traces in evalTotal / suspendTotal / terminate then it would be good to add some tests for them

paualarco · 2020-11-07T12:33:09Z

core/shared/src/main/scala/monix/bio/IO.scala

+      TaskTracing.uncached()
+    } else {
+      null
+    }


this if-else conditions repeat over different operators, wouldn't be good idea to encapsulate it in a function getTrace?

Avasil

Thank you once again!

Fantastic job, I will make it my priority to release it within a week or so :)

WesselVS added 2 commits October 31, 2020 20:08

ported traces from monix

b8f7b4b

update mima filters

e71b1b4

Avasil requested changes Nov 7, 2020

View reviewed changes

paualarco reviewed Nov 7, 2020

View reviewed changes

WesselVS added 3 commits November 7, 2020 16:22

processed some feedback

9d3668f

added tests

a27a39c

update monix version

0fe5235

WesselVS requested a review from Avasil November 8, 2020 08:44

Avasil approved these changes Nov 10, 2020

View reviewed changes

Avasil merged commit 185193f into monix:master Nov 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ported improved stack traces from monix #190

Ported improved stack traces from monix #190

WesselVS commented Oct 31, 2020 •

edited

Loading

Avasil left a comment

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

Avasil Nov 7, 2020

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

Avasil Nov 7, 2020

WesselVS Nov 8, 2020

paualarco Nov 7, 2020 •

edited

Loading

WesselVS Nov 8, 2020

Avasil left a comment

Ported improved stack traces from monix #190

Ported improved stack traces from monix #190

Conversation

WesselVS commented Oct 31, 2020 • edited Loading

Avasil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paualarco Nov 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avasil left a comment

Choose a reason for hiding this comment

WesselVS commented Oct 31, 2020 •

edited

Loading

paualarco Nov 7, 2020 •

edited

Loading