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

Abstract Logging #1735

Open
parsonsmatt opened this issue Aug 4, 2021 · 5 comments · May be fixed by #1736
Open

Abstract Logging #1735

parsonsmatt opened this issue Aug 4, 2021 · 5 comments · May be fixed by #1736

Comments

@parsonsmatt
Copy link
Contributor

Right now, yesod hardcodes logging to LoggerSet, which only operates on filepaths. This makes it difficult to integrate with any other means of logging.

Specifically, I want yesod to log via katip.

This would be a breaking change, since Yesod's makeLogger :: app -> IO LoggerSet is a wired in part of the application.

@snoyberg
Copy link
Member

snoyberg commented Aug 5, 2021

My ultimate goal here is really to move Yesod over to rio's logging system, and rio more generally.

That said, even without a breaking change, I think it's possible to extend Yesod to be more accepting of other logging functions. The MonadLogger instance (https://www.stackage.org/haddock/nightly-2021-08-05/yesod-core-1.6.21.0/src/Yesod.Core.Types.html#line-510) uses the rheLog field, so any underlying log system that can accept those four parameters should work.

@ivanbakel
Copy link
Contributor

I've written a library for hooking Yesod up to Katip scribes - but it's forced to hijack messageLoggerSource, which is not ideal. First class support would be best, especially to be able to easily use the same logger when creating the site as when using it.

@ivanbakel ivanbakel linked a pull request Aug 25, 2021 that will close this issue
@snoyberg
Copy link
Member

Can you clarify what you mean by hijacking messageLoggerSource? I'd like to understand what the current workaround is, since I'm not hugely in favor of an associated type family approach if we can avoid it (referencing #1736).

@ivanbakel
Copy link
Contributor

I mean that logging to Katip scribes required manually calling out to (e.g.) logItem in messageLoggerSource, basically ignoring the Logger argument and the flow around makeLogger.

It was also problematic because I found that I had to remember to manually respect shouldLogIO, which is normally only called by the default messageLoggerSource implementation.

(The code for the instance is here.)

@snoyberg
Copy link
Member

There's definitely some legacy cruft in the Yesod typeclass around logging. I'm trying to separate out the "relevant to other loggers" stuff from "would be nice to clean up" stuff. messageLoggerSource and shouldLogIO fall into the latter category.

If I'm reading the code in question, and the comments above, it seems this is less about "Katip isn't supported" and more "Katip isn't the default supported type and we'd like that configurable." Which I understand, but I'm hesitant to do because of the potential for more complex error messages and overall maintenance burden.

I am in favor of moving away from the current logging approach to something built around rio, and I think if we did that the right solution here would be having some kind of a HasLogFunc instance for Katip that makes everything tie together correctly. Yesod is currently too tied to a specific implementation. I'm just not sure I want to take the current type instance approach.

Does that make sense?

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 a pull request may close this issue.

3 participants