Add some helpers for using Kleisli or IOLocal for logging context #676

wunderk1nd-e · 2022-08-17T12:51:55Z

Add helpers for logging with some propagated ctx and kleisli implementation
Add ce3 module and iolocal instances

kubukoz · 2022-08-17T14:31:42Z

build.sbt

+lazy val ce3 = crossProject(JSPlatform, JVMPlatform)
+  .settings(commonSettings)
+  .dependsOn(core)
+  .settings(
+    name := "log4cats-ce3",
+    libraryDependencies ++= Seq(
+      "org.typelevel" %%% "cats-effect" % catsEffectV
+    )
+  )
+


~~doesn't core already depend on CE3?~~ oh, it doesn't. cool

Probably we can move this to the slf4j module to don't create a new module. Speaking from a user experience it'd be handier - one wouldn't need to add a new dependency. WDYT?

Happy to go with whatever you guys think is most appropriate - just didn't add it to either of the existing ones because:

Core doesn't depend on CE3

Isn't strictly speaking an slf4j concern

With #678 we will be depending on cats-effect, and I see no other way to resolve the UUIDGen issue otherwise.

We'll have to track how the dependency on CE in core goes there as well.

Now it depends on cats-effect-std. This still introduces the IO dependency.

kubukoz · 2022-08-17T14:32:46Z

core/shared/src/main/scala/org/typelevel/log4cats/LoggerFactory.scala

@@ -48,4 +48,20 @@ object LoggerFactory extends LoggerFactoryGenCompanion {
        fk(logger)
      }
    }
+


I somehow missed that LoggerFactory was a thing - nice!

kubukoz · 2022-08-17T14:36:16Z

core/shared/src/main/scala/org/typelevel/log4cats/SelfAwareStructuredLogger.scala

+  def withContextF[F[_]: FlatMap](
+      sl: SelfAwareStructuredLogger[F]
+  )(ctx: F[Map[String, String]]): SelfAwareStructuredLogger[F] =
+    new ModifiedContextFSelfAwareStructuredLogger[F](sl)(existingCtx => ctx.map(_ ++ existingCtx))


I think what we're missing is something like this:

def fa: F[A] def faWithContext: F[A] = StructuredLogger[F].locally(Map("k" -> "v"))(fa)

so that you can wrap an effect, not just a logger. That would help propagate the context to whatever services you're calling, without a need to pass the context or the updated logger.

Hmm, I guess I was thinking of users doing that using Kleisli.local or similar for IOLocal.
e.g.

def scope[G, T](local: IOLocal[T], value: T)(task: IO[G]): IO[G] = local.getAndSet(value).bracket(_ => task)(local.set)

How are you imagining something like StructuredLogger[F].locally would work?
Just specific implementations for where there's a IOLocal or Kleisli around?

yeah, I think we'll need concrete implementations

I have come to believe that almost all uses of IOLocal should have that cats.mtl.Local shape. See typelevel/cats-effect#3385. And I like the scope name: that's what Local calls it.

ce3/shared/src/main/scala/org/typelevel/log4cats/ce3/IOLocalHelpers.scala

armanbilge · 2022-08-20T04:16:47Z

Instead of adding a CE3 module I think we should just add a CE3 dependency to the core module. CE2 is officially sunset and there's already blocking footguns in core that can only be fixed with CE3.

randomUUID is blocking #677

armanbilge · 2022-09-10T15:26:00Z

So I had a discussion with @ChristopherDavenport on Discord wrt #677. Although he is supportive of adding a CE dependency to core, he still thinks anything IOLocal-based should be in a separate module.

I would expect that should be in a different module.
Not core
Because it's not a way I think folks expect, and the way to do it would be unsafe almost certainly.
Since they will globalize.
IOLocal is overspecified. And FiberLocal is a side library.
So it should be a module or repo with FiberLocal which is a better abstraction then forcing everything to IO

rossabaker · 2023-01-04T04:00:33Z

So the cats-effect-std dependency landed in core, but IOLocal would require the full cats-effect. Doing that would pull cats-effect (and IO) into all the downstream projects that limited themselves to cats-effect-std. That's a pretty good argument for a separate module.

…tation

wunderk1nd-e · 2023-01-23T16:02:33Z

So the cats-effect-std dependency landed in core, but IOLocal would require the full cats-effect. Doing that would pull cats-effect (and IO) into all the downstream projects that limited themselves to cats-effect-std. That's a pretty good argument for a separate module.

I've updated the branch with main - any chance you could approve the workflow run? 🙏

rossabaker

I apologize for losing track of this one... I approved the CI run and resolved a merge conflict.

rossabaker · 2023-02-20T02:35:43Z

build.sbt

+lazy val ce3 = crossProject(JSPlatform, JVMPlatform)
+  .settings(commonSettings)
+  .dependsOn(core)
+  .settings(
+    name := "log4cats-ce3",
+    libraryDependencies ++= Seq(
+      "org.typelevel" %%% "cats-effect" % catsEffectV
+    )
+  )
+


Now it depends on cats-effect-std. This still introduces the IO dependency.

rossabaker · 2023-02-20T02:36:50Z

build.sbt

+  .settings(commonSettings)
+  .dependsOn(core)
+  .settings(
+    name := "log4cats-ce3",


I would call this log4cats-cats-effect. Anything beyond 3 will be a breaking change, and I'd avoid colloquial abbrevations in a jar name.

rossabaker · 2023-02-20T02:42:57Z

core/shared/src/main/scala/org/typelevel/log4cats/SelfAwareStructuredLogger.scala

+  def withContextF[F[_]: FlatMap](
+      sl: SelfAwareStructuredLogger[F]
+  )(ctx: F[Map[String, String]]): SelfAwareStructuredLogger[F] =
+    new ModifiedContextFSelfAwareStructuredLogger[F](sl)(existingCtx => ctx.map(_ ++ existingCtx))


I have come to believe that almost all uses of IOLocal should have that cats.mtl.Local shape. See typelevel/cats-effect#3385. And I like the scope name: that's what Local calls it.

armanbilge · 2023-02-20T02:47:39Z

I have come to believe that almost all uses of IOLocal should have that cats.mtl.Local shape. See typelevel/cats-effect#3385. And I like the scope name: that's what Local calls it.

Should this PR then be in terms of Local, instead of offering specific instances?

rossabaker · 2023-02-20T02:56:10Z

I would have thought so, until cats-mtl's future stability was questioned on this pull request. The uncertainty around that statement is blocking a cats-effect PR, a clear path forward on otel4s, and now this.

armanbilge · 2023-02-20T02:59:25Z

Yeah. I know it's annoying, but might be worth pausing on this one as well until that's decided. We already have a CE dependency in log4cats core, and CE is already (technically) tied to MTL versioning, and thus so are we.

rossabaker · 2023-02-20T03:52:31Z

If @wunderk1nd-e isn't desperate for a release on that, I think that's a sound strategy. The fact everything is being done twice is because there's a lovely type class for this if we'd accept it as a routine dependency.

mattyjbrown · 2023-03-01T15:12:59Z

Is it worth it/feasible to add a plain and simple LoggerFactory.withContext, as a way of propagating context when you're passing around LoggerFactorys, or is that going to conflict the eventual implementation of this PR?

wunderk1nd-e changed the title ~~ctxtual~~ Add some helpers for using Kleisli or IOLocal for logging context Aug 17, 2022

wunderk1nd-e mentioned this pull request Aug 17, 2022

FiberLocal / IOLocal logger wrapper #461

Open

kubukoz reviewed Aug 17, 2022

View reviewed changes

bplommer reviewed Aug 18, 2022

View reviewed changes

ce3/shared/src/main/scala/org/typelevel/log4cats/ce3/IOLocalHelpers.scala Outdated Show resolved Hide resolved

wunderk1nd-e mentioned this pull request Aug 23, 2022

Add scope to IOLocal typelevel/cats-effect#3128

Closed

wunderk1nd-e added 2 commits January 23, 2023 15:54

Add helpers for logging with some propagated ctx and kleisli implemen…

8709392

…tation

Add ce3 module and iolocal instances

d8f1164

wunderk1nd-e force-pushed the ctxtual branch from aa13836 to d8f1164 Compare January 23, 2023 15:59

wunderk1nd-e marked this pull request as ready for review January 23, 2023 16:01

Merge branch 'main' into ctxtual

d2d340d

rossabaker added this to the 3.6.0 milestone Feb 20, 2023

rossabaker reviewed Feb 20, 2023

View reviewed changes

Regenerate workflow after merge

f0629f3

iRevive mentioned this pull request Oct 26, 2023

Allow adding effect's contextual environment to the log context #800

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some helpers for using Kleisli or IOLocal for logging context #676

Add some helpers for using Kleisli or IOLocal for logging context #676

wunderk1nd-e commented Aug 17, 2022

kubukoz Aug 17, 2022 •

edited

Loading

danicheg Aug 17, 2022

wunderk1nd-e Aug 17, 2022

lorandszakacs Aug 20, 2022

rossabaker Feb 20, 2023

kubukoz Aug 17, 2022

kubukoz Aug 17, 2022

wunderk1nd-e Aug 17, 2022 •

edited

Loading

wunderk1nd-e Aug 17, 2022

kubukoz Aug 17, 2022

rossabaker Feb 20, 2023

armanbilge commented Aug 20, 2022

armanbilge commented Sep 10, 2022

rossabaker commented Jan 4, 2023

wunderk1nd-e commented Jan 23, 2023 •

edited

Loading

rossabaker left a comment

rossabaker Feb 20, 2023

rossabaker Feb 20, 2023

rossabaker Feb 20, 2023

armanbilge commented Feb 20, 2023 •

edited

Loading

rossabaker commented Feb 20, 2023

armanbilge commented Feb 20, 2023

rossabaker commented Feb 20, 2023

mattyjbrown commented Mar 1, 2023

Add some helpers for using Kleisli or IOLocal for logging context #676

Are you sure you want to change the base?

Add some helpers for using Kleisli or IOLocal for logging context #676

Conversation

wunderk1nd-e commented Aug 17, 2022

kubukoz Aug 17, 2022 • edited Loading

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

wunderk1nd-e Aug 17, 2022 • edited Loading

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

armanbilge commented Aug 20, 2022

armanbilge commented Sep 10, 2022

rossabaker commented Jan 4, 2023

wunderk1nd-e commented Jan 23, 2023 • edited Loading

rossabaker 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

armanbilge commented Feb 20, 2023 • edited Loading

rossabaker commented Feb 20, 2023

armanbilge commented Feb 20, 2023

rossabaker commented Feb 20, 2023

mattyjbrown commented Mar 1, 2023

kubukoz Aug 17, 2022 •

edited

Loading

wunderk1nd-e Aug 17, 2022 •

edited

Loading

wunderk1nd-e commented Jan 23, 2023 •

edited

Loading

armanbilge commented Feb 20, 2023 •

edited

Loading