Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add logging of read requests that redirect from first path. #47

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

robertjustjones commented Mar 4, 2014

This PR implements the logging discussed in enhancement issue #46. It sounds like you may have in mind to implement this in a different way long term so this is offered as clarification of the request intent.

For a registry read request where the package is not in the first path, the output in the log looks like the following where the INFO,REDIRECT line is added.

2014-03-04T22:23:26.098Z HAPI,RECEIVED {"id":"1393971806098-16338-25189","method":"get","url":"/hapi","agent":"node/v0.10.26 linux x64"}
2014-03-04T22:23:26.476Z HAPI,HANDLER {"msec":377.4348230063915}
2014-03-04T22:23:26.488Z INFO,REDIRECT {"package":"hapi","path":"https://registry.npmjs.org/"}
2014-03-04T22:23:26.511Z HAPI,RESPONSE unspecified
2014-03-04T22:23:26.511Z INFO,REQUEST 127.0.0.1 GET /hapi 200

There is no change to logging for a read request package that was found in the first path or for write requests.

Owner

tlivings commented Apr 3, 2014

Hi, sorry this has taken so long to get back to this!

I think the additional logging is fine while we rethink our overall logging picture. I'll leave comments inline to the code.

@tlivings tlivings commented on the diff Apr 3, 2014

@@ -199,6 +199,13 @@ module.exports = {
plugin.events.on('response', function (req) {
plugin.log(['info', 'request'], [ req.info.remoteAddress, req.method.toUpperCase(), req.path, req.raw.res.statusCode ].join(' '));
+
+ var res = req.response,
@tlivings

tlivings Apr 3, 2014

Owner

Can these variables be hoisted to keep with our conventions?

i.e.

 plugin.events.on('response', function (req) {
    var res, path, pkg;

    res = req.response;
    path = res.headers && res.headers['x-registry'];
    pkg = req.params.p.split('/')[0];

...etc

@tlivings tlivings commented on the diff Apr 3, 2014

@@ -170,7 +170,7 @@ module.exports = {
rewrite = util.rewriter(host, registry);
util.transform(response.source, 'tarball', rewrite);
}
-
+
@tlivings

tlivings Apr 3, 2014

Owner

White space?

Owner

tlivings commented Oct 13, 2014

Closing this PR since a lot has changed around logging. See: #69

@tlivings tlivings closed this Oct 13, 2014

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