bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
798 stars 186 forks source link

Support "combined" (CLF) log format #76

Closed jzaefferer closed 9 years ago

jzaefferer commented 9 years ago

I've took the implementation from #72, rebased it on top of master, dropped most of the (questionable) package.json changes, adjusted the implementation based on the review (mostly renaming to clfFormat), extracted the datetime helper into a separate module and added a primitive test, which just verifies the current output.

I've also tested this in my own application, where it seems to work fine. I'm not sure how good the actual log format is, so the test may just confirm that the implementation is wrong.

As to the question from #72 why "basic-auth" is needed: Its used to output the ":remote-user" part, as far as I can tell.

jzaefferer commented 9 years ago

Also the test failed on node 0.12, which is weird, since that is what I tested with locally. It expected "ms", but got "1ms" - maybe the template doesn't output anything for response time of 0...?

jzaefferer commented 9 years ago

Should set a fixed response time on the response mock. I haven't yet figured out how to do that...

jzaefferer commented 9 years ago

I tried this, but it doesn't do anything:

res = mocks.createResponse();
res.responseTime = 5;
res.status(200);
floatingLomas commented 9 years ago

I'll have a look at this soon.

floatingLomas commented 9 years ago

Response time isn't part of either the Common or (Combined)[https://httpd.apache.org/docs/trunk/logs.html#combined] Log Formats...?

FYI: Here are the regexes to match the two of them. I have a total love-hate thing with regex.

var commonLogRegex = /^((\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})|-) (.*|-) (.*|-) \[\d{1,2}\/.{3}\/\d{4}:\d{2}:\d{2}:\d{2} [+-]\d{4}\] "(GET|HEAD|POST|PUT|PATCH|DELETE|TRACE|CONNECT) \/.{0,} HTTP\/\d\.\d" \d{3} \d{1,}$/;

var combinedLogRegex = /^((\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})|-) (.*|-) (.*|-) \[\d{1,2}\/.{3}\/\d{4}:\d{2}:\d{2}:\d{2} [+-]\d{4}\] "(GET|HEAD|POST|PUT|PATCH|DELETE|TRACE|CONNECT) \/.{0,} HTTP\/\d\.\d" \d{3} \d{1,} ".*?" ".*?"$/;

We probably should have both available...?

Also, you're going to have to add the httpVersion to the req right around here in the setUp() function, since it expects something:

req = mocks.createRequest(reqSpec); req.httpVersion = '1.0';

Also, res._headers["content-length"] should default to 0, not -.

jzaefferer commented 9 years ago

In the 3 days since I created the PR, requirements on my project changed and I now need a winston format that I can't implement with this module. You're welcome to use the code from this PR and make it mergable.

bithavoc commented 9 years ago

:+1: @floatingLomas what do we do there?

floatingLomas commented 9 years ago

@bithavoc, as of right now, I'd say nothing. I'll go poke whoever submitted #72 and see if they want to get it passing/tested/etc, but if someone really wants this, they can always just use a custom msg anyways.