-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add HTTPS probes and tests #455
Changes from 3 commits
2d78fcf
c895086
13a7285
a0c2326
e10f9ec
a1b521d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,210 @@ | ||
/******************************************************************************* | ||
* Copyright 2017 IBM Corp. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*******************************************************************************/ | ||
'use strict'; | ||
|
||
var Probe = require('../lib/probe.js'); | ||
var aspect = require('../lib/aspect.js'); | ||
var request = require('../lib/request.js'); | ||
var util = require('util'); | ||
var url = require('url'); | ||
var am = require('../'); | ||
var semver = require('semver'); | ||
|
||
var methods; | ||
if (semver.lt(process.version, '8.0.0')) { | ||
methods = ['request']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd perhaps add an explaining comment, it's not clear from context why node 8 is special. |
||
} else { | ||
methods = ['request', 'get']; | ||
} | ||
|
||
// Probe to instrument outbound https requests | ||
|
||
function HttpsOutboundProbe() { | ||
Probe.call(this, 'https'); // match the name of the module we're instrumenting | ||
} | ||
util.inherits(HttpsOutboundProbe, Probe); | ||
|
||
HttpsOutboundProbe.prototype.attach = function(name, target) { | ||
var that = this; | ||
if (name == 'https') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (target.__outboundProbeAttached__) return target; | ||
target.__outboundProbeAttached__ = true; | ||
|
||
aspect.around( | ||
target, | ||
methods, | ||
// Before 'https.request' function | ||
function(obj, methodName, methodArgs, probeData) { | ||
// Get HTTPS request method from options | ||
var options = methodArgs[0]; | ||
var requestMethod = 'GET'; | ||
var urlRequested = ''; | ||
var headers = ''; | ||
if (typeof options === 'object') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just googled this - I didn't realise typeof null was 'object'... |
||
urlRequested = formatURL(options); | ||
if (options.method) { | ||
requestMethod = options.method; | ||
} | ||
if (options.headers) { | ||
headers = options.headers; | ||
} | ||
} else if (typeof options === 'string') { | ||
urlRequested = options; | ||
var parsedOptions = url.parse(options); | ||
if (parsedOptions.method) { | ||
requestMethod = parsedOptions.method; | ||
} | ||
if (parsedOptions.headers) { | ||
headers = parsedOptions.headers; | ||
} | ||
} | ||
|
||
// Start metrics | ||
that.metricsProbeStart(probeData, requestMethod, urlRequested); | ||
that.requestProbeStart(probeData, requestMethod, urlRequested); | ||
|
||
// End metrics | ||
aspect.aroundCallback( | ||
methodArgs, | ||
probeData, | ||
function(target, args, probeData) { | ||
methodArgs.statusCode = args[0].statusCode; | ||
that.metricsProbeEnd(probeData, requestMethod, urlRequested, args[0], headers); | ||
that.requestProbeEnd(probeData, requestMethod, urlRequested, args[0], headers); | ||
}, | ||
function(target, args, probeData, ret) { | ||
// Don't need to do anything after the callback | ||
return ret; | ||
} | ||
); | ||
}, | ||
// After 'https.request' function returns | ||
function(target, methodName, methodArgs, probeData, rc) { | ||
// If no callback has been used then end the metrics after returning from the method instead | ||
if (aspect.findCallbackArg(methodArgs) == undefined) { | ||
// Need to get request method and URL again | ||
var options = methodArgs[0]; | ||
var requestMethod = 'GET'; | ||
var urlRequested = ''; | ||
var headers = ''; | ||
if (typeof options === 'object') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise. |
||
urlRequested = formatURL(options); | ||
if (options.method) { | ||
requestMethod = options.method; | ||
} | ||
if (options.headers) { | ||
headers = options.headers; | ||
} | ||
} else if (typeof options === 'string') { | ||
urlRequested = options; | ||
var parsedOptions = url.parse(options); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Callling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll look into this. Metrics mode definitely doesn't need it but requests seem to need a name. We might be able to put a dummy name in though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I rewrote this bit in both http-outbound-probe and https-outbound-probe. They're both now only calling |
||
if (parsedOptions.method) { | ||
requestMethod = parsedOptions.method; | ||
} | ||
if (parsedOptions.headers) { | ||
headers = parsedOptions.headers; | ||
} | ||
} | ||
|
||
// End metrics (no response available so pass empty object) | ||
that.metricsProbeEnd(probeData, requestMethod, urlRequested, {}, headers); | ||
that.requestProbeEnd(probeData, requestMethod, urlRequested, {}, headers); | ||
} | ||
return rc; | ||
} | ||
); | ||
} | ||
return target; | ||
}; | ||
|
||
// Get a URL as a string from the options object passed to https.get or https.request | ||
// See https://nodejs.org/api/http.html#http_http_request_options_callback | ||
function formatURL(httpsOptions) { | ||
var url; | ||
if (httpsOptions.protocol) { | ||
url = httpsOptions.protocol; | ||
} else { | ||
url = 'https:'; | ||
} | ||
url += '//'; | ||
if (httpsOptions.auth) { | ||
url += httpsOptions.auth + '@'; | ||
} | ||
if (httpsOptions.host) { | ||
url += httpsOptions.host; | ||
} else if (httpsOptions.hostname) { | ||
url += httpsOptions.host; | ||
} else { | ||
url += 'localhost'; | ||
} | ||
if (httpsOptions.port) { | ||
url += ':' + httpsOptions.port; | ||
} | ||
if (httpsOptions.path) { | ||
url += httpsOptions.path; | ||
} else { | ||
url += '/'; | ||
} | ||
return url; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this differ from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It strips off trailing ? or # components, which we don't use. |
||
|
||
/* | ||
* Lightweight metrics probe for HTTPS requests | ||
* | ||
* These provide: | ||
* time: time event started | ||
* method: HTTPS method, eg. GET, POST, etc | ||
* url: The url requested | ||
* requestHeaders: The HTTPS headers for the request | ||
* duration: The time for the request to respond | ||
* contentType: HTTPS content-type | ||
* statusCode: HTTPS status code | ||
*/ | ||
HttpsOutboundProbe.prototype.metricsEnd = function(probeData, method, url, res, headers) { | ||
if (probeData && probeData.timer) { | ||
probeData.timer.stop(); | ||
am.emit('https-outbound', { | ||
time: probeData.timer.startTimeMillis, | ||
method: method, | ||
url: url, | ||
duration: probeData.timer.timeDelta, | ||
statusCode: res.statusCode, | ||
contentType: res.headers ? res.headers['content-type'] : 'undefined', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then contentType gets set to undefined, which I think is ok There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, makes sense |
||
requestHeaders: headers, | ||
}); | ||
} | ||
}; | ||
|
||
/* | ||
* Heavyweight request probes for HTTPS outbound requests | ||
*/ | ||
HttpsOutboundProbe.prototype.requestStart = function(probeData, method, url) { | ||
var reqType = 'https-outbound'; | ||
// Do not mark as a root request | ||
probeData.req = request.startRequest(reqType, url, false, probeData.timer); | ||
}; | ||
|
||
HttpsOutboundProbe.prototype.requestEnd = function(probeData, method, url, res, headers) { | ||
if (probeData && probeData.req) | ||
probeData.req.stop({ | ||
url: url, | ||
statusCode: res.statusCode, | ||
contentType: res.headers ? res.headers['content-type'] : 'undefined', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise. |
||
requestHeaders: headers, | ||
}); | ||
}; | ||
|
||
module.exports = HttpsOutboundProbe; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
/******************************************************************************* | ||
* Copyright 2015 IBM Corp. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*******************************************************************************/ | ||
'use strict'; | ||
|
||
var Probe = require('../lib/probe.js'); | ||
var aspect = require('../lib/aspect.js'); | ||
var request = require('../lib/request.js'); | ||
var util = require('util'); | ||
var am = require('../'); | ||
|
||
function HttpsProbe() { | ||
Probe.call(this, 'https'); | ||
this.config = { | ||
filters: [], | ||
}; | ||
} | ||
util.inherits(HttpsProbe, Probe); | ||
|
||
HttpsProbe.prototype.attach = function(name, target) { | ||
var that = this; | ||
if (name == 'https') { | ||
if (target.__probeAttached__) return target; | ||
target.__probeAttached__ = true; | ||
var methods = ['on', 'addListener']; | ||
|
||
aspect.before(target.Server.prototype, methods, function(obj, methodName, args, probeData) { | ||
if (args[0] !== 'request') return; | ||
if (obj.__httpsProbe__) return; | ||
obj.__httpsProbe__ = true; | ||
aspect.aroundCallback(args, probeData, function(obj, args, probeData) { | ||
var httpsReq = args[0]; | ||
var res = args[1]; | ||
// Filter out urls where filter.to is '' | ||
var traceUrl = that.filterUrl(httpsReq); | ||
if (traceUrl !== '') { | ||
that.metricsProbeStart(probeData, httpsReq.method, traceUrl); | ||
that.requestProbeStart(probeData, httpsReq.method, traceUrl); | ||
aspect.after(res, 'end', probeData, function(obj, methodName, args, probeData, ret) { | ||
that.metricsProbeEnd(probeData, httpsReq.method, traceUrl, res, httpsReq); | ||
that.requestProbeEnd(probeData, httpsReq.method, traceUrl, res, httpsReq); | ||
}); | ||
} | ||
}); | ||
}); | ||
} | ||
return target; | ||
}; | ||
|
||
/* | ||
* Custom req.url parser that strips out any trailing query | ||
*/ | ||
var parse = function(url) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just |
||
['?', '#'].forEach(function(separator) { | ||
var index = url.indexOf(separator); | ||
if (index !== -1) url = url.substring(0, index); | ||
}); | ||
return url; | ||
}; | ||
|
||
/* | ||
* Ignore requests for URLs which we've been configured via regex to ignore | ||
*/ | ||
HttpsProbe.prototype.filterUrl = function(req) { | ||
var resultUrl = parse(req.url); | ||
var filters = this.config.filters; | ||
if (filters.length == 0) return resultUrl; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
var identifier = req.method + ' ' + resultUrl; | ||
for (var i = 0; i < filters.length; ++i) { | ||
var filter = filters[i]; | ||
if (filter.regex.test(identifier)) { | ||
return filter.to; | ||
} | ||
} | ||
return resultUrl; | ||
}; | ||
|
||
/* | ||
* Lightweight metrics probe for HTTPS requests | ||
* | ||
* These provide: | ||
* time: time event started | ||
* method: HTTPS method, eg. GET, POST, etc | ||
* url: The url requested | ||
* duration: the time for the request to respond | ||
*/ | ||
|
||
HttpsProbe.prototype.metricsEnd = function(probeData, method, url, res, httpsReq) { | ||
if (probeData && probeData.timer) { | ||
probeData.timer.stop(); | ||
am.emit('https', { | ||
time: probeData.timer.startTimeMillis, | ||
method: method, | ||
url: url, | ||
duration: probeData.timer.timeDelta, | ||
header: res._header, | ||
statusCode: res.statusCode, | ||
contentType: res.getHeader('content-type'), | ||
requestHeader: httpsReq.headers, | ||
}); | ||
} | ||
}; | ||
|
||
/* | ||
* Heavyweight request probes for HTTPS requests | ||
*/ | ||
|
||
HttpsProbe.prototype.requestStart = function(probeData, method, url) { | ||
var reqType = 'https'; | ||
// Mark as a root request as this happens due to an external event | ||
probeData.req = request.startRequest(reqType, url, true, probeData.timer); | ||
}; | ||
|
||
HttpsProbe.prototype.requestEnd = function(probeData, method, url, res, httpsReq) { | ||
if (probeData && probeData.req) | ||
probeData.req.stop({ | ||
url: url, | ||
method: method, | ||
requestHeader: httpsReq.headers, | ||
statusCode: res.statusCode, | ||
header: res._header, | ||
contentType: res.getHeader('content-type'), | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd put braces around the body for legibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
}; | ||
|
||
/* | ||
* Set configuration by merging passed in config with current one | ||
*/ | ||
HttpsProbe.prototype.setConfig = function(newConfig) { | ||
if (typeof newConfig.filters !== 'undefined') { | ||
newConfig.filters.forEach(function(filter) { | ||
if (typeof filter.regex === 'undefined') { | ||
filter.regex = new RegExp(filter.pattern); | ||
} | ||
}); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This mutates There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think this is ok, it's because there's more than one way of setting config options and this merges them (copied from existing http-probe). |
||
for (var prop in newConfig) { | ||
if (typeof newConfig[prop] !== 'undefined') { | ||
this.config[prop] = newConfig[prop]; | ||
} | ||
} | ||
}; | ||
|
||
module.exports = HttpsProbe; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer requires to be sorted alphabetically (by line) rather being in some random order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but don't know if that's part of the appmetrics coding style)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I like to group related requires together, e.g. all local requires together and any chained requires together. Then alphabetical after that. This is not my ordering though (copied) so I will improve it a bit so that it looks less random.
We don't really have a coding style beyond what eslint does and some naming/case stuff. I wouldn't like to enforce something like alphabetical ordering with eslint or otherwise because ordering can change behaviour, e.g. appmetrics itself usually needs to be required first.