Issue/62 custom access and error log handlers (against master) #72

robertmassaioli · 2015-05-21T22:31:56Z

This is the exact same pull request as #71 except that it is against master instead of 0.9-stable.

This is for issue: #62

This is my first attempt at custom access and error log handlers.

…ue/62-custom-access-and-error-log-handlers-against-master

sopvop · 2015-05-22T07:54:56Z

There is a note in Snap.Internal.Http.Server.Config with what @gregorycollins wants to do with log configuration in 1.0.
So, maybe you can also deal with it, as part of logging enhancement.

-- As I understand it, `Config` should only deal with command line options,
-- And ConfigLog is just replaced with FilePath, and no logging to file is done by default.
data Config m a = Config
    { ...
    , accesslog :: Maybe FilePath
    , errorlog : Maybe FilePath
    }
-- So for custom logging you actually need to modify `ServerConfig`
setAccessLogging :: OutputStream LogEvent -> ServerConfig a -> ServerConfig a
setErrorLogging :: OutputStream ByteString -> ServerConfig a -> ServerConfig a
-- And LogEvent can mimic _logAccess field in ServerConfig
type LogEvent = (Request, Response, Word64)

But you should really wait for @gregorycollins comments, I could have wrong understanding of it.
The server configuration is one of the major things in 1.0 TODO.

robertmassaioli · 2015-05-22T08:08:38Z

Oh I see, he would just like to produce an output stream of LogEvents. That would make sense and would mean that I would have to modify what I have done. It could be done though I think. I guess I just have to wait for @gregorycollins to come back and comment.

I also wonder if it is okay if we do it one way for the 0.9-stable branch and another for the 1.0 branch. After all, 1.0 is a breaking release so the two approaches don't have to be compatible. (Although I guess ideally they would be)

sopvop · 2015-05-22T08:20:46Z

Well, I've switched to 1.0 half a year ago, and have no problems.
So if you don't want to do work twice and you are ok with getting dependencies from git and not hackage (sandboxes help a lot), you can switch too ;-)

robertmassaioli · 2015-05-22T09:55:48Z

@sopvop I prefer to use packages that are available in Hackage. I am writing code that is used by a lot of people and I don't want to have to build a secondary build system on top of cabal in order to let those people download all of the required dependencies without having intimate knowledge of what I am trying to do. Unfortunately, without tooling support, using git repositories directly only works for development on my local machine. :)

So I would really like to see 1.0 released to Hackage before I start using it in projects that I will be woken up in the middle of the night for if they have issues.

mightybyte · 2015-05-22T12:31:45Z

@robertmassaioli I would like to point out that using git submodules can provide a nice way to use packages only available on github. Our top level snap package is using submodules and it has made working with 1.0 MUCH easier. I started using submodules in commit snapframework/snap@6f39bf7. You can look at the diff to see the details but it basically involved two things. First I created a deps directory, cloned all the non-hackage dependencies there and added them as submodules. Then I created an init-sandbox.sh script that sets up a sandbox appropriately.

Now, all a user has to do to get up and running with snap 1.0 is clone snap, do a git pull && git submodule update --init --recursive (which I've automated with a pull.sh script), run init-sandbox.sh, and then build with cabal as normal.

I'm not here to try to convince you to use 1.0. You are the best person to decide whether you should upgrade or not. But I will say that based on my experience if you use Haskell enough I expect that you will eventually encounter the need to use code that has not been released on hackage. And if you need to do that, this approach provides a nice way of doing it.

robertmassaioli · 2015-05-22T22:25:02Z

@mightybyte I hear you loud and clear. :) In fact, I have used git submodules in the past and exactly in the way that you described (except we used a Makefile instead of init-sandbox.sh script). We used that setup while we were developing:

However, we have reached the point where I have managed to remove all references go git submodules. I think that is nice. That said, I'm not completely against the idea of using submodules if it clearly the winning option. And maybe you guys are really close to the 1.0 release? I'm not sure, I have not asked in a while. At any rate, for now, I am quite happy to invest my effort into backporting this to 0.9-stable as there will be a lot of people that use snap-server that would be advantaged by that.

And on that note @sopvop or @mightybyte are either of you project collaborators on snap-server? Could you review this PR and tell me what needs improving? Should I just make that OutputStream change right now for example? Cheers and thanks for the comments and help, I really appreciate it!

mightybyte · 2015-05-22T22:31:57Z

@robertmassaioli Ok, good. I just wanted to throw that out there. I leave the snap-server stuff to @gregorycollins. He has been on vacation but is back now, so I expect he'll take a look at it soon.

gregorycollins · 2015-05-27T17:52:35Z

src/Snap/Http/Server.hs

@@ -44,10 +44,10 @@ import           Data.ByteString.Builder           (Builder, toLazyByteString)
 ------------------------------------------------------------------------------
 import qualified Paths_snap_server                 as V
 import           Snap.Core                         (MonadSnap (..), Request, Response, Snap, rqClientAddr, rqHeaders, rqMethod, rqURI, rqVersion, rspStatus)
-import           Snap.Http.Server.Config           (Config, ConfigLog (..), commandLineConfig, completeConfig, defaultConfig, getAccessLog, getBind, getCompression, getDefaultTimeout, getErrorHandler, getErrorLog, getHostname, getLocale, getOther, getPort, getProxyType, getSSLBind, getSSLCert, getSSLChainCert, getSSLKey, getSSLPort, getStartupHook, getVerbose)
+import           Snap.Http.Server.Config           (Config, ConfigLog (..), commandLineConfig, completeConfig, defaultConfig, getAccessLog, getBind, getCompression, getDefaultTimeout, getErrorHandler, getErrorLog, getHostname, getLocale, getOther, getPort, getProxyType, getSSLBind, getSSLCert, getSSLChainCert, getSSLKey, getSSLPort, getStartupHook, getVerbose, getAccessLogHandler, getErrorLogHandler)


Please run stylish-haskell (the .stylish-haskell.yaml in the repo root should be used) so that the imports are properly sorted.

gregorycollins · 2015-05-27T17:56:55Z

This looks OK to me for the master branch -- assuming I don't do more aggressive surgery to the config mechanisms before then (but I'd fix that) -- but I'm not sure I want to break backwards-compatibility for 0.9.x just to release this when 1.0 is right around the corner. Can you use 1.0.x? How desperate are you for this functionality on the 0.9.x branch?

gregorycollins · 2015-05-27T18:03:50Z

src/Snap/Internal/Http/Server/Config.hs

+------------------------------------------------------------------------------
+-- | This handler may be used (in conjunction with setErrorLogHandler) to write out error logs in a 
+-- custom manner.
+type ErrorLogHandler = ByteString -> IO ByteString


Move this into ".Types" (see the _accessLog member, we are already using this type without an alias there)

I've just tried to work out what you meant by this comment and I can't work it out. :( I think you mean: remove the ErrorLogHandler definition and just use ByteString -> IO ByteString directly all around the code?

But part of me is thinking that maybe you want me to move these type definitions to: src/Snap/Internal/Http/Server/Types.hs

I'm just confused because the only thing that _accessLog does that is similar to this is the ConfigIoLog constructor.

So maybe what you really want me to do is:

data ErrorLogHandler = ErrorLogHandler (ByteString -> IO ByteString)

Is that more like what you meant? Which one is it? Or am I totally off the mark?

robertmassaioli · 2015-05-27T21:31:34Z

@gregorycollins On the topic of, can I use the 1.0 branch? The answer is: not really unless I want to put a bunch of effort into refactoring our testing and deployment process for our services. It would be much simpler to just rely upon packages that are released to Hackage. So I would say that, if 1.0 is going to be released in the next week or two then I could wait. But 1.0 has been close to release for many, many months now and I would prefer to not wait that long: it's why I went to the effort of back-porting it.

And, on another note, the AccessLogHandler is also not the same between 0.9 and 1.0 anyway. So further changes will probably be acceptable for people that are upgrading. After all, if you can't break things in a breaking release then when can you? 😄

I guess it is ultimately up to you as the maintainer to decide wether to merge in the back-ported changes but my response would be: please? 🙏

robertmassaioli · 2015-05-27T21:35:12Z

src/Snap/Internal/Http/Server/Config.hs

+------------------------------------------------------------------------------
+-- | This handler may be used (in conjunction with setErrorLogHandler) to write out error logs in a
+-- custom manner.
+type ErrorLogHandler = ByteString -> IO ByteString


Wow, it deleted my previous changes here when I updated the PR. Not used to that. I do most of my work on BitBucket. So, what did you mean specifically about this type? Did you want me to:

Move it to the Types.hs module OR

Rename the type to: data ErrorLogHandler = ErrorLogHandler (ByteString -> IO ByteString)

Delete it altogether and use (ByteString -> IO ByteString) everywhere?

I am just not sure what your previous comment here was asking for. Cheers.

I just meant that we already have this type in .Types without an alias (search for _logAccess in that file), so we should move the alias into .Types and use it there.

BTW why is the type of ErrorLogHandler ByteString -> IO ByteString here? We use "Builder -> IO ()" internally in the server, and we should probably do the same here.

@gregorycollins So you want me to change the type to Builder -> IO Builder? Because otherwise the ErrorLogHandler would actually have to write the logs to the ConfigLog, rather than just producing logs in a custom format as it currently does.

Yes, the error log handler should write the logs itself and return (). The idea is that out of the box we give you logging to file, in the usual format. If you want something else, you can override with a user-provided function and do whatever you want, and we will provide you with the same building blocks that we use (that's what I meant about exporting the function that generates these common log format messages to the user).

Let's say for example that (and this is a bad idea, but just for the sake of argument) you'd like to log the access log information into an SQL database. In this case, the solution you're providing is awkward, because we serialize to string without really needing to do that. In this case it's because the API we provide the user right now is bad --- the ConfigLog type should be broken into AccessLogType and ErrorLogType, and their equivalents to the ConfigIoLog constructor should have the correct, more specific types that we use internally in snap-server 1.0.

Ideally 0.10 and 1.0 should have the same API here, so that we introduce this feature once and we don't have to break user code in the upgrade from 0.10 to 1.0. In this case I'm totally OK with breaking users

gregorycollins · 2015-09-23T15:52:41Z

I've been working on the command-line config refactor, albeit slowly because my schedule has been completely jammed since August: https://github.com/snapframework/snap-server/blob/commandline/src/Snap/Internal/Http/Server/CmdlineConfig.hs

The idea is that the configuration of the webserver will be separated into two phases: first processing the command line arguments, then producing a new internal configuration that the server will actually use. The command-line config stuff will export a lot of stuff that we currently do (e.g. default 404 handlers, setting up compression, proxying) as utility functions so that users can hook into configurations more easily by mixing and matching.

The access/error log stuff is going to be part of that. Just to get this down on the pull request history: as I believe I've written to you over email, the API the 1.0 server actually uses is:

data ServerConfig hookState = ServerConfig
    { _logAccess :: !(Request -> Response -> Word64 -> IO ())
    , _logError  :: !(Builder -> IO ())
...

What has to change there is the access log type:

setAccessLog :: CmdlineConfigLog -> CmdlineConfig m a -> CmdlineConfig m a
setAccessLog x c = c { accessLog = Just x }

data CmdlineConfigLog = ConfigNoLog                     -- ^ no logging
                      | ConfigFileLog FilePath          -- ^ log to text file
                      | ConfigIoLog (Builder -> IO ())  -- ^ log custom IO handler

The issue here (as you pointed out) is that we don't want Builder -> IO () to be the type here. Instead we want to propagate the type (Request -> Response -> Word64 -> IO ()) for access logging all the way back to the user, so we need to create a new interface type for that.

robertmassaioli · 2016-01-30T07:05:16Z

I'll come back to this PR later if I have time.

robertmassaioli added 6 commits May 20, 2015 14:26

Issue 62: First attempt at custom access and error log handlers.

fb438cc

This is my first attempt at custom access and error log handlers.

Issue 62: Made the error handler work and exported the handler types.

b3be5db

Issue 62: Cleaning up the code to get it ready for a pull request.

e28ec29

Issue 62: Forgot to add one message.

6b01854

Merge branch 'issue/62-custom-access-and-error-log-handlers' into iss…

8fe340f

…ue/62-custom-access-and-error-log-handlers-against-master

Issue 62: Cleaned up some imports.

8c4515f

robertmassaioli mentioned this pull request May 21, 2015

Issue/62 custom access and error log handlers (against 0.9-stable) #71

Open

gregorycollins reviewed May 27, 2015
View reviewed changes

Issue 62: Running Stylish Haskell on the code.

b5e2305

robertmassaioli reviewed May 27, 2015
View reviewed changes

robertmassaioli closed this Jan 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/62 custom access and error log handlers (against master) #72

Issue/62 custom access and error log handlers (against master) #72

robertmassaioli commented May 21, 2015

sopvop commented May 22, 2015

robertmassaioli commented May 22, 2015

sopvop commented May 22, 2015

robertmassaioli commented May 22, 2015

mightybyte commented May 22, 2015

robertmassaioli commented May 22, 2015

mightybyte commented May 22, 2015

gregorycollins May 27, 2015

gregorycollins commented May 27, 2015

gregorycollins May 27, 2015

robertmassaioli May 27, 2015

robertmassaioli commented May 27, 2015

robertmassaioli May 27, 2015

gregorycollins May 29, 2015

robertmassaioli May 29, 2015

gregorycollins Jun 19, 2015

gregorycollins commented Sep 23, 2015

robertmassaioli commented Jan 30, 2016

Issue/62 custom access and error log handlers (against master) #72

Issue/62 custom access and error log handlers (against master) #72

Conversation

robertmassaioli commented May 21, 2015

sopvop commented May 22, 2015

robertmassaioli commented May 22, 2015

sopvop commented May 22, 2015

robertmassaioli commented May 22, 2015

mightybyte commented May 22, 2015

robertmassaioli commented May 22, 2015

mightybyte commented May 22, 2015

gregorycollins May 27, 2015

Choose a reason for hiding this comment

gregorycollins commented May 27, 2015

gregorycollins May 27, 2015

Choose a reason for hiding this comment

robertmassaioli May 27, 2015

Choose a reason for hiding this comment

robertmassaioli commented May 27, 2015

robertmassaioli May 27, 2015

Choose a reason for hiding this comment

gregorycollins May 29, 2015

Choose a reason for hiding this comment

robertmassaioli May 29, 2015

Choose a reason for hiding this comment

gregorycollins Jun 19, 2015

Choose a reason for hiding this comment

gregorycollins commented Sep 23, 2015

robertmassaioli commented Jan 30, 2016