More consistent logging system #360

Closed
snoyberg opened this Issue Jun 5, 2012 · 11 comments

Comments

Projects
None yet
4 participants
@snoyberg
Member

snoyberg commented Jun 5, 2012

I think we have a number of different components to logging floating around between wai and yesod, and it would be nice to consolidate them together (and maybe add persistent to the mix). As @gregwebs has written most of the logging code, I'm not entirely familiar with what's going on under the surface, so I'd like to start off with just a discussion of what our ideal system would look like.

Here are the components I'm aware of:

  • fast-logger and wai-logger are used under the surface. We are not currently using the apacheFormat, but we should. We should also have a setting somewhere for switching from FromSocket to FromHeader. The latter is especially important in keter deployments.
  • wai-extra's request logger does not directly use wai-logger. Is there a reason for this?
  • wai-extra also differentiates between devel and production, with the latter not flushing the buffer. This causes problems in production, and I think we should always flush the buffer. If this causes performance issues, then a user can always disable logging.
  • yesod-core includes Yesod.Logger, which includes a bunch of utility functions. I'm not sure if we really need this module, or if we can just use wai-logger directly.
  • yesod itself provides warpDebug, which uses wai-logger directly.
  • yesod-default provides a Logger to the application.
@meteficha

This comment has been minimized.

Show comment
Hide comment
@meteficha

meteficha Jun 5, 2012

Member

Regarding yesod-core, I've just written the following type class for my code:

import qualified Yesod as Y

-- | Logging function type.
type LogFunction m = TH.Loc -> Y.LogLevel -> Text -> m ()

-- | Class for monads that support logging.
class Monad m => MonadLogging m where
  -- | Log a message.
  monadLoggingLog :: LogFunction m

The idea is that logDebug, logInfo, logWarn, logError and logOther won't require a GHandler anymore. So I may use these logging function inside my Model and just add a constraint to MonadLogging. The relevant instances are trivial:

-- | All 'Y.GHandler'@s@ are instances of 'MonadLogging'.
instance Y.Yesod master => MonadLogging (Y.GHandler sub master) where
  monadLoggingLog loc level msg = do
    y <- Y.getYesod
    liftIO $ Y.messageLogger y loc level msg

-- | 'SqlPersist' uses 'Y.lift'.
instance MonadLogging m => MonadLogging (SqlPersist m) where
  monadLoggingLog loc level msg = Y.lift (monadLoggingLog loc level msg)

Of course you may write more instances for other monads as well.

Member

meteficha commented Jun 5, 2012

Regarding yesod-core, I've just written the following type class for my code:

import qualified Yesod as Y

-- | Logging function type.
type LogFunction m = TH.Loc -> Y.LogLevel -> Text -> m ()

-- | Class for monads that support logging.
class Monad m => MonadLogging m where
  -- | Log a message.
  monadLoggingLog :: LogFunction m

The idea is that logDebug, logInfo, logWarn, logError and logOther won't require a GHandler anymore. So I may use these logging function inside my Model and just add a constraint to MonadLogging. The relevant instances are trivial:

-- | All 'Y.GHandler'@s@ are instances of 'MonadLogging'.
instance Y.Yesod master => MonadLogging (Y.GHandler sub master) where
  monadLoggingLog loc level msg = do
    y <- Y.getYesod
    liftIO $ Y.messageLogger y loc level msg

-- | 'SqlPersist' uses 'Y.lift'.
instance MonadLogging m => MonadLogging (SqlPersist m) where
  monadLoggingLog loc level msg = Y.lift (monadLoggingLog loc level msg)

Of course you may write more instances for other monads as well.

@gregwebs

This comment has been minimized.

Show comment
Hide comment
@gregwebs

gregwebs Jun 5, 2012

Member

I like what Felipe is doing here: ideally we would also make it easy to log in Persistent.

I like how the current wai-extra devel logger does not use Apache format and instead is trying to be as informational as possible. Production can switch to wai-logger Apache style (it doesn't use wai-logger because I wrote it before wai-logger was released) and also flush the log file on every request.

Yesod.Logger is a nice abstraction that should allow someone to swap out different logging systems and also give file location information.

Member

gregwebs commented Jun 5, 2012

I like what Felipe is doing here: ideally we would also make it easy to log in Persistent.

I like how the current wai-extra devel logger does not use Apache format and instead is trying to be as informational as possible. Production can switch to wai-logger Apache style (it doesn't use wai-logger because I wrote it before wai-logger was released) and also flush the log file on every request.

Yesod.Logger is a nice abstraction that should allow someone to swap out different logging systems and also give file location information.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 4, 2012

Member

I don't really see what nice abstraction Yesod.Logger is providing over System.Log.FastLogger. It's hiding away details of flushing, provides some convenience wrappers for common types, and then provide some formatting functions (formatLogText and timed). I don't see a reason any of that needs to be in yesod-core.

Edit: That first sentence makes no sense in context. What I mean is: I don't think it's enough of a nice abstraction to create a yesod-specific library that incompatible with any other logging system. Let's just try to get the functionality moved upstream so all packages can use it.

@kazu-yamamoto What do you think about adding some of this functionality to fast-logger? I like the idea of centralizing everything there, since (1) we're already using it and (2) it's a logical place for persistent to pull from as well. Could you imagine including something like MonadLogging in fast-logger?

Another question: is there a reason that the DateRef code is in wai-logger and not fast-logger? Maybe we could:

  1. Put DateRef in fast-logger
  2. Add a Logger type like Greg's from Yesod.Logger which keeps a DateRef
  3. Change initHandle to mkLogger :: Handle -> IO Logger
  4. hPutLogStr uses a Logger instead of a Handle.

This helps with type safety (you can't accidentally use a non-buffered Handle) and keeps a DateRef available for logging usage.

If you're OK with this stuff in principle, I can start a fork and send a pull request when I have the whole thing worked out, and we can tweak it then. Does that sound reasonable?

Member

snoyberg commented Jul 4, 2012

I don't really see what nice abstraction Yesod.Logger is providing over System.Log.FastLogger. It's hiding away details of flushing, provides some convenience wrappers for common types, and then provide some formatting functions (formatLogText and timed). I don't see a reason any of that needs to be in yesod-core.

Edit: That first sentence makes no sense in context. What I mean is: I don't think it's enough of a nice abstraction to create a yesod-specific library that incompatible with any other logging system. Let's just try to get the functionality moved upstream so all packages can use it.

@kazu-yamamoto What do you think about adding some of this functionality to fast-logger? I like the idea of centralizing everything there, since (1) we're already using it and (2) it's a logical place for persistent to pull from as well. Could you imagine including something like MonadLogging in fast-logger?

Another question: is there a reason that the DateRef code is in wai-logger and not fast-logger? Maybe we could:

  1. Put DateRef in fast-logger
  2. Add a Logger type like Greg's from Yesod.Logger which keeps a DateRef
  3. Change initHandle to mkLogger :: Handle -> IO Logger
  4. hPutLogStr uses a Logger instead of a Handle.

This helps with type safety (you can't accidentally use a non-buffered Handle) and keeps a DateRef available for logging usage.

If you're OK with this stuff in principle, I can start a fork and send a pull request when I have the whole thing worked out, and we can tweak it then. Does that sound reasonable?

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Jul 4, 2012

Historically speaking, I split one package into three because Greg asked me to do so. (It's not my requirement.) Please feel free to do anything convenient for you. My requirement is that these three packages can be used from Mighttpd2. Please send me pull requests and I will see if Mighttpd2 can use new ones.

Historically speaking, I split one package into three because Greg asked me to do so. (It's not my requirement.) Please feel free to do anything convenient for you. My requirement is that these three packages can be used from Mighttpd2. Please send me pull requests and I will see if Mighttpd2 can use new ones.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 5, 2012

Member

First changes to RequestLogger at: yesodweb/wai@aa7d631

Based on the changes in my logger repo: https://github.com/snoyberg/logger

@kazu-yamamoto I'm not done yet, but what do you think? I'll understand if you think the TH and MonadLogging aspects don't belong in this package, in which case I'll split it off into a separate one.

Member

snoyberg commented Jul 5, 2012

First changes to RequestLogger at: yesodweb/wai@aa7d631

Based on the changes in my logger repo: https://github.com/snoyberg/logger

@kazu-yamamoto I'm not done yet, but what do you think? I'll understand if you think the TH and MonadLogging aspects don't belong in this package, in which case I'll split it off into a separate one.

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Jul 5, 2012

Logger type is very good. :-)

I don't have logical reasons but I guess TH (and Monad stuff) should be in a separate package (if possible). This is not a strong opinion, though.

When you update fast-logger and wai-logger, I will update wai-logger-prefork by myself.

Logger type is very good. :-)

I don't have logical reasons but I guess TH (and Monad stuff) should be in a separate package (if possible). This is not a strong opinion, though.

When you update fast-logger and wai-logger, I will update wai-logger-prefork by myself.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 5, 2012

Member

I have no problem with a separate package. Any objections to the name monad-logger, with a module of Control.Monad.Logger?

Other question is where to keep it. I see could either include it in the logger repo and send you a pull request, or put it in a separate repo. The advantage of the former is that we keep the packages all together, with the (possible) downside of making you maintain it. I'll go either way here, whichever you prefer.

Member

snoyberg commented Jul 5, 2012

I have no problem with a separate package. Any objections to the name monad-logger, with a module of Control.Monad.Logger?

Other question is where to keep it. I see could either include it in the logger repo and send you a pull request, or put it in a separate repo. The advantage of the former is that we keep the packages all together, with the (possible) downside of making you maintain it. I'll go either way here, whichever you prefer.

@kazu-yamamoto

This comment has been minimized.

Show comment
Hide comment
@kazu-yamamoto

kazu-yamamoto Jul 5, 2012

I feel Control.Monad.Logger is a little bit aggressive. What if other people want to impliment another monadic logger? But this is not a strong opinion.

If you want, I will add your account to the maintainer of the logger package on github. In this case, you can just push your modifications.

I feel Control.Monad.Logger is a little bit aggressive. What if other people want to impliment another monadic logger? But this is not a strong opinion.

If you want, I will add your account to the maintainer of the logger package on github. In this case, you can just push your modifications.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 5, 2012

Member

Persistent logging is now implementing. When signing in to a newly scaffolded site:

05/Jul/2012:13:29:51 +0300 [Other "SQL"] "SELECT \"id\",\"ident\",\"password\" FROM \"user\" WHERE \"ident\"=?" [PersistText "michael.snoyman@gmail.com"] @(persistent-1.0.0:Database.Persist.GenericSql.Raw ./Database/Persist/GenericSql/Raw.hs:78:12)
05/Jul/2012:13:29:51 +0300 [Other "SQL"] "INSERT INTO \"user\"(\"ident\",\"password\") VALUES(?,?)" [PersistText "michael.snoyman@gmail.com",PersistNull] @(persistent-1.0.0:Database.Persist.GenericSql.Raw ./Database/Persist/GenericSql/Raw.hs:87:5)
05/Jul/2012:13:29:51 +0300 [Other "SQL"] "SELECT last_insert_rowid()" [] @(persistent-1.0.0:Database.Persist.GenericSql.Raw ./Database/Persist/GenericSql/Raw.hs:78:12)

I've written monad-logger. I'm going to port over the repos to use it, and if everything works, I'll send a pull request.

Member

snoyberg commented Jul 5, 2012

Persistent logging is now implementing. When signing in to a newly scaffolded site:

05/Jul/2012:13:29:51 +0300 [Other "SQL"] "SELECT \"id\",\"ident\",\"password\" FROM \"user\" WHERE \"ident\"=?" [PersistText "michael.snoyman@gmail.com"] @(persistent-1.0.0:Database.Persist.GenericSql.Raw ./Database/Persist/GenericSql/Raw.hs:78:12)
05/Jul/2012:13:29:51 +0300 [Other "SQL"] "INSERT INTO \"user\"(\"ident\",\"password\") VALUES(?,?)" [PersistText "michael.snoyman@gmail.com",PersistNull] @(persistent-1.0.0:Database.Persist.GenericSql.Raw ./Database/Persist/GenericSql/Raw.hs:87:5)
05/Jul/2012:13:29:51 +0300 [Other "SQL"] "SELECT last_insert_rowid()" [] @(persistent-1.0.0:Database.Persist.GenericSql.Raw ./Database/Persist/GenericSql/Raw.hs:78:12)

I've written monad-logger. I'm going to port over the repos to use it, and if everything works, I'll send a pull request.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 5, 2012

Member

Pull request sent, please review and let me know if you have any comments.

Member

snoyberg commented Jul 5, 2012

Pull request sent, please review and let me know if you have any comments.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Jul 9, 2012

Member

Closing this issue, if anyone comes up with more improvements to add in, please open as a separate issue.

Member

snoyberg commented Jul 9, 2012

Closing this issue, if anyone comes up with more improvements to add in, please open as a separate issue.

@snoyberg snoyberg closed this Jul 9, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment