-
Notifications
You must be signed in to change notification settings - Fork 74
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
WIP: Feature: logging capability #421
WIP: Feature: logging capability #421
Conversation
Includes both generic, and fixed in the type of logger
See gitter discussion for ideas |
Therefore, your problem might be: | ||
1) you didn't create a LoggingF[${F}] to begin with, or didn't mark it as implicit | ||
2) you only have a GenLoggerF[G[_], SelfAwareStructuredLogger[${F}]] for some G[_] type other than type ${F} | ||
3) you do actually have a GenLogger[${F}, L], but where L is not SelfAwareStructuredLogger[${F}]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can/should we support users doing import org.typelevel.log4s.Slf4jLogging.implicits._
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would prefer not to 😅
It would carry something like implicit def genericSlf4jLogging[F[_]: Sync]: Slf4jLogging[F, F]
? And others, provided they don't wind up conflicting with each other under the same import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, why not? Shouldn't conflict as long as types are different, so it could be both Logging
and LoggingF
under one object. I've no objections to not having it as an experienced Scala-FP-CE-TF developer but not everybody's like that, and a page's worth of text for implicitNotFound
to describe how to refactor all your code is probably going to alienate more than help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. But I am still in favor of a page worth of implicitNotFound
because adopters of Logging as capability are probably experienced Scala-FP-CE-TF devs, since it's an entirely new feature.
I can also move the practical examples first, with the alternative of a-la-carte implicits
import as well, and then move the general points about what might be wrong at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds better. Even "experienced Scala-FP-CE-TF devs" prefer a simple entry point (cue: CE2 Timer/ContextShift/Concurrent vs CE3 import unsafe), and for people just trying log4cats out (there's always a lot of people swayed by TF but not yet with enough experience to handle all the issues) that would provide a straightforward experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR, rephrased the implicitNotFound
message so it doesn't seem like we're giving refactoring advice. But now it's somehow even longer 😅
def anyFSyncLogger[F[_]: Sync]: F[SelfAwareStructuredLogger[F]] = LoggingF[F].create | ||
``` | ||
|
||
Alternatively, a mutually exclusive solution, is to explicitely create your own LoggingF[F] instances |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just passing by here... Is this wall of text really necessary? I imagine that in most cases this warning would be caused by missing imports. I feel like the rest of this should be moved to documentation, with the warning itself stripped to something like this:
Implicit not found for LoggingF[${F}]. This is often caused by missing imports:
import org.typelevel.log4cats._ import org.typelevel.log4cats.slf4j.implicits._Additional information can be found here: https://log4cats.github.io/logging-capability.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably a better approach 👍 will move the message once I put docs updates in the PR.
Hi guys, any progress here? I would really like to use something similar to #224 |
Closing in favor of #629 |
This issue would close #224. But in a more generic way, that allows users to define their own
Logging
capabilities easier than previous attempts, and without having to expose our macros.The idea is that - especially - with the advent of CE3, it's a shame to have to pass along such a powerful capability like
Sync[F]
everywhere you just need logging. And since more and more people are moving towards the pattern of "instantiate a bunch of specific capabilities in your main then pass along where needed", I would like for log4cats to encourage users to do the same with logging.Now, the problem is that the macros we have for sl4j specifically is not ameanable to user extension, meaning that we couldn't provide a user friendly API for creating loggers with zero params. That's why I wrote, and introduced the enclosure microlibrary that solves the issue of getting the enclosing class/object/package name as a data structure, that you can do anything with. The library itself is well tested, and outputs the same things as our sl4j macros.
Which you can see, from a user perspective offers the same user friendly method as the
Slf4jLogger.getLogger
macro backed one. But, again, in an extensible way, especially on the user side. This is extra useful in case users will want to write their own logger capabilities that talk to various external services, while maintaining the ability to create loggers with the same naming pattern.I understand that it's a tough decision to add an external dependency, but I'd be more than happy to donate to typelevel the
enclosure
library, and maintain it myself.Various other advantages to this approach:
Slf4jLogger.getLogger[F]
usingEnclosure
in a source compatible way.Todo:
Enclosure
. Without it, we can't have a genericGenLogging
in core, and still support creating loggers with the name of the enclosing class/object/package@implicitNotFound
annotation onLogging
andLoggingF
do not work on Scala 3LoggingGenId
andLoggingGenF
specifically.LoggingF
convenience type, and method names onGenLogging
.Sync[F]
everywhere.