Add module for log4cats support for debugging purposes #1896
Conversation
Creates a Log4CatsLogHandler class
| | | ||
| | ${s.linesIterator.dropWhile(_.trim.isEmpty).mkString("\n ")} | ||
| | | ||
| | arguments = [${a.mkString(", ")}] |
There was a problem hiding this comment.
I think logging arguments would be a security violation in many applications. Perhaps we can make this an option (default false)?
There was a problem hiding this comment.
I just copied whatever jdkLogHandler does.
https://github.com/tpolecat/doobie/blob/main/modules/free/src/main/scala/doobie/util/log.scala#L66
There was a problem hiding this comment.
jdkLogHandler is not for production use (as written in the scaladoc). Although I think we can update that scaladoc too to provide additional warning that it prints arguments
There was a problem hiding this comment.
This class would still be very valuable for debugging purposes in non-production environments.
Indeed it is true that it could log sensitive data.
EDIT: I added a scaladoc to make the purpose clear.
| """.stripMargin) | ||
|
|
||
| case ProcessingFailure(s, a, l, e1, e2, t) => | ||
| logger.warn( |
There was a problem hiding this comment.
I think this should be an error too? (e.g. error in Put/Get definition)
There was a problem hiding this comment.
It seemed to me not as severe like ExecFailure, and giving it a different priority also give more power to tune what (and how) is logged.
But if you'd still prefer to have both as errors, no problem, just let me know.
There was a problem hiding this comment.
Thinking about it more, perhaps both these failure cases should be warn level because
- If the failure isn't intentional, their code is going to fail anyway and their execution will fail with an Exception anwyay.
- If the (Execution) failure is intentional, then we don't really want to log at error level to avoid triggering alerts.
WDYT? I don't really mind
sideeffffect
changed the title
Creates the
Log4CatsDebuggingLogHandlerclass