Use `UUIDGen` instead of `randomUUID()` #678

armanbilge · 2022-08-20T04:33:08Z

Fixes #677.

armanbilge · 2022-08-20T04:34:13Z

build.sbt

-  .jsSettings(
-    // https://www.scala-js.org/news/2022/04/04/announcing-scalajs-1.10.0#fixes-with-compatibility-concerns
-    libraryDependencies += ("org.scala-js" %%% "scalajs-java-securerandom" % "1.0.0")
-      .cross(CrossVersion.for3Use2_13)
-  )


Although in theory this dependency is secure, in practice it is not safe to rely on. See discussion in:

Securely implement UUIDGen for Scala.js cats-effect#2945

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

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

@@ -52,12 +53,26 @@ object PagingSelfAwareStructuredLogger {
   * @return
   *   SelfAwareStructuredLogger with pagination.
   */
-  def withPaging[F[_]: Monad](pageSizeK: Int = 64, maxPageNeeded: Int = 999)(
+  def withPaging2[F[_]: Monad: UUIDGen](pageSizeK: Int = 64, maxPageNeeded: Int = 999)(


Sorry, I wasn't feeling creative with naming at this moment 😅 I will happily change this to any suggestion, thanks!

Maybe we should name it paged? 🤔 and just keep that as the future API

Works for me! :)

armanbilge · 2022-08-20T04:36:41Z

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

+    new PagingSelfAwareStructuredLogger[F](
+      pageSizeK,
+      maxPageNeeded,
+      Monad[F].unit.map(_ => UUID.randomUUID())


Terrible, right! But this just more clearly exposes the bad behavior that already existed here. There is no safe way to generate random UUIDs with only a Monad.

Absolutely! I'm just sorry I didn't catch it before

lorandszakacs · 2022-08-20T07:01:57Z

LGTM! I'm fine with merging this. Leaving it open for other maintainers to weigh in.

danicheg · 2022-08-22T08:09:32Z

build.sbt

@@ -46,18 +46,14 @@ lazy val core = crossProject(JSPlatform, JVMPlatform)
  .settings(
    name := "log4cats-core",
    libraryDependencies ++= Seq(
-      "org.typelevel" %%% "cats-core" % catsV
+      "org.typelevel" %%% "cats-core"       % catsV,
+      "org.typelevel" %%% "cats-effect-std" % catsEffectV


Just to keep the initial goal of having the core module CE-less, can we introduce an internal interface for UUIDGen and its implementation within the slf4j module?

Can we instead create a new module that is explicitely dependent on CE? And move the correct version of this there?

I feel like re-defining a UUIDGen equivalent leads to a lot of confusion and bad UX. Instead, having a log4cats-core package that is honest about its limitations, and an extension logs4cats-ce (naming is hard) that overcomes aforementioned limitations might be better.

Although not entirely sure how to pull this off in a bincompat and user friendly way.

I agree, adding a "log4cats-effect" would be better for UX, but still not great for UX+bincompat.

Just so that we are clear: what are the arguments against just have a CE dependency in the core module?

what are the arguments against just have a CE dependency in the core module?

Very good question! I personally don't know since I probably missed those discussions during my absence. 🙈 Personally I think it's ok to have CE in core

I can only guess, but perhaps the reason is to apply the Rule of Least Power. But once again, I'm not sure if this is a thing still.

I would say it is a thing still :) The problem is the repo is currently applying the principle of too-little-power. This is a course-correction.

armanbilge · 2022-09-09T22:42:51Z

@danicheg so wdyt, can we move forward with adding CE to core?

If not, my best suggestion is to:

Create a "log4cats-effect" module.
Completely deprecate log4cats.PagingSelfAwareStructuredLogger as blocking and unfixable.
Re-introduce it as log4cats.effect.PagingSelfAwareStructuredLogger or similar.

danicheg · 2022-09-10T14:22:08Z

To sum up my thoughts:
I haven't a strict 'no' about having CE in the core module.
I was wondering – is it possible to preserve the initial goal of not having CE in the core module? Perhaps the only reason why this could be still a thing – is the principle of least power. That said if we will benefit from adding this dependency, then probably, we are good.
It'd be extremely helpful if the initial architects of log4cats would have a glance at this one. But based on common sense, I don't see much of the evil in adding CE to the core module.

armanbilge · 2022-09-10T15:08:02Z

I spoke with @ChristopherDavenport on Discord. Here's a quick summary:

Regarding CE in core:

Having cats-effect to in core I don't mind though. The idea was a bunch of abstractions and helpers there, with the effects of them in the helper modules. But... If we need the std to have cats-effect for those helpers. I'm fine with that.
Basically, core is like http4s core. (I reuse concepts)

Regarding UUIDs:

It's to identify all the split logs as a single log action.
So it's really just a string.
So I'm not seeing why it wouldn't be F[String] and let people define their own way, one of which could be via UUID and UuidGen

So it boils down to a question of UX:

We can keep core on Cats only, and force users to bring their own F[String] since we cannot provide a default. This makes it harder for users to fix the deprecation.
We can move ahead with this PR, which adds Cats Effect, and provides a default F[String] via UUIDGen[F]. Then in a follow-up we can make this customizable (or I can do it in this PR if it's easy).

Thoughts?

@danicheg thanks again for raising this issue, it's opened some good questions.

danicheg · 2022-09-11T08:18:48Z

It comes that all of us don't mind adding the CE std module to the log4cats core. So let's move onward with this. 👍 from me to that.

Use UUIDGen instead of randomUUID()

4c2c5e5

armanbilge commented Aug 20, 2022

View reviewed changes

armanbilge added 3 commits August 20, 2022 04:37

Formatting

dcd9203

Bump base version

48841c9

Bincompat

fefa49d

withPaging2 -> paged

eb768c0

lorandszakacs approved these changes Aug 20, 2022

View reviewed changes

lorandszakacs mentioned this pull request Aug 20, 2022

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

Open

danicheg reviewed Aug 22, 2022

View reviewed changes

armanbilge mentioned this pull request Sep 9, 2022

Cross-build for Native #687

Merged

ChristopherDavenport merged commit eb768c0 into typelevel:main Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `UUIDGen` instead of `randomUUID()` #678

Use `UUIDGen` instead of `randomUUID()` #678

armanbilge commented Aug 20, 2022

armanbilge Aug 20, 2022 •

edited

Loading

armanbilge Aug 20, 2022

lorandszakacs Aug 20, 2022

armanbilge Aug 20, 2022

armanbilge Aug 20, 2022

lorandszakacs Aug 20, 2022

lorandszakacs commented Aug 20, 2022

danicheg Aug 22, 2022

lorandszakacs Aug 29, 2022 •

edited

Loading

armanbilge Aug 29, 2022

lorandszakacs Aug 30, 2022

danicheg Aug 30, 2022

armanbilge Aug 30, 2022

armanbilge commented Sep 9, 2022

danicheg commented Sep 10, 2022 •

edited

Loading

armanbilge commented Sep 10, 2022 •

edited

Loading

danicheg commented Sep 11, 2022

Use UUIDGen instead of randomUUID() #678

Use UUIDGen instead of randomUUID() #678

Conversation

armanbilge commented Aug 20, 2022

armanbilge Aug 20, 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

lorandszakacs commented Aug 20, 2022

Choose a reason for hiding this comment

lorandszakacs Aug 29, 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

armanbilge commented Sep 9, 2022

danicheg commented Sep 10, 2022 • edited Loading

armanbilge commented Sep 10, 2022 • edited Loading

danicheg commented Sep 11, 2022

Use `UUIDGen` instead of `randomUUID()` #678

Use `UUIDGen` instead of `randomUUID()` #678

armanbilge Aug 20, 2022 •

edited

Loading

lorandszakacs Aug 29, 2022 •

edited

Loading

danicheg commented Sep 10, 2022 •

edited

Loading

armanbilge commented Sep 10, 2022 •

edited

Loading