Expunge all unsafe logging [resolve conflicts] #7122
Conversation
mergify
bot
added
module:core
module:server
module:client
module:ember-server
module:ember-client
module:circe
docs
There was a problem hiding this comment.
Thank you so much for fixing this up! Seems fine to me.
Commenting on my own changes ... unfortunately this was a bit messy, but I think it's better to rip-off this bandaid. Future merges from 0.23 are going to be Fun:tm: for anything this touched.
I'll leave this open for the rest of the week to give a chance for other http4s maintainers to weigh in.
| httpVersion = firstResp.httpVersion, | ||
| headers = firstResp.headers.redactSensitive(redactWhen), | ||
| ).withEntity(JsonErrorHandlerResponse[G](req, mf)) | ||
| } | ||
| case t => | ||
| serviceErrorLogger |
There was a problem hiding this comment.
Currently, 'serviceErrorLogger' is defined as a def. Should we define it as a val right above the Kleisli?
): Kleisli[F, Request[G], Response[G]] = {
val serviceErrorLogger = LoggerFactory[F].getLoggerFromName(...)
Kleisli { req =>
...
}
}
There was a problem hiding this comment.
Oh good point. We can probably do the same with messageFailureLogger?
Btw is there a meaningful difference between the effectful and non-effectful APIs for obtaining a logger?
/~https://github.com/typelevel/log4cats/blob/36bfbb6bd8d89e5f81575f85c78b0f8616de5c0b/core/shared/src/main/scala/org/typelevel/log4cats/LoggerFactoryGen.scala#L21-L26
| .fold( | ||
| _ => { | ||
| Message.logger | ||
| .warn(s"Attempt to provide a negative content length of $l") |
There was a problem hiding this comment.
We are not logging a warning anymore.
There was a problem hiding this comment.
I removed this in #6645 (comment) because it makes the ergonomics really bad. I don't know how useful it is.
| service: Kleisli[F, Request[G], Response[G]], | ||
| redactWhen: CIString => Boolean = Headers.SensitiveHeaders.contains, | ||
| ): Kleisli[F, Request[G], Response[G]] = | ||
| Kleisli { req => | ||
| import cats.syntax.applicative._ | ||
| import cats.syntax.applicativeError._ | ||
| implicit def entEnc[M[_]]: EntityEncoder.Pure[JsonErrorHandlerResponse[M]] = |
There was a problem hiding this comment.
Not related to the PR, but it caught my eye.
The entEnc is always used with G type parameter. Technically, we can preallocate it too:
implicit val entityEncoder: EntityEncider.Pure[JsonErrorHandlerResponse[F]] =
JsonErrorHandlerResponse.entEnc[M](redactWhen)On the other hand, since it's defined as def, no error = no allocation. The same is true for loggers.
docs/docs/json.md
Outdated
| @@ -228,6 +229,7 @@ import org.http4s.circe._ | |||
| import org.http4s.dsl.io._ | |||
| import org.http4s.ember.server._ | |||
| import org.http4s.implicits._ | |||
| import org.typelevel.log4cats.slf4j._ | |||
There was a problem hiding this comment.
importing these will cause warnings, we should use Slf4jFactory.create[IO] in app boot.
There was a problem hiding this comment.
Good point. I bumped logs4cats to 2.6.0 and replaced org.typelevel.log4cats.slf4j._ with Slf4jFactory.create.
|
Ok, let's rip off this bandaid. Thank you so much for getting this cleaned up, and sorry to anyone caught in the crossfire. |
The follow-up to #6645 (comment).
@armanbilge I hope I didn't break anything 😬