Skip to content

Commit

Permalink
feat: implement trace option like mysql client (#290)
Browse files Browse the repository at this point in the history
  • Loading branch information
killagu authored and fengmk2 committed Sep 26, 2018
1 parent dcf87cb commit b760530
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 3 deletions.
16 changes: 16 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ httpclient.request('http://nodejs.org', function (err, body) {
- ***proxy*** String | Object - proxy agent uri or options, default is `null`.
- ***lookup*** Function - Custom DNS lookup function, default is `dns.lookup`. Require node >= 4.0.0(for http protocol) and node >=8(for https protocol)
- ***checkAddress*** Function: optional, check request address to protect from SSRF and similar attacks. It receive tow arguments(`ip` and `family`) and should return true or false to identified the address is legal or not. It rely on `lookup` and have the same version requirement.
- ***trace*** Boolean - Enable capture stack include call site of library entrance, default is `false`.
- ***callback(err, data, res)*** Function - Optional callback.
- **err** Error - Would be `null` if no error accured.
- **data** Buffer | Object - The data responsed. Would be a Buffer if `dataType` is set to `text` or an JSON parsed into Object if it's set to `json`.
Expand Down Expand Up @@ -361,6 +362,21 @@ https_proxy=https://localhost:8008
$ http_proxy=http://localhost:8008 node index.js
```

### Trace
If set trace true, error stack will contains full call stack, like
```
Error: connect ECONNREFUSED 127.0.0.1:11
at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1113:14)
--------------------
at ~/workspace/urllib/lib/urllib.js:150:13
at new Promise (<anonymous>)
at Object.request (~/workspace/urllib/lib/urllib.js:149:10)
at Context.<anonymous> (~/workspace/urllib/test/urllib_promise.test.js:49:19)
....
```

When open the trace, urllib may have poor perfomance, please consider carefully.

## TODO

* [ ] Support component
Expand Down
52 changes: 52 additions & 0 deletions benchmark/trace.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict';

var Benchmark = require('benchmark');
var http = require('http');
var urllib = require('../');
var suite = new Benchmark.Suite();
var agent = new http.Agent({
keepAlive: true,
keepAliveMsecs: 5000,
maxSockets: 100,
timeout: 5000,
});

// start your nginx listen 8080 with hello,world

function request(trace, deferred) {
urllib.request('http://127.0.0.1:8080', {
timeout: 20000,
trace: trace,
agent: agent,
}).then(function() {
deferred.resolve();
});
}

suite
.add('warm up agent', {
defer: true,
fn: function(deferred) {
request(false, deferred);
}
})
.add('request with no trace', {
defer: true,
fn: function(deferred) {
request(false, deferred);
}
})
.add('request with trace', {
defer: true,
fn: function(deferred) {
request(true, deferred);
}
})
.on('cycle', function(event) {
console.log(String(event.target));
})
.on('complete', function() {
console.log('Fastest is ' + this.filter('fastest').map('name'));
})
// run async
.run({ 'async': true });
31 changes: 29 additions & 2 deletions lib/urllib.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ exports.agent.maxSockets = 1000;
exports.httpsAgent = new https.Agent();
exports.httpsAgent.maxSockets = 1000;

var LONG_STACK_DELIMITER = '\n --------------------\n';

/**
* The default request timeout(in milliseconds).
* @type {Number}
Expand Down Expand Up @@ -183,7 +185,7 @@ exports.requestThunk = function requestThunk(url, args) {
};
};

exports.requestWithCallback = function requestWithCallback(url, args, callback) {
function requestWithCallback(url, args, callback) {
// requestWithCallback(url, callback)
if (!url || (typeof url !== 'string' && typeof url !== 'object')) {
var msg = util.format('expect request url to be a string or a http request options, but got %j', url);
Expand Down Expand Up @@ -490,6 +492,7 @@ exports.requestWithCallback = function requestWithCallback(url, args, callback)
err.status = statusCode;
err.headers = headers;
err.res = response;
addLongStackTrace(err, req);
}

// only support agentkeepalive module for now
Expand Down Expand Up @@ -862,6 +865,10 @@ exports.requestWithCallback = function requestWithCallback(url, args, callback)
// request headers checker will throw error
try {
req = httplib.request(options, onResponse);
if (args.trace) {
req._callSite = {};
Error.captureStackTrace(req._callSite, requestWithCallback);
}
} catch (err) {
return done(err);
}
Expand Down Expand Up @@ -989,7 +996,9 @@ exports.requestWithCallback = function requestWithCallback(url, args, callback)

req.requestId = reqId;
return req;
};
}

exports.requestWithCallback = requestWithCallback;

var JSONCtlCharsMap = {
'"': '\\"', // \u0022
Expand Down Expand Up @@ -1080,3 +1089,21 @@ function parseContentType(str) {
return { parameters: {} };
}
}

function addLongStackTrace(err, req) {
if (!req) {
return;
}
var callSiteStack = req._callSite && req._callSite.stack;
if (!callSiteStack || typeof callSiteStack !== 'string') {
return;
}
if (err._longStack) {
return;
}
var index = callSiteStack.indexOf('\n');
if (index !== -1) {
err._longStack = true;
err.stack += LONG_STACK_DELIMITER + callSiteStack.substr(index + 1);
}
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
"devDependencies": {
"agentkeepalive": "^3.4.0",
"autod": "*",
"benchmark": "^2.1.4",
"bluebird": "*",
"busboy": "^0.2.14",
"co": "*",
Expand Down
30 changes: 29 additions & 1 deletion test/urllib_promise.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,16 @@ describe('test/urllib_promise.test.js', function () {
data: {
q: 'foo'
},
timeout: 20000
timeout: 20000,
trace: true
})
.then(function () {
throw new Error('should not run this');
}).catch(function (err) {
assert(err);
assert(err.code === 'ECONNREFUSED');
assert(err.status === -1);
assert(err.stack.indexOf('--------------------') > 0);
assert.deepEqual(err.headers, {});
assert.deepEqual(Object.keys(err.res), [
'status', 'statusCode', 'headers', 'size', 'aborted', 'rt',
Expand All @@ -66,4 +68,30 @@ describe('test/urllib_promise.test.js', function () {
]);
});
});

describe('disable trace',function () {
it('should not with long stack trace', function () {
return urllib.request('http://127.0.0.1:11', {
data: {
q: 'foo'
},
timeout: 20000,
trace: false
})
.then(function () {
throw new Error('should not run this');
}).catch(function (err) {
assert(err);
assert(err.code === 'ECONNREFUSED');
assert(err.status === -1);
assert(err.stack.indexOf('--------------------') < 0);
assert.deepEqual(err.headers, {});
assert.deepEqual(Object.keys(err.res), [
'status', 'statusCode', 'headers', 'size', 'aborted', 'rt',
'keepAliveSocket', 'data', 'requestUrls', 'timing', 'remoteAddress', 'remotePort',
'socketHandledRequests', 'socketHandledResponses',
]);
});
});
});
});

0 comments on commit b760530

Please sign in to comment.