Add support for .child() on clients with beforeSync and afterSync hooks. by joshwilsdon · Pull Request #77 · restify/clients

joshwilsdon · 2016-09-12T22:19:15Z

This change adds support for .child() on HttpClient, StringClient and JsonClient. Child creation takes an object that can have a .beforeSync() and/or .afterSync() function. These functions will be called before and after each request made through the resulting child client. This is especially useful for enabling distributed tracing for restify clients when using a restify server.

coveralls · 2016-09-12T22:20:57Z

Coverage increased (+0.4%) to 84.421% when pulling 683dc21 on joshwilsdon:upstream into 3bc24fe on restify:master.

micahr · 2016-09-13T16:43:52Z

Thank you for the PR, Josh. I'll take a look through the code, documentation and tests.

micahr · 2016-09-13T16:46:06Z

lib/HttpClient.js

+    }
+
+    req.on('result', function _onResult(res_err, res) {
+        res.on('end', function () {


Do you mind naming this function?

Not at all. Sorry I missed that one.

coveralls · 2016-09-13T17:13:53Z

Coverage increased (+0.4%) to 84.421% when pulling 6a3ac4d on joshwilsdon:upstream into 3bc24fe on restify:master.

coveralls · 2016-09-13T17:13:53Z

Coverage increased (+0.4%) to 84.421% when pulling 6a3ac4d on joshwilsdon:upstream into 3bc24fe on restify:master.

coveralls · 2016-09-27T07:07:26Z

Coverage increased (+0.4%) to 84.421% when pulling e5eb281 on joshwilsdon:upstream into 3bc24fe on restify:master.

joshwilsdon · 2016-09-28T22:43:44Z

I found an issue with this for a use-case where beforeSync and afterSync need to coordinate for an individual request. The issue is that there's no way to pass something from beforeSync to afterSync so that the two know they're related. If you have something like the following:

createChild(client, log) {
    var headers;

    client.child({
        beforeSync: function (opts) {
            headers=opts.headers;
        }, afterSync: function (err, req, res) {
            log.debug({headers: headers}, 'afterSync headers');
        }
    });
}

where client is a restifyclient, and you made two queries in parallel, what you'd see is that the 'afterSync headers' would be logged twice with the same data. That's because the beforeSync() would set headers, but the other beforeSync would also set headers in this scope. Then the two afterSync calls are called sharing the same headers.

In order to allow for this sort of use where you want beforeSync and afterSync to share data, I will be adding an additional ctx object parameter to both beforeSync and afterSync. This way you could modify the above to:

createChild(client, log) {
    client.child({
        beforeSync: function (opts, ctx) {
            ctx.headers=opts.headers;
        }, afterSync: function (err, req, res, ctx) {
            log.debug({headers: ctx.headers}, 'afterSync headers');
        }
    });
}

and it would work the way the caller originally intended.

coveralls · 2016-09-28T23:17:54Z

Coverage increased (+0.4%) to 84.441% when pulling 1be6c65 on joshwilsdon:upstream into 3bc24fe on restify:master.

coveralls · 2016-09-28T23:17:54Z

Coverage increased (+0.4%) to 84.441% when pulling 1be6c65 on joshwilsdon:upstream into 3bc24fe on restify:master.

yunong · 2016-09-29T00:26:27Z

Can you look into using continuation local storage to pass context around?

On Wednesday, September 28, 2016, Josh Wilsdon notifications@github.com
wrote:

I found an issue with this for a use-case where beforeSync and afterSync
need to coordinate for an individual request. The issue is that there's no
way to pass something from beforeSync to afterSync so that the two know
they're related. If you have something like the following:

createChild(client, log) {
var headers;
client.child({
    beforeSync: function (opts) {
        headers=opts.headers;
    }, afterSync: function (err, req, res) {
        log.debug({headers: headers}, 'afterSync headers');
    }
});
}

where client is a restifyclient, and you made two queries in parallel,
what you'd see is that the 'afterSync headers' would be logged twice with
the same data. That's because the beforeSync() would set headers, but the
other beforeSync would also set headers in this scope. Then the two
afterSync calls are called sharing the same headers.

In order to allow for this sort of use where you want beforeSync and
afterSync to share data, I will be adding an additional ctx object
parameter to both beforeSync and afterSync. This way you could modify the
above to:

createChild(client, log) {
client.child({
beforeSync: function (opts, ctx) {
ctx.headers=opts.headers;
}, afterSync: function (err, req, res, ctx) {
log.debug({headers: ctx.headers}, 'afterSync headers');
}
});
}

and it would work the way the caller originally intended.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#77 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABK2ENBSA1jdzR9k0JliihmT4Nt1ffEOks5quu2hgaJpZM4J7F-9
.

Via mobile

joshwilsdon · 2016-09-29T19:22:41Z

@yunong: sure, I'll look into that and report back.

yunong · 2016-09-29T19:32:23Z

Awesome!

On Thursday, September 29, 2016, Josh Wilsdon notifications@github.com
wrote:

@yunong https://github.com/yunong: sure, I'll look into that and report
back.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#77 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABK2EBUKVjeGyoFgLcuY6aGGBwyyTjboks5qvBACgaJpZM4J7F-9
.

Via mobile

joshwilsdon · 2016-10-03T21:06:12Z

@yunong: I've spent some time looking at continuation local storage and I now have a question for you. Was your intention here that we be able to avoid adding the ctx parameter to the beforeSync and afterSync methods because the caller would be able to pass in methods that were already wrapped and had continuation local storage available to them? Or did you mean to suggest that restify-clients should include continuation local storage to deal with this problem instead of ctx?

What I looked at was based on this example which demonstrates the original problem:

var restify = require('restify');
var restifyClients = require('./lib/index');
var vasync = require('vasync');
var uuid = require('node-uuid');

var PORT = 8080;

// generates [0..9]
var nums = Array(10+1).join('0').split('') .map(function (v, i) { return i; });

function respond(req, res, next) {
    setTimeout(function _respond() {
        res.send('hello ' + req.params.id);
        next();
    }, Math.random() * 100);
}

var client;
var server = restify.createServer();

server.get('/hello/:id', respond);

server.listen(PORT, function() {
    var child;
    var id;

    console.log('%s listening at %s', server.name, server.url);

    client = restifyClients.createJsonClient({url: server.url});
    child = client.child({
        beforeSync: function _beforeSync(opts) {
            id = uuid.v4();
            console.log('before for ' + opts.path + ' (' + id + ')');
        }, afterSync: function _afterSync(err, req, res) {
            console.log('after for ' + req.path + ' (' + id + ')');
        }
    });

    vasync.forEachParallel({
        func: function _doReq(id, cb) {
           child.get('/hello/' + id, function (err, req, res, obj) {
               cb(err);
           });
        }, inputs: nums
    }, function _done(err) {
        console.log('DONE');
        process.exit(0);
    });
});

when run that outputs something like:

restify listening at http://0.0.0.0:8080
before for /hello/0 (37d9514d-fed4-4c08-a777-96104049c121)
before for /hello/1 (955e7e1f-8ca1-4b5b-a749-763c2143d312)
before for /hello/2 (fbe85aaf-520e-4c31-a620-63c4f33a5e2e)
before for /hello/3 (4a190b58-4499-473d-bd43-80e986487d71)
before for /hello/4 (d9572eca-9c1f-4834-abde-2fa98d0c1fbf)
before for /hello/5 (75fa2857-f003-48d6-b72d-293fdadf5d6a)
before for /hello/6 (8fb596fd-6f98-4253-9596-5268ce04da27)
before for /hello/7 (e60b8994-b605-4e16-a0f8-8796df838a06)
before for /hello/8 (511fc00d-d83e-4de1-bbe1-a4327f4c3586)
before for /hello/9 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/0 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/1 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/2 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/3 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/5 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/4 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/7 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/6 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/9 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/8 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
DONE

Whereas changing to use the ctx here looks like:

var restify = require('restify');
var restifyClients = require('./lib/index');
var vasync = require('vasync');
var uuid = require('node-uuid');

var PORT = 8080;

// generates [0..9]
var nums = Array(10+1).join('0').split('') .map(function (v, i) { return i; });

function respond(req, res, next) {
    setTimeout(function _respond() {
        res.send('hello ' + req.params.id);
        next();
    }, Math.random() * 100);
}

var client;
var server = restify.createServer();

server.get('/hello/:id', respond);

server.listen(PORT, function() {
    var child;

    console.log('%s listening at %s', server.name, server.url);

    client = restifyClients.createJsonClient({url: server.url});
    child = client.child({
        beforeSync: function _beforeSync(opts, ctx) {
            ctx.id = uuid.v4();
            console.log('before for ' + opts.path + ' (' + ctx.id + ')');
        }, afterSync: function _afterSync(err, req, res, ctx) {
            console.log('after for ' + req.path + ' (' + ctx.id + ')');
        }
    });

    vasync.forEachParallel({
        func: function _doReq(id, cb) {
           child.get('/hello/' + id, function (err, req, res, obj) {
               cb(err);
           });
        }, inputs: nums
    }, function _done(err) {
        console.log('done');
        process.exit(0);
    });
});

and produces output closer that looks like what one might expect:

restify listening at http://0.0.0.0:8080
before for /hello/0 (80a4a022-c087-48d7-9f00-831038aff21b)
before for /hello/1 (138a4b65-5c6a-44b5-bd12-1ed5f7c00959)
before for /hello/2 (df87b470-d811-472d-aa3f-5ed155e19ae5)
before for /hello/3 (783f8379-e986-4fda-9f26-71968b270e2d)
before for /hello/4 (620c4638-c6b4-4dd4-9dc9-0510b271a7c7)
before for /hello/5 (7b943c95-d87d-4a51-9fba-b574d9f5ebb2)
before for /hello/6 (c870f2d8-4138-4c06-81cc-c838b5b8ba90)
before for /hello/7 (7f7b682e-93a1-4b65-8cf7-c42f269e70ec)
before for /hello/8 (69fa4dc1-cb8f-411a-b379-7675282d2cff)
before for /hello/9 (74ea6a7b-c82b-423c-a77c-259b2d0de6df)
after for /hello/4 (620c4638-c6b4-4dd4-9dc9-0510b271a7c7)
after for /hello/1 (138a4b65-5c6a-44b5-bd12-1ed5f7c00959)
after for /hello/2 (df87b470-d811-472d-aa3f-5ed155e19ae5)
after for /hello/5 (7b943c95-d87d-4a51-9fba-b574d9f5ebb2)
after for /hello/6 (c870f2d8-4138-4c06-81cc-c838b5b8ba90)
after for /hello/3 (783f8379-e986-4fda-9f26-71968b270e2d)
after for /hello/0 (80a4a022-c087-48d7-9f00-831038aff21b)
after for /hello/7 (7f7b682e-93a1-4b65-8cf7-c42f269e70ec)
after for /hello/8 (69fa4dc1-cb8f-411a-b379-7675282d2cff)
after for /hello/9 (74ea6a7b-c82b-423c-a77c-259b2d0de6df)
done

The same code also works if I swap in continuation-local-storage and don't use ctx:

var cls = require('continuation-local-storage');
var restify = require('restify');
var restifyClients = require('./lib/index');
var vasync = require('vasync');
var uuid = require('node-uuid');

var PORT = 8080;

// generates [0..9]
var nums = Array(10+1).join('0').split('') .map(function (v, i) { return i; });

function respond(req, res, next) {
    setTimeout(function _respond() {
        res.send('hello ' + req.params.id);
        next();
    }, Math.random() * 100);
}

var client;
var server = restify.createServer();
var storagebag = cls.createNamespace('my bag');

server.get('/hello/:id', respond);

server.listen(PORT, function() {
    var child;

    console.log('%s listening at %s', server.name, server.url);
    client = restifyClients.createJsonClient({url: server.url});

    storagebag.run(function () {
    });

    child = client.child({
        beforeSync: function _beforeSync(opts) {
            var id = uuid.v4();

            storagebag.set('id', id);
            console.log('before for ' + opts.path + ' (' + id + ')');
        }, afterSync: function _afterSync(err, req, res) {
            var id = storagebag.get('id');

            console.log('after for ' + req.path + ' (' + id + ')');
        }
    });

    vasync.forEachParallel({
        func: function _doReq(id, cb) {
           storagebag.run(function () {
               child.get('/hello/' + id, function (err, req, res, obj) {
                   cb(err);
               });
           });
        }, inputs: nums
    }, function _done(err) {
        console.log('done');
        process.exit(0);
    });
});

so this does work, it just pushes a bit more complexity up to the caller. It also would cause problems for post-mortem debugging, at least the standard continuation-local-storage module which uses a try {} when one calls namespace.run() here: https://github.com/othiym23/node-continuation-local-storage/blob/1ecb99ccf30e1699a84627a961f1900c871c338a/context.js#L47-L50 . It would of course be possible to fork or create an alternate implementation without that problem.

Adding ctx parameters here also doesn't prevent one from using continuation local storage either. In that case the parameter can just be left out when writing the beforeSync and afterSync functions.

So I guess my question boils down to: is there a reason you'd not like to have the ctx parameter here that offsets the additional complexity a caller needs here in order to pass data from beforeSync to the afterSync for the same request? If so: I'll revert the ctx change here and deal with this problem in the code I'm writing that calls restify-clients and uses this feature.

yunong · 2016-10-06T13:38:31Z

Hey Josh,

I'm out of the country at this moment -- mind if we pick this up next week?

-Y

On 4 October 2016 at 05:06, Josh Wilsdon notifications@github.com wrote:

@yunong https://github.com/yunong: I've spent some time looking at
continuation local storage and I now have a question for you. Was your
intention here that we be able to avoid adding the ctx parameter to the
beforeSync and afterSync methods because the caller would be able to pass
in methods that were already wrapped and had continuation local storage
available to them? Or did you mean to suggest that restify-clients should
include continuation local storage to deal with this problem instead of
ctx?

What I looked at was based on this example which demonstrates the original
problem:

var restify = require('restify');
var restifyClients = require('./lib/index');
var vasync = require('vasync');
var uuid = require('node-uuid');

var PORT = 8080;

// generates [0..9]
var nums = Array(10+1).join('0').split('') .map(function (v, i) { return i; });

function respond(req, res, next) {
setTimeout(function _respond() {
res.send('hello ' + req.params.id);
next();
}, Math.random() * 100);
}

var client;
var server = restify.createServer();

server.get('/hello/:id', respond);

server.listen(PORT, function() {
var child;
var id;
console.log('%s listening at %s', server.name, server.url);

client = restifyClients.createJsonClient({url: server.url});
child = client.child({
    beforeSync: function _beforeSync(opts) {
        id = uuid.v4();
        console.log('before for ' + opts.path + ' (' + id + ')');
    }, afterSync: function _afterSync(err, req, res) {
        console.log('after for ' + req.path + ' (' + id + ')');
    }
});

vasync.forEachParallel({
    func: function _doReq(id, cb) {
       child.get('/hello/' + id, function (err, req, res, obj) {
           cb(err);
       });
    }, inputs: nums
}, function _done(err) {
    console.log('DONE');
    process.exit(0);
});
});

when run that outputs something like:

restify listening at http://0.0.0.0:8080
before for /hello/0 (37d9514d-fed4-4c08-a777-96104049c121)
before for /hello/1 (955e7e1f-8ca1-4b5b-a749-763c2143d312)
before for /hello/2 (fbe85aaf-520e-4c31-a620-63c4f33a5e2e)
before for /hello/3 (4a190b58-4499-473d-bd43-80e986487d71)
before for /hello/4 (d9572eca-9c1f-4834-abde-2fa98d0c1fbf)
before for /hello/5 (75fa2857-f003-48d6-b72d-293fdadf5d6a)
before for /hello/6 (8fb596fd-6f98-4253-9596-5268ce04da27)
before for /hello/7 (e60b8994-b605-4e16-a0f8-8796df838a06)
before for /hello/8 (511fc00d-d83e-4de1-bbe1-a4327f4c3586)
before for /hello/9 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/0 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/1 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/2 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/3 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/5 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/4 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/7 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/6 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/9 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
after for /hello/8 (a4f760ba-8654-4db0-ab11-09832d8c1faf)
DONE

Whereas changing to use the ctx here looks like:

var restify = require('restify');
var restifyClients = require('./lib/index');
var vasync = require('vasync');
var uuid = require('node-uuid');

var PORT = 8080;

// generates [0..9]
var nums = Array(10+1).join('0').split('') .map(function (v, i) { return i; });

function respond(req, res, next) {
setTimeout(function _respond() {
res.send('hello ' + req.params.id);
next();
}, Math.random() * 100);
}

var client;
var server = restify.createServer();

server.get('/hello/:id', respond);

server.listen(PORT, function() {
var child;
console.log('%s listening at %s', server.name, server.url);

client = restifyClients.createJsonClient({url: server.url});
child = client.child({
    beforeSync: function _beforeSync(opts, ctx) {
        ctx.id = uuid.v4();
        console.log('before for ' + opts.path + ' (' + ctx.id + ')');
    }, afterSync: function _afterSync(err, req, res, ctx) {
        console.log('after for ' + req.path + ' (' + ctx.id + ')');
    }
});

vasync.forEachParallel({
    func: function _doReq(id, cb) {
       child.get('/hello/' + id, function (err, req, res, obj) {
           cb(err);
       });
    }, inputs: nums
}, function _done(err) {
    console.log('done');
    process.exit(0);
});
});

and produces output closer that looks like what one might expect:

restify listening at http://0.0.0.0:8080
before for /hello/0 (80a4a022-c087-48d7-9f00-831038aff21b)
before for /hello/1 (138a4b65-5c6a-44b5-bd12-1ed5f7c00959)
before for /hello/2 (df87b470-d811-472d-aa3f-5ed155e19ae5)
before for /hello/3 (783f8379-e986-4fda-9f26-71968b270e2d)
before for /hello/4 (620c4638-c6b4-4dd4-9dc9-0510b271a7c7)
before for /hello/5 (7b943c95-d87d-4a51-9fba-b574d9f5ebb2)
before for /hello/6 (c870f2d8-4138-4c06-81cc-c838b5b8ba90)
before for /hello/7 (7f7b682e-93a1-4b65-8cf7-c42f269e70ec)
before for /hello/8 (69fa4dc1-cb8f-411a-b379-7675282d2cff)
before for /hello/9 (74ea6a7b-c82b-423c-a77c-259b2d0de6df)
after for /hello/4 (620c4638-c6b4-4dd4-9dc9-0510b271a7c7)
after for /hello/1 (138a4b65-5c6a-44b5-bd12-1ed5f7c00959)
after for /hello/2 (df87b470-d811-472d-aa3f-5ed155e19ae5)
after for /hello/5 (7b943c95-d87d-4a51-9fba-b574d9f5ebb2)
after for /hello/6 (c870f2d8-4138-4c06-81cc-c838b5b8ba90)
after for /hello/3 (783f8379-e986-4fda-9f26-71968b270e2d)
after for /hello/0 (80a4a022-c087-48d7-9f00-831038aff21b)
after for /hello/7 (7f7b682e-93a1-4b65-8cf7-c42f269e70ec)
after for /hello/8 (69fa4dc1-cb8f-411a-b379-7675282d2cff)
after for /hello/9 (74ea6a7b-c82b-423c-a77c-259b2d0de6df)
done

The same code also works if I swap in continuation-local-storage and
don't use ctx:

var cls = require('continuation-local-storage');
var restify = require('restify');
var restifyClients = require('./lib/index');
var vasync = require('vasync');
var uuid = require('node-uuid');

var PORT = 8080;

// generates [0..9]
var nums = Array(10+1).join('0').split('') .map(function (v, i) { return i; });

function respond(req, res, next) {
setTimeout(function _respond() {
res.send('hello ' + req.params.id);
next();
}, Math.random() * 100);
}

var client;
var server = restify.createServer();
var storagebag = cls.createNamespace('my bag');

server.get('/hello/:id', respond);

server.listen(PORT, function() {
var child;
console.log('%s listening at %s', server.name, server.url);
client = restifyClients.createJsonClient({url: server.url});

storagebag.run(function () {
});

child = client.child({
    beforeSync: function _beforeSync(opts) {
        var id = uuid.v4();

        storagebag.set('id', id);
        console.log('before for ' + opts.path + ' (' + id + ')');
    }, afterSync: function _afterSync(err, req, res) {
        var id = storagebag.get('id');

        console.log('after for ' + req.path + ' (' + id + ')');
    }
});

vasync.forEachParallel({
    func: function _doReq(id, cb) {
       storagebag.run(function () {
           child.get('/hello/' + id, function (err, req, res, obj) {
               cb(err);
           });
       });
    }, inputs: nums
}, function _done(err) {
    console.log('done');
    process.exit(0);
});
});

so this does work, it just pushes a bit more complexity up to the
caller. It also would cause problems for post-mortem debugging, at least
the standard continuation-local-storage module which uses a try {} when
one calls namespace.run() here: https://github.com/othiym23/
node-continuation-local-storage/blob/1ecb99ccf30e1699a84627a961f190
0c871c338a/context.js#L47-L50 . It would of course be possible to fork or
create an alternate implementation without that problem.

Adding ctx parameters here also doesn't prevent one from using
continuation local storage either. In that case the parameter can just be
left out when writing the beforeSync and afterSync functions.

So I guess my question boils down to: is there a reason you'd not like to
have the ctx parameter here that offsets the additional complexity a
caller needs here in order to pass data from beforeSync to the afterSync
for the same request? If so: I'll revert the ctx change here and deal
with this problem in the code I'm writing that calls restify-clients and
uses this feature.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#77 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABK2ED8ZoN5uUyfDXQ-LEr13BjTZgKQbks5qwW5FgaJpZM4J7F-9
.

joshwilsdon · 2016-10-07T19:03:00Z

@yunong: sure, no problem.

coveralls · 2016-10-12T07:14:21Z

Coverage increased (+0.3%) to 84.371% when pulling 16f294d on joshwilsdon:upstream into 3bc24fe on restify:master.

coveralls · 2016-10-12T07:14:21Z

Coverage increased (+0.3%) to 84.371% when pulling 16f294d on joshwilsdon:upstream into 3bc24fe on restify:master.

joshwilsdon · 2016-10-18T17:27:13Z

@yunong: I've been experimenting some more with cls and I really like the idea of using it in the abstract. Thanks for pointing me at that. I'm still working on trying to figure out a way to use it that avoids monkey-patching all of node core and avoids breaking postmortem debugging. I'll continue prototyping and let you know when I have something further for discussion on this PR.

yunong · 2016-10-18T18:03:21Z

@joshwilsdon 👍

joshwilsdon · 2016-11-04T22:50:52Z

@yunong: Sorry for the delay getting back to you.

I've spent more time looking into this and banging my head against it and
figured out a few things:

CLS involves a large number of complicated monkey patches on node
versions < v4.5
CLS on node versions that are currently released (including v4.6.1 and
v6.9.1) have a pretty serious bug with regard to the HTTP parser in node that
means you can't reliably use it across HTTP Parser boundaries. See:
https://github.com/misterdjules/repro-async-wrap-http-parser-duplicate-id
for some details (and links) on this issue.

This last point means that the only workable solution for either of the current
LTS releases, is to pass data from the before hook to the after hook using
an object instead of CLS. It's quite possible to pass data into your before
hook using CLS however and this works quite well for avoiding the need for the
.child() methods I have in this PR.

Meanwhile I also spent some time thinking about this implementation and realized
that if retries were done within the HttpClient.request(), we'd not see those
individual requests in traces. As such, I decided that it seemed more logical to
have the before and after wrapped around the rawRequest instead.

In making the change above, I've modified the API so that:

you pass before and after hooks to the create*Client functions directly
(no more .child())
the hooks are now async and require that you call their next callbacks
an object passed to the before hook's next callback will be passed into
the after hook as an argument.
the before and after hooks wrap rawRequest and can therefore be run
multiple times for a given .request().

Since this is such a major change over what's in this current PR, I will now
close this one and have opened a new PR (#95) that implements
this new API.

If in the future the issues in node are fixed, users will be able to stop
passing the ctx parameter to the callback in their before hooks and will be
able to get the data using CLS instead, but I don't think having the option to
also use this without CLS seems that onerous. Would welcome your thoughts and
continuing discussion over in that new PR.

Add support for .child() on clients with beforeSync and afterSync hooks.

683dc21

micahr reviewed Sep 13, 2016
View reviewed changes

name the _onEnd function

6a3ac4d

make sure we only call afterSync once

e5eb281

Add ctx parameter to both beforeSync and afterSync to allow sharing.

1be6c65

don't use res.once() when res is undefined

16f294d

joshwilsdon mentioned this pull request Nov 4, 2016

add support for before/after hooks to support tracing outbound requests #95

Closed

joshwilsdon closed this Nov 4, 2016

Conversation

joshwilsdon commented Sep 12, 2016

Uh oh!

coveralls commented Sep 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

micahr commented Sep 13, 2016

Uh oh!

micahr Sep 13, 2016

Choose a reason for hiding this comment

Uh oh!

joshwilsdon Sep 13, 2016

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 13, 2016

Uh oh!

coveralls commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshwilsdon commented Sep 28, 2016

Uh oh!

coveralls commented Sep 28, 2016

Uh oh!

coveralls commented Sep 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yunong commented Sep 29, 2016

Uh oh!

joshwilsdon commented Sep 29, 2016

Uh oh!

yunong commented Sep 29, 2016

Uh oh!

joshwilsdon commented Oct 3, 2016

Uh oh!

yunong commented Oct 6, 2016

Uh oh!

joshwilsdon commented Oct 7, 2016

Uh oh!

coveralls commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 12, 2016

Uh oh!

joshwilsdon commented Oct 18, 2016

Uh oh!

yunong commented Oct 18, 2016

Uh oh!

joshwilsdon commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Sep 12, 2016 •

edited

Loading

coveralls commented Sep 13, 2016 •

edited

Loading

coveralls commented Sep 27, 2016 •

edited

Loading

coveralls commented Sep 28, 2016 •

edited

Loading

coveralls commented Oct 12, 2016 •

edited

Loading

joshwilsdon commented Nov 4, 2016 •

edited

Loading