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.

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

Slf4j v2 backend #639

Merged
merged 14 commits into from
Feb 10, 2023
Merged

Slf4j v2 backend #639

merged 14 commits into from
Feb 10, 2023

Conversation

justcoon
Copy link
Contributor

@justcoon justcoon commented Jan 30, 2023

depends on #638

fixes: #637

similar like slf4j v1 implementation (for easier migrations), but using some of new features like key value pairs
instead of MDC which have issue (like #491), as MDC using ThreadLocal

throwable/exception handling was changed to:

  • when there is Cause.Die or Cause.Fail with Throwable, that throwable is logged
  • otherwise cause is transformed to zio.FiberFailure - as it was originally in all cases

TODO:

  • markers
    • check support for multiple Markers - slf4j v1 backend is able to set just one marker, by marker name
    • check other marker related features - option to provide created Marker?

@justcoon justcoon marked this pull request as ready for review February 7, 2023 20:01
@justcoon justcoon requested a review from a team as a code owner February 7, 2023 20:01
@@ -105,7 +105,11 @@ object SLF4J {
*/
override def appendCause(cause: Cause[Any]): Unit = {
if (!cause.isEmpty) {
throwable = FiberFailure(cause)
cause match {
Copy link
Member

Choose a reason for hiding this comment

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

Matching on a cause directly is not reliable because of nodes like or / and / stackless /e tc.

Instead, if you are trying to just "get the most important throwable", then do something like squash or defects or perhaps: (cause.errors.collect { case t : Throwable } ++ cause.defects).head (or its equivalent).

Because you want to prefer an error before you look into the defects, and you want to be robust in the presence of intermediate / metadata nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdegoes

I changed that to

        if (!cause.isEmpty) {
          val maybeThrowable = (cause.failures.collect { case t: Throwable => t } ++ cause.defects).headOption
          maybeThrowable match {
            case Some(t) => throwable = t
            case None    => throwable = FiberFailure(cause)
          }
        }

can you check it? Thank you

Copy link
Member

@jdegoes jdegoes left a comment

Choose a reason for hiding this comment

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

One quick comment to make extracting a Throwable from a Cause more robust.

Copy link
Member

@jdegoes jdegoes left a comment

Choose a reason for hiding this comment

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

Looks great! Please also open a ticket on ZIO to add Cause#toThrowable that does something similar (but more efficiently).

@jdegoes jdegoes merged commit f9d5c28 into zio:master Feb 10, 2023
@justcoon justcoon deleted the slf4j2_backend branch February 13, 2023 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slf4j v2 logging backend
2 participants