Skip to content
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

Merged
merged 6 commits into from
Jul 19, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
"fix-prettier": "prettier --single-quote --trailing-comma es5 --print-width 120 --write {bin,lib,probes,tests}/**/*.js *.js",
"fix-eslint": "eslint --fix {bin,lib,probes,tests}/**/*.js *.js",
"pretest": "eslint .",
"test": "tap --reporter tap --timeout=120 tests/*tests.js tests/probes/http-outbound-probe-test.js tests/probes/http-probe-test.js tests/headless_test.js",
"travis": "tap --reporter tap --timeout=120 tests/*tests.js tests/probes/http-outbound-probe-test.js tests/probes/http-probe-test.js tests/headless_test.js --coverage",
"test": "tap --reporter tap --timeout=120 tests/*tests.js tests/probes/http*test.js tests/headless_test.js",
"travis": "tap --reporter tap --timeout=120 tests/*tests.js tests/probes/http*test.js tests/headless_test.js --coverage",
"posttravis": "./get_code_cov.sh && tap --coverage-report=lcov && codecov --disable=gcov",
"install": "node extract_all_binaries.js || node-gyp rebuild"
},
Expand Down
210 changes: 210 additions & 0 deletions probes/https-outbound-probe.js
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');
Copy link
Contributor

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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.


var methods;
if (semver.lt(process.version, '8.0.0')) {
methods = ['request'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https.get() exists in older versions. Is there a reason for the conditional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The 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') {
Copy link
Contributor

Choose a reason for hiding this comment

The 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') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (options !== null && typeof options === 'object') {?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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') {
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callling url.parse() before and after is arguably a little wasteful, it's not cheap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 url.parse() on metrics end.

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;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this differ from url.format()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if res.headers['content-type'] === undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then contentType gets set to undefined, which I think is ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe should be : undefined. right now, if there is no res.headers, contentType is a string 'undefined', if there are res.headers, but no 'content-type', then its undefined (not a string), seems inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise.

requestHeaders: headers,
});
};

module.exports = HttpsOutboundProbe;
157 changes: 157 additions & 0 deletions probes/https-probe.js
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just function parse(url) {?

['?', '#'].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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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'),
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put braces around the body for legibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mutates newConfig. Is that something callers expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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).
As a side note it looks like we don't have any tests for URL filtering so that would be good to do.

for (var prop in newConfig) {
if (typeof newConfig[prop] !== 'undefined') {
this.config[prop] = newConfig[prop];
}
}
};

module.exports = HttpsProbe;
Loading