Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use UUIDGen instead of randomUUID() #678

Merged
merged 5 commits into from
Sep 15, 2022
Merged

Use UUIDGen instead of randomUUID() #678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4c2c5e5
Use `UUIDGen` instead of `randomUUID()`
armanbilge Aug 20, 2022
dcd9203
Formatting
armanbilge Aug 20, 2022
48841c9
Bump base version
armanbilge Aug 20, 2022
fefa49d
Bincompat
armanbilge Aug 20, 2022
eb768c0
`withPaging2` -> `paged`
armanbilge Aug 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ val Scala213 = "2.13.8"
val Scala212 = "2.12.16"
val Scala3 = "3.1.3"

ThisBuild / tlBaseVersion := "2.4"
ThisBuild / tlBaseVersion := "2.5"
ThisBuild / crossScalaVersions := Seq(Scala213, Scala212, Scala3)
ThisBuild / scalaVersion := Scala213
ThisBuild / startYear := Some(2018)
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@lorandszakacs lorandszakacs Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

),
libraryDependencies ++= {
if (tlIsScala3.value) Seq.empty
else Seq("org.scala-lang" % "scala-reflect" % scalaVersion.value % Provided)
}
)
.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)
)
Comment on lines -56 to -60
Copy link
Member Author

@armanbilge armanbilge Aug 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


lazy val testing = crossProject(JSPlatform, JVMPlatform)
.settings(commonSettings)
Expand Down
45 changes: 31 additions & 14 deletions core/shared/src/main/scala/org/typelevel/log4cats/PagingSelfAwareStructuredLogger.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
package org.typelevel.log4cats

import cats._
import cats.implicits._
import cats.effect.std.UUIDGen
import cats.syntax.all._

import java.io.{PrintWriter, StringWriter}
import java.util.UUID
Expand Down Expand Up @@ -52,19 +53,35 @@ object PagingSelfAwareStructuredLogger {
* @return
* SelfAwareStructuredLogger with pagination.
*/
def paged[F[_]: Monad: UUIDGen](pageSizeK: Int = 64, maxPageNeeded: Int = 999)(
logger: SelfAwareStructuredLogger[F]
): SelfAwareStructuredLogger[F] =
new PagingSelfAwareStructuredLogger[F](pageSizeK, maxPageNeeded, UUIDGen.randomUUID)(logger)

@deprecated("Use paged", "2.5.0")
def withPaging[F[_]: Monad](pageSizeK: Int = 64, maxPageNeeded: Int = 999)(
logger: SelfAwareStructuredLogger[F]
): SelfAwareStructuredLogger[F] =
new PagingSelfAwareStructuredLogger[F](pageSizeK, maxPageNeeded)(logger)

private class PagingSelfAwareStructuredLogger[F[_]: Monad](pageSizeK: Int, maxPageNeeded: Int)(
private class PagingSelfAwareStructuredLogger[F[_]: Monad](
pageSizeK: Int,
maxPageNeeded: Int,
randomUUID: F[UUID]
)(
sl: SelfAwareStructuredLogger[F]
) extends SelfAwareStructuredLogger[F] {
if (pageSizeK <= 0 || maxPageNeeded <= 0)
throw new IllegalArgumentException(
s"pageSizeK(=$pageSizeK) and maxPageNeeded(=$maxPageNeeded) must be positive numbers."
)

@deprecated("Use constructor with randomUUID", "2.5.0")
def this(pageSizeK: Int, maxPageNeeded: Int)(
sl: SelfAwareStructuredLogger[F]
) =
this(pageSizeK, maxPageNeeded, Monad[F].unit.map(_ => UUID.randomUUID()))(sl)

private val pageIndices = (1 to maxPageNeeded).toList
private val logSplitIdN = "log_split_id"
private val pageSize = pageSizeK * 1024
Expand Down Expand Up @@ -100,16 +117,17 @@ object PagingSelfAwareStructuredLogger {
private def addCtx(
msg: => String,
ctx: Map[String, String]
): (String, Map[String, String]) = {
val logSplitId = UUID.randomUUID().show
(
logSplitId,
ctx
.updated(logSplitIdN, logSplitId)
.updated("page_size", s"${pageSizeK.show} Kib")
.updated("log_size", s"${msg.length.show} Byte")
)
}
): F[(String, Map[String, String])] =
randomUUID.map { uuid =>
val logSplitId = uuid.show
(
logSplitId,
ctx
.updated(logSplitIdN, logSplitId)
.updated("page_size", s"${pageSizeK.show} Kib")
.updated("log_size", s"${msg.length.show} Byte")
)
}

private def doLogging(
loggingLevelChk: => F[Boolean],
Expand All @@ -118,8 +136,7 @@ object PagingSelfAwareStructuredLogger {
ctx: Map[String, String] = Map()
): F[Unit] = {
loggingLevelChk.ifM(
{
val (logSplitId, newCtx) = addCtx(msg, ctx)
addCtx(msg, ctx).flatMap { case (logSplitId, newCtx) =>
pagedLogging(logOpWithCtx(newCtx), logSplitId, msg)
},
Applicative[F].unit
Expand Down
4 changes: 2 additions & 2 deletions ...red/src/test/scala/org/typelevel/log4cats/PagingSelfAwareStructuredLoggerTestRunner.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class PagingSelfAwareStructuredLoggerTestRunner extends CatsEffectSuite {
val pageSize = pageSizeK * 1024
val stl = StructuredTestingLogger.impl[IO]()
val pagingStl: SelfAwareStructuredLogger[IO] =
PagingSelfAwareStructuredLogger.withPaging[IO](pageSizeK, maxPageNeeded)(stl)
PagingSelfAwareStructuredLogger.paged[IO](pageSizeK, maxPageNeeded)(stl)
val logResult: IO[Unit] = logTest(pagingStl)

val test = logResult >> stl.logged
Expand Down Expand Up @@ -103,7 +103,7 @@ class PagingSelfAwareStructuredLoggerTestRunner extends CatsEffectSuite {
): Unit = {
val stl = StructuredTestingLogger.impl[IO]()
try {
PagingSelfAwareStructuredLogger.withPaging[IO](pageSizeK, maxPageNeeded)(stl)
PagingSelfAwareStructuredLogger.paged[IO](pageSizeK, maxPageNeeded)(stl)
fail(s"$suiteName $caseName: Expected exception not thrown")
} catch {
case thr: Throwable => assert(thr.isInstanceOf[IllegalArgumentException])
Expand Down