Add helpers to construct `Trace` backed by `IOLocal` #729

mrdziuban · 2022-04-11T12:51:35Z

Following up on #723, this adds a new cats-effect module with ioLocalTrace helper methods for constructing a Trace[IO] backed by an IOLocal[Span[IO]]

janstenpickle · 2022-04-13T19:14:08Z

Thanks for this @mrdziuban!

Last time I looked at this, the IOLocal instance had to be shared by all threads using Trace. If that is the case the parent span could be overwritten by any thread using the Trace instance. Have you tried this in a multi-threaded app?

Also for this to work properly with the Http4s module, you need to have a Provide instance.

ybasket · 2022-04-13T21:47:33Z

Thanks for this @mrdziuban!

Yes, absolutely love this PR and the fact someone else is excited about this possibility. I have played with IOLocal for tracing recently, glad to see someone invested the time to make a proper PR out of it! I didn't have the time for that until now. Tested your changes locally and implemented a bit more on top (hope you don't mind me sharing it here), see https://github.com/ybasket/trace4cats/commits/trace-iolocal – explanations below. Feel free to copy over whatever you like, I'll send a PR with the leftovers once this one is merged.

Last time I looked at this, the IOLocal instance had to be shared by all threads using Trace. If that is the case the parent span could be overwritten by any thread using the Trace instance. Have you tried this in a multi-threaded app?

IOLocal works per CE Fiber, so if you have things running in parallel, they will all only modify their own copy (created on first modification within that fiber), so ideal for tracing. Natchez has an implementation already. The only artefact you have is a root span that lives for as long as your Trace instance lives. But this can be mitigated by a special kind of span, see the DischargeSpan commit on my branch. It's a PoC only, its ergonomics can probably be optimised, but it works and is remotely inspired by https://github.com/armanbilge/bayou. I tested the combination of IOLocal tracing from this PR with this special span on two linked http4s services of mine and a dummy Fibonacci app that does use massive parallelism, Jaeger showed all spans as expected.

Also for this to work properly with the Http4s module, you need to have a Provide instance.

I implemented a Provide instance and also instances for an IOLocal[Ctx] where Ctx has a Lens to a Span[IO] on my branch.

janstenpickle · 2022-04-14T07:21:33Z

Thanks for the explanation @ybasket! All we should need is a Provide instance on Ctx then, with helpers providing Lens

mrdziuban · 2022-04-14T12:45:03Z

Thanks all for the feedback, and thank you @ybasket for the enhancements! I've just added your commits here. @janstenpickle there were a couple Provide instances in @ybasket's work, do either of them match what you're thinking of? If not, I'm happy to add them, just would need a bit more guidance on the exact types to target.

build.sbt

modules/base-laws/src/test/scala/io/janstenpickle/trace4cats/base/context/laws/LiftTests.scala

modules/base/src/main/scala/io/janstenpickle/trace4cats/base/context/Lift.scala

...les/base-laws/src/test/scala/io/janstenpickle/trace4cats/base/context/laws/UnliftTests.scala

modules/base/src/main/scala/io/janstenpickle/trace4cats/base/context/Lift.scala

modules/base/src/main/scala/io/janstenpickle/trace4cats/base/context/Unlift.scala

modules/cats-effect/src/main/scala/io/janstenpickle/trace4cats/cats/effect/DischargeSpan.scala

...les/cats-effect/src/main/scala/io/janstenpickle/trace4cats/cats/effect/LocalForIOLocal.scala

modules/cats-effect/src/main/scala/io/janstenpickle/trace4cats/cats/effect/package.scala

catostrophe · 2022-04-14T19:02:58Z

Sorry for short and shallow comments. I will return later with concrete suggestions.

mrdziuban · 2022-04-14T19:16:04Z

No problem! I may not be super active or responsive the next few days but will keep my eye on this and add relevant changes (including the context bound changes) next week

catostrophe · 2022-04-16T08:29:03Z

@mrdziuban I rebased your branch on master and added 2 new commits with fixes to base(-laws) and cio modules.

The review is not finished though as I have a few questions for @ybasket and want to try the IOLocal instances in different use-cases to make sure it compiles and works as expected.

catostrophe

I left some comments for @mrdziuban and @ybasket

catostrophe · 2022-04-16T08:32:38Z

modules/base-laws/src/test/scala/io/janstenpickle/trace4cats/base/context/laws/IdTests.scala

+
+class IdTests extends BaseSuite {
+  implicit val cogenOption2Option: Cogen[Option ~> Option] =
+    Cogen(k => k(Some(0)).size.toLong)


@mrdziuban I changed this line as there may be 2 instances of Option ~> Option

catostrophe · 2022-04-16T08:33:33Z

modules/base-laws/src/test/scala/io/janstenpickle/trace4cats/base/context/laws/IdTests.scala

+import io.janstenpickle.trace4cats.base.context.laws.discipline.UnliftTests
+import org.scalacheck.Cogen
+
+class IdTests extends BaseSuite {


I renamed the file as it tests laws for F <~> F which we call identity.

catostrophe · 2022-04-16T08:34:35Z

modules/base-laws/src/test/scala/io/janstenpickle/trace4cats/base/context/laws/IdTests.scala

+  implicit val cogenOption2Option: Cogen[Option ~> Option] =
+    Cogen(k => k(Some(0)).size.toLong)
+
+  checkAll("Option <~> Option", UnliftTests[Option, Option].unlift[String, Int])


@mrdziuban UnliftTests extends LiftTests, so we need only 1 file and 1 test invokation, not two.

catostrophe · 2022-04-16T08:36:51Z

modules/base/src/main/scala/io/janstenpickle/trace4cats/base/context/ContextRoot.scala

      def provide[A](fa: Kleisli[F, R, A])(r: R): F[A] = fa.run(r)
    }
+
+  implicit def idInstance[F[_]: Monad]: Unlift[F, F] = new Unlift[F, F] {


@mrdziuban ContextRoot is the root trait for the whole hierarchy, so it is best to put instances in its companion - it will be summonable for any descendant typeclass.

catostrophe · 2022-04-16T08:37:42Z

modules/base/src/test/scala/io/janstenpickle/trace4cats/base/context/IdInstanceSummonTest.scala

@@ -0,0 +1,9 @@
+package io.janstenpickle.trace4cats.base.context
+
+object IdInstanceSummonTest {


@mrdziuban Summon tests - we just check that our instance is discoverable by the compiler

catostrophe · 2022-04-16T08:38:49Z

...ats-effect-io/src/main/scala/io/janstenpickle/trace4cats/cats/effect/ProvideForIOLocal.scala

+import cats.effect.{IO, IOLocal}
+import io.janstenpickle.trace4cats.base.context.Provide
+
+private[trace4cats] class ProvideForIOLocal[Ctx](rootCtx: IOLocal[Ctx]) extends Provide[IO, IO, Ctx] {


@mrdziuban We need just one implementation, no more need for IdLift/IdUnlift mixins.

catostrophe · 2022-04-16T08:40:21Z

...ats-effect-io/src/main/scala/io/janstenpickle/trace4cats/cats/effect/ProvideForIOLocal.scala

+
+  def lift[A](la: IO[A]): IO[A] = la
+  def ask[R1 >: Ctx]: IO[R1] = rootCtx.get
+  def local[A](fa: IO[A])(f: Ctx => Ctx): IO[A] = rootCtx.get.flatMap { parent =>


@mrdziuban This instance seems fine but it must be tested against ProvideLaws. So we need tests just as we have them in the zio module.

catostrophe · 2022-04-16T08:41:50Z

modules/cats-effect-io/src/main/scala/io/janstenpickle/trace4cats/cats/effect/package.scala

+import io.janstenpickle.trace4cats.base.optics.Lens
+import io.janstenpickle.trace4cats.inject.Trace
+
+package object effect {


@mrdziuban I simplified this file as I could. Still need time to check how these instances are usable

...es/cats-effect-io/src/main/scala/io/janstenpickle/trace4cats/cats/effect/DischargeSpan.scala

catostrophe · 2022-04-16T08:51:57Z

modules/base/src/main/scala/io/janstenpickle/trace4cats/base/context/Ask.scala


  def zoom[R1](g: Getter[R, R1]): Ask[F, R1] = new Ask[F, R1] {
-    def F: Monad[F] = self.F
+    val F: Monad[F] = self.F


Ignore all these def to val changes. It's my old debt.

catostrophe · 2022-04-18T12:24:28Z

modules/cats-effect-io/src/main/scala/io/janstenpickle/trace4cats/cats/effect/package.scala

+import io.janstenpickle.trace4cats.base.optics.Lens
+import io.janstenpickle.trace4cats.inject.Trace
+
+package object effect {


@ybasket please double check my changes here. I hope I didn't break anything

catostrophe · 2022-04-18T12:26:21Z

I think I'll be ready to merge this PR when we have law tests for the Provide instance. Does anyone want to add them? I'll have some spare time in a few days only.

ybasket · 2022-04-19T08:05:44Z

I think I'll be ready to merge this PR when we have law tests for the Provide instance. Does anyone want to add them? I'll have some spare time in a few days only.

@catostrophe I tried to add them in ybasket@330fa65. The test passes, but I'm afraid it's at least partially non-sensical. I had to implement an instance of Eq[IO[IO ~> IO]], a type for which I don't see any reasonable meaning nor implementation. As a placeholder, I always return false (true also works) – that hints towards not being used well. I'm no expert in law testing, wdyt could be done here? Extract the one law that requires the nonsensical instance and not check it for IOLocal?

catostrophe · 2022-04-20T09:16:41Z

@ybasket hey! You should have got an ability to push to contributors fork branches. Please check and push your commits straight to this PR. I will review them a bit later and apply fix if any needed.

ybasket · 2022-04-20T09:33:20Z

@ybasket hey! You should have got an ability to push to contributors fork branches. Please check and push your commits straight to this PR. I will review them a bit later and apply fix if any needed.

Oh, thank you for the honour 😊
Just pushed the commit.

catostrophe · 2022-04-24T13:48:12Z

Will review it tonight. Sorry for the delay

Instances of Provide and a Trace instance for an IOLocal that contains a context from which a Span[IO] can be extracted.

DischargeSpan is a root span that spans new (non-discharge) root spans as children. Useful for situations where you have to construct resources in a traced type and need to "discharge" the tracing without having all children being linked to this long-lived artifact span.

By using Lens.focus, both classes can be merged into one.

1. Move one single instance to ContextRoot 2. Remove IdLift, IdUnlift traits 3. Add summon tests for idInstance 4. Fix law tests for idInstance 5. Override defs with vals where needed

1. Change bounds on F in DischargeSpan 2. Replace LocalForIOLocal with ProvideForIOLocal 3. Simplify instance providers in package object

Provide a law test for Provide on IOLocal – somewhat dummy implementation as not all signatures make sense for IO. Test passes, but its value is questionable.

catostrophe · 2022-04-25T12:09:52Z

I refactored the module. I did it in the same way as we have it in the zio integration.

Still no law tests. Still working on it.

...-io/src/main/scala/io/janstenpickle/trace4cats/base/context/io/IOLocalContextInstances.scala

modules/core/src/main/scala/io/janstenpickle/trace4cats/Span.scala

...s/inject-io/src/main/scala/io/janstenpickle/trace4cats/inject/io/IOLocalTraceInstances.scala

catostrophe · 2022-04-25T12:17:52Z

modules/inject-io/src/main/scala/io/janstenpickle/trace4cats/inject/io/package.scala

+
+import _root_.io.janstenpickle.trace4cats.base.context.io.IOLocalContextInstances
+
+package object io extends IOLocalTraceInstances with IOLocalContextInstances


The usage will be as follows:

package my.app import cats.effect.{IO, IOApp, IOLocal} import io.janstenpickle.trace4cats.Span import io.janstenpickle.trace4cats.base.context.Provide import io.janstenpickle.trace4cats.inject.{EntryPoint, Trace} import io.janstenpickle.trace4cats.kernel.{SpanCompleter, SpanSampler} // this imports both provide and trace instances for IOLocal import io.janstenpickle.trace4cats.inject.io._ object Test extends IOApp.Simple { def run: IO[Unit] = { val spanCompleter = SpanCompleter.empty[IO] Span.discharge(SpanSampler.always[IO], spanCompleter).flatMap { span => IOLocal(span).map { ioLocal => implicit val trace: Trace[IO] = ioLocalTrace(ioLocal) implicit val provide: Provide[IO, IO, Span[IO]] = ioLocalProvide(ioLocal) Fibonacci(EntryPoint[IO](SpanSampler.always[IO], spanCompleter)) } } } }

Looks good, I'll try it on the demo repo later!

Once this PR is merged I'd like to experiment with adding some helper for this as good parts of this snippet will always be the same, so we can make user code nicer by introducing some IOLocalHelper (better name to be found) that creates the discharge span and the IOLocal and returns type class instances ready to use. Especially on Scala 3 (context functions), this could simplify usage noticeably.

@ybasket We can include a syntax extension for Span.type. So it may look as:

val x = Span.dischargeIOLocal(sampler, completer) : IO[IOLocal[Span[IO]]

If you don't mind, let's put it in a separate PR? I just want to release something we can play with.

Yes, definitely follow-up PR(s) for anything that's not in here yet.

catostrophe · 2022-04-26T05:37:59Z

I improved tests a bit (by adding more sensical arb instances), but I still can't get why Eq[IO ~> IO] works. Been busy at $WORK and can't dig deeper atm. Let's keep it as is and return to it later.

catostrophe requested review from catostrophe and janstenpickle April 11, 2022 19:59

catostrophe added the enhancement New feature or request label Apr 11, 2022

catostrophe requested changes Apr 14, 2022

View reviewed changes

catostrophe force-pushed the trace-iolocal branch 2 times, most recently from 7b13b5a to eca8723 Compare April 16, 2022 08:26

catostrophe reviewed Apr 16, 2022

View reviewed changes

catostrophe marked this pull request as draft April 18, 2022 07:00

catostrophe reviewed Apr 18, 2022

View reviewed changes

ybasket mentioned this pull request Apr 24, 2022

Improve derivation for TraceId.Gen & SpanId.Gen #736

Merged

mrdziuban and others added 7 commits April 25, 2022 11:48

Add Lift[F, F] and Unlift[F, F] instances.

7ae9fb4

Add cats-effect module with Trace backed by IOLocal.

4ba87b7

Update .github/workflows/ci.yml

686c2f7

Add more instances for IOLocal

e3d8e2f

Instances of Provide and a Trace instance for an IOLocal that contains a context from which a Span[IO] can be extracted.

Rename cats-effect module to cats-effect-io.

089beae

Make Local instances for IOLocal more DRY

3c7fdc0

By using Lens.focus, both classes can be merged into one.

mrdziuban and others added 4 commits April 25, 2022 11:48

Update .github/workflows/ci.yml

9b8ae14

fix: rework idInstance for Unlift

11bd595

1. Move one single instance to ContextRoot 2. Remove IdLift, IdUnlift traits 3. Add summon tests for idInstance 4. Fix law tests for idInstance 5. Override defs with vals where needed

fix: rework cats-effect-io module

5785ede

1. Change bounds on F in DischargeSpan 2. Replace LocalForIOLocal with ProvideForIOLocal 3. Simplify instance providers in package object

Add test for Provide laws (IOLocal)

8be0ba4

Provide a law test for Provide on IOLocal – somewhat dummy implementation as not all signatures make sense for IO. Test passes, but its value is questionable.

catostrophe force-pushed the trace-iolocal branch 2 times, most recently from d768950 to c34da14 Compare April 25, 2022 12:02

rearrange modules/files for IOLocal integration

784b7b3

catostrophe force-pushed the trace-iolocal branch from c34da14 to 784b7b3 Compare April 25, 2022 12:06

catostrophe reviewed Apr 25, 2022

View reviewed changes

ybasket mentioned this pull request Apr 25, 2022

Bug: Duplicate span IDs #740

Closed

add tests for Provide via IOLocal

dbc213f

catostrophe marked this pull request as ready for review April 26, 2022 05:38

catostrophe requested review from catostrophe and ybasket April 26, 2022 06:00

catostrophe approved these changes Apr 26, 2022

View reviewed changes

ybasket approved these changes Apr 26, 2022

View reviewed changes

catostrophe merged commit 973c327 into trace4cats:master Apr 26, 2022

mrdziuban deleted the trace-iolocal branch September 23, 2022 14:37

		@@ -0,0 +1,9 @@
		package io.janstenpickle.trace4cats.base.context

		object IdInstanceSummonTest {


		import _root_.io.janstenpickle.trace4cats.base.context.io.IOLocalContextInstances

		package object io extends IOLocalTraceInstances with IOLocalContextInstances

Add helpers to construct Trace backed by IOLocal #729

Add helpers to construct Trace backed by IOLocal #729

Uh oh!

Conversation

mrdziuban commented Apr 11, 2022

Uh oh!

janstenpickle commented Apr 13, 2022

Uh oh!

ybasket commented Apr 13, 2022

Uh oh!

janstenpickle commented Apr 14, 2022

Uh oh!

mrdziuban commented Apr 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

catostrophe commented Apr 14, 2022

Uh oh!

mrdziuban commented Apr 14, 2022

Uh oh!

catostrophe commented Apr 16, 2022

Uh oh!

catostrophe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catostrophe commented Apr 18, 2022

Uh oh!

ybasket commented Apr 19, 2022

Uh oh!

catostrophe commented Apr 20, 2022

Uh oh!

ybasket commented Apr 20, 2022

Uh oh!

catostrophe commented Apr 24, 2022

Uh oh!

catostrophe commented Apr 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catostrophe commented Apr 26, 2022

Add helpers to construct `Trace` backed by `IOLocal` #729

Add helpers to construct `Trace` backed by `IOLocal` #729