Add HTTPS probes and tests #455

sjanuary · 2017-06-15T09:17:46Z

Fixes #409

sjanuary · 2017-06-15T09:49:01Z

Just investigating test failures on Node 8...

gibfahn · 2017-06-15T10:25:09Z

Just investigating test failures on Node 8...

Make sure you're not using 8.1.1 (and if you are look for nodejs/node#13667)

codecov-io · 2017-06-15T10:44:09Z

Codecov Report

Merging #455 into master will increase coverage by 2.02%.
The diff coverage is 86.2%.

@@           Coverage Diff            @@
##           master   #455      +/-   ##
========================================
+ Coverage   57.97%    60%   +2.02%     
========================================
  Files          44     49       +5     
  Lines        2967   3195     +228     
========================================
+ Hits         1720   1917     +197     
- Misses       1247   1278      +31

Impacted Files	Coverage Δ
tests/probes/https-outbound-probe-test.js	`100% <100%> (ø)`
tests/probes/https-probe-test.js	`100% <100%> (ø)`
tests/probes/http-outbound-probe-test.js	`100% <100%> (ø)`	⬆️
tests/test_https_server.js	`100% <100%> (ø)`
tests/probes/http-probe-test.js	`67.39% <100%> (ø)`	⬆️
probes/http-probe.js	`79.03% <100%> (-0.34%)`	⬇️
probes/http-outbound-probe.js	`82.41% <68.18%> (-4.54%)`	⬇️
probes/https-probe.js	`79.03% <79.03%> (ø)`
probes/https-outbound-probe.js	`81.81% <81.81%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68ff88b...a1b521d. Read the comment docs.

sjanuary · 2017-06-15T10:48:58Z

Test failure was just a timing issue, the test was closing the server after sending requests but before they had reached it. Now fixed.

sjanuary · 2017-06-21T09:25:52Z

Updated to comply with eslint #274

bnoordhuis · 2017-06-22T12:24:56Z

probes/https-outbound-probe.js

+
+HttpsOutboundProbe.prototype.attach = function(name, target) {
+  var that = this;
+  if (name == 'https') {


bnoordhuis · 2017-06-22T12:25:53Z

probes/https-outbound-probe.js

+        var requestMethod = 'GET';
+        var urlRequested = '';
+        var headers = '';
+        if (typeof options === 'object') {


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

Just googled this - I didn't realise typeof null was 'object'...

bnoordhuis · 2017-06-22T12:28:05Z

probes/https-outbound-probe.js

+          var requestMethod = 'GET';
+          var urlRequested = '';
+          var headers = '';
+          if (typeof options === 'object') {


bnoordhuis · 2017-06-22T12:29:00Z

probes/https-outbound-probe.js

+            }
+          } else if (typeof options === 'string') {
+            urlRequested = options;
+            var parsedOptions = url.parse(options);


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

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.

I rewrote this bit in both http-outbound-probe and https-outbound-probe. They're both now only calling url.parse() on metrics end.

bnoordhuis · 2017-06-22T12:29:47Z

probes/https-outbound-probe.js

+    url += '/';
+  }
+  return url;
+}


How does this differ from url.format()?

It strips off trailing ? or # components, which we don't use.

bnoordhuis · 2017-06-22T12:39:48Z

tests/probes/https-outbound-probe-test.js

+
+function isNumeric(n) {
+  return !isNaN(parseFloat(n)) && isFinite(n);
+}


Seemingly unused functions.

used on line 47

bnoordhuis · 2017-06-22T12:40:30Z

tests/probes/https-outbound-probe-test.js

+tap.tearDown(function() {
+  setTimeout(function() {
+    server.close();
+  }, 1000);


Is there a reason for this timeout? The next test doesn't have one.

Yes, the test was crashing because the server was being closed after the http outbound call was made but before it was received by the server, so this gives the server time to receive the call. The other test tests incoming https calls so by definition doesn't close the server until the calls have got there.

bnoordhuis · 2017-06-22T12:40:59Z

tests/probes/https-probe-test.js

+
+function isNumeric(n) {
+  return !isNaN(parseFloat(n)) && isFinite(n);
+}


Unused functions.

lines 66, 70, 82

bnoordhuis · 2017-06-22T12:41:41Z

tests/test_https_server.js

+var fs = require('fs');
+var path = require('path');
+
+console.log('dirname = ' + __dirname);


What is this used for?

Debug, left in by mistake, will remove

bnoordhuis · 2017-06-22T12:44:16Z

probes/https-outbound-probe.js

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


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

I'd perhaps add an explaining comment, it's not clear from context why node 8 is special.

sjanuary · 2017-06-22T13:50:15Z

@bnoordhuis thanks for the comments, I've fixed or replied to all of them, just one that I need to look into further.

sam-github · 2017-06-22T15:54:21Z

probes/https-outbound-probe.js

+var util = require('util');
+var url = require('url');
+var am = require('../');
+var semver = require('semver');


I prefer requires to be sorted alphabetically (by line) rather being in some random order.

(but don't know if that's part of the appmetrics coding style)

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.

sam-github · 2017-06-22T15:59:10Z

tests/probes/https-outbound-probe-test.js

+'use strict';
+
+process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
+var appmetrics = (appmetrics = require('../../'));


I don't understand this line, I thought maybe you were trying to set global.appmetrics as well as the module-scope appmetrics, but that isn't how it works. Typo?

Yes, this confused me too but looking at the history there was an extra appmetrics = in the original commit and I think eslint-fix or prettify must have added the brackets.

sam-github · 2017-06-22T16:03:59Z

tests/probes/https-outbound-probe-test.js

+};
+
+// Request with a callback
+https.get('https://localhost:8000/', function(res) {});


maybe assert the callback is called?

sam-github · 2017-06-22T16:07:27Z

tests/probes/https-outbound-probe-test.js

+  }, 1000);
+});
+
+var completedTests = 0;


you don't need this, if you get more (or less) than 3 https-outbound events, then there is a bug.

sam-github · 2017-06-22T16:24:46Z

tests/probes/https-probe-test.js

+'use strict';
+
+process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
+var appmetrics = (appmetrics = require('../../'));


it this test is the same as http (and it seems it is), it can be factored into a utility function, and just called with httpTest(require('https')) (or http). Besides eliminating cut-n-paste, it makes it clear that the behaviour of the probes should be identical. EDIT: and if they ever are not identical, the test will fail.

You'd also have to pass the event and monitor names ('https-outbound' vs 'http-outbound'), the rest looks the same.

tobespc · 2017-07-19T07:45:58Z

@sjanuary Looks like a lot of codebeat issues have been introduced with this request. Could you take a look please, thanks

sjanuary · 2017-07-19T08:34:18Z

@tobespc I don't really rate codebeat tbh, I'm not sure why we're using it. The 'issues' are things like 'function too long', 'similar code in 2 places', 'too many function arguments', 'block nesting too deep'. There are a lot introduced because there's a lot of new code here and some of it is similar to existing code, but some are not fixable because of how the probes framework work and others are not fixable for other reasons. I've looked through the codebeat report and there's nothing I really want to change - are there any specific issues that you would like to be addressed?

tobespc · 2017-07-19T10:13:44Z

accepting pull request whilst we look at the wider issue of codebeat and its findings

anthonywebb · 2017-07-28T15:24:32Z

Tremendous work! Any idea when the next version will be released that includes this https stuff?

Add https probes and tests

2d78fcf

fix error in https test

c895086

eslint for https probe

13a7285

bnoordhuis reviewed Jun 22, 2017

View reviewed changes

style and readability changes

a0c2326

sam-github reviewed Jun 22, 2017

View reviewed changes

sjanuary added 2 commits June 26, 2017 14:36

implement review comments

e10f9ec

Only parse URLs once in http and https outbound probes

a1b521d

tobespc merged commit 5e72dc8 into RuntimeTools:master Jul 19, 2017

sjanuary added this to the 3.1 milestone Oct 2, 2017

Add HTTPS probes and tests #455

Add HTTPS probes and tests #455

Conversation

sjanuary commented Jun 15, 2017

sjanuary commented Jun 15, 2017

gibfahn commented Jun 15, 2017

codecov-io commented Jun 15, 2017 • edited Loading

Codecov Report

sjanuary commented Jun 15, 2017

sjanuary commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjanuary Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjanuary commented Jun 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-github Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobespc commented Jul 19, 2017

sjanuary commented Jul 19, 2017

tobespc commented Jul 19, 2017

anthonywebb commented Jul 28, 2017

codecov-io commented Jun 15, 2017 •

edited

Loading

sjanuary Jun 22, 2017 •

edited

Loading

sam-github Jun 22, 2017 •

edited

Loading