Add TCP support #79

michalholasek · 2018-07-09T16:00:36Z

This PR builds upon @remie's great work at sivy/node-statsd#67, with couple shortcomings:

I was not able to resolve problems with global tags and should properly send a and b with the same value test cases (skipped with xit). @bdeitte, could you please point me to correct direction to what is missing / wrong?
TCP test cases are pretty much copy/paste from the PR mentioned above - since there are lot of changes to this codebase since, it is possible that some test cases are missing

bdeitte · 2018-07-10T02:07:15Z

Thanks for this, great to see! A few things:

I see there's a conflicting file- looks like a merge from master resolve needs to happen here?
I don't like the the idea of having so much duplication in tests. This is very likely to get out of sync and be problematic for further updates. Is there a way this can be done where both ways to test share with each other?
Could you tell me how you are seeing some of these tests fail? I can take a look at another time in more depth but can't do that at the moment, but if you can let me know what you're seeing now I can see what I can do to help out.
Thanks again!

honzajavorek

I have just a few nitpicks, but I didn't find anything obvious to cause problems in the test. I'll spend some time playing with it locally and let's see.

As mentioned, there is a conflict needed to be resolved.
I agree with @bdeitte we should think about the duplication of tests, but I suggest we first get them passing this way, isolated and separated. I believe debugging problems in the initial TCP implementation while having the tests tangled together with the UPC implementation could be quite hard. Maybe we could make the tests more DRY as a next step when everything is passing?

honzajavorek · 2018-07-10T08:45:02Z

lib/statsd.js

    };
  }

+  function createSocket(args) {


I'd use options instead of args, but both are saying nothing about the contents, so it's just a convention nitpick.

honzajavorek · 2018-07-10T08:45:49Z

lib/statsd.js

@@ -91,6 +102,10 @@ var Client = function (host, port, prefix, suffix, globalize, cacheDns, mock,
    global.statsd = this;
  }

+  if (options.protocol === 'tcp') {
+    this.socket.setKeepAlive(true);
+  }


Shouldn't this be a part of the createSocket "method"?

Addressed in 69de8cb.

honzajavorek · 2018-07-10T09:02:44Z

Also, would you mind removing the xit so we can see (after the conflict is resolved) what exactly fails here in the CI? The Travis is testing ~4 versions of Node here, so maybe we can factor out whether the issue isn't specific to a particular Node version?

honzajavorek · 2018-07-10T09:08:42Z

When running locally on Node 8, I'm getting following failures randomly (flakily) even when I keep the xit marks intact:

1) StatsD [TCP] #globalTags should not add global tags if they are not specified:
     Uncaught Error: connect ECONNREFUSED 127.0.0.1:8125
      at Object._errnoException (util.js:992:11)
      at _exceptionWithHostPort (util.js:1014:20)
      at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1186:14)

  2) StatsD [TCP] #globalTags Uncaught error outside test suite:
     Uncaught TypeError: Cannot read property 'currentRetry' of undefined

Also

1) StatsD [TCP] #globalTags should not add global tags if they are not specified:
     Uncaught Error: getaddrinfo ENOTFOUND host host:1234
      at errnoException (dns.js:50:10)
      at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:92:26)

  2) StatsD [TCP] #globalTags Uncaught error outside test suite:
     Uncaught TypeError: Cannot read property 'currentRetry' of undefined

They occur especially when running the whole test suite repeatedly. Looks like there is a flawed tear down somewhere.

michalholasek · 2018-07-10T09:47:01Z

The conflict was probably caused by test_statsd.js -> test_statsd_udp.js rename - I removed the commits with changes in test_statsd.js for now. xit test cases are also unskipped.

Regarding tests:

I think that one cause for the flakiness could be old version of mocha. Another could be race conditions in create server -> assert -> close server logic.
We discussed the DRYness / refactoring with @honzajavorek, but decided not do it for the sake of PR review complexity (a lot of changes) and time constraints. I agree that current implementation is suboptimal, but I would rather go with working current suite and then do the refactor as next step / PR

@bdeitte In #globalTags suite, there is should combine global tags and metric tags test case - it expects the result to be test:1337|c|#foo,gtag, but the actual value is test:1337|c|#gtag,foo (position of the tags is inverted). I guess it has something to do with overrideTags in helpers.js file, but I am not able to track why it works for UDP, but does not for TCP. Another is should properly send a and b with the same value test case in every suite, be it #gauge, #histogram, ... - expects 0, but actual value is NaN.

honzajavorek · 2018-07-10T13:05:34Z

race conditions in create server -> assert -> close server logic

I suspect that as well. We should hunt those down. Do you experience similar flakiness locally or is it just me?

michalholasek · 2018-07-10T13:12:32Z

@honzajavorek I do. I am not sure it will be possible without the rewrite 😞

honzajavorek · 2018-07-10T17:36:02Z

My hunch is the tcpTest function does something incorrectly and makes all the tests non-deterministic.

michalholasek · 2018-07-11T10:49:57Z

@honzajavorek Changes made in b571d8c ensure that the tests pass/fail consistently now. The solution is not ideal, but at least we can now focus on making them pass.

honzajavorek

Running npm test on top of the branch fails due to some linter errors. I marked them with comments.

honzajavorek · 2018-07-11T11:42:04Z

test/test_statsd_tcp.js

-        metrics.forEach(function (metric) {
-          test(metric, server);
-        });
+        var metrics = parts.filter(part => part !== '');


test/test_statsd_tcp.js: line 37, col 41, 'arrow function syntax (=>)' is only available in ES6 (use 'esversion: 6').

honzajavorek · 2018-07-11T11:42:23Z

test/test_statsd_tcp.js

+          protocol: 'tcp'
+        });
+        assert.ok(statsd.socket instanceof net.Socket);
+        done();


test/test_statsd_tcp.js: line 72, col 9, 'done' is not defined.

honzajavorek · 2018-07-11T13:43:46Z

BTW I can still see #increment being strangely nested inside #gauge in the tests output, which is rather suspicious.

#gauge
      ✓ should send proper gauge format without prefix, suffix, sampling and callback
      ✓ should send proper gauge format with tags
      ✓ should send proper gauge format with prefix, suffix, sampling and callback
      4) should properly send a and b with the same value
      5) "after each" hook
      #increment

I'll try to help with debugging this. Maybe I can figure something out. @bdeitte if you don't have any immediate ideas on what could be wrong with either the TCP implementation or the tests, then feel free to ignore it here until we get the thing passing. And sorry for the noise.

michalholasek · 2018-07-11T15:43:10Z

@bdeitte The build is green now, could you please take another look? Thank you.

bdeitte · 2018-07-12T01:56:42Z

@michalholasek Thanks for getting the tests working and doing all this work. Unfortunately I still have the same feeling I have before- the duplication of tests in the way they are isn't something I am comfortable with. The tests are already drifting apart from what I see, and this is only going to increase without careful diligence over time. There needs to be something done to get them closer in line, even if it's not exactly right. I know that you mention this could be done in a followup PR, but that is not a great place for this codebase to be in. If you need to have this right away and can't make those changes, I would suggest using your branch for now until that can be done. (Although I hope you will make them now, would be great to have this in!)

I thought about this some more, to try to think of something that isn't too painful to do but which gets more at what I'm worried about here. Here's my suggestion: https://gist.github.com/bdeitte/4ae7fd92edb490e2adcd22e38d62a5a6

What I did in the above is take your new tests, rip out everything other than globalTags tests, and then try to add in the UDP tests. I ran this in your branch and it passes there.

If you want to try to add this way of doing things, here's you would need to do:

Start with test_statsd_udp.js, and rename createServer to createTCPServer. Change all references inline. Also add in createUDPServer as shown above.
Copy each "describe" section you've made and paste a new one below it. Name one UDP and one TCP. For the UDP one, switch all TCP references to UDP
Run it all and hopefully see it all pass
Look back in test_statsd.js to see if anything is missing in there- I know there's some tests that were just in there. Copy them in and switch to createTCPServer()
Rename test_statsd_udp.js to test_statsd.js

In the future, it would be good to split these tests into multiple files (while still keeping UDP/TCP tests for related functionality together) as this file is really big. It'd also probably be good to think more about ways to shorten some of the boilerplate in the tests. But I started to think about that and realized it's not what I'm asking here and definitely don't want to make this something more than it is, which is just a request to have similar tests live by each other for easier maintenance. Thanks!

honzajavorek · 2018-07-12T08:42:40Z

lib/statsd.js

+
+  if (this.protocol === 'tcp') {
+    message += '\n';
+  }


I think this should be in the sendMessage method as that one deals with the quirks of the individual transmission protocols. This is an implementation details of sending the message over TCP and IMHO should be encapsulated by sendMessage.

honzajavorek · 2018-07-12T08:44:03Z

lib/statsd.js

  // hidden global_tags option for backwards compatibility
  options.globalTags = options.globalTags || options.global_tags;

+  this.protocol = options.protocol;


Nitpick: Although it's against various code styles, I'd stick to the code style already in place and add the alignment whitespaces:

- this.protocol = options.protocol; + this.protocol = options.protocol;

honzajavorek · 2018-07-12T08:47:35Z

Just for the record, once this is done, it closes #76 (I miss the connection when browsing the issues/PRs here 😄 )

michalholasek · 2018-07-15T16:24:51Z

@bdeitte Test refactor is finished now, PR is ready for re-review:

UDP / TCP tests are now 1/1
Almost every public method has its own file, with the exception of most methods from statsFunctions.js

bdeitte · 2018-07-16T23:12:07Z

lib/statsd.js

+    }
+
+    return socket;
+  }.bind(this);


Doing it this way means we have two ways to have functions in here. Could you make this like the existing "_send" method? Alternatively, if you feel strongly about it, it's not that I'm a big fan of "_send"- you could also make that method like this one.

I added the bind to appease jshint, which is not good enough reason, I agree. I changed the createSocket function signature to accept instance argument instead (dee23b3).

bdeitte · 2018-07-16T23:16:08Z

lib/statsd.js

@@ -42,26 +43,55 @@ var Client = function (host, port, prefix, suffix, globalize, cacheDns, mock,
      maxBufferSize : maxBufferSize,
      bufferFlushInterval: bufferFlushInterval,
      telegraf    : telegraf,
-      sampleRate    : sampleRate
+      sampleRate  : sampleRate,
+      protocol    : protocol
    };
  }


Someone is going to do "TCP" at some point and get confused. How about adding:

if (options.protocol) { options.protocol = options.toLowerCase(); }

Addressed in fe2d446.

bdeitte · 2018-07-16T23:17:40Z

package.json

-    "test": "mocha -R spec",
-    "lint": "jshint lib/**.js test/**.js",
+    "test": "mocha -R spec --timeout 5000 test/index.js",
+    "lint": "jshint lib/**.js",


Why not lint the tests anymore? If you had an issue, can we just exclude that with a jshint ignore comment?

Addressed in 34215ff.

bdeitte · 2018-07-16T23:18:19Z

test/buffer.js

+
+var createStatsdClient = require('./helpers').createStatsdClient;
+var createTCPServer = require('./helpers').createTCPServer;
+var createUDPServer = require('./helpers').createUDPServer;


These tests and the abstractions in them look great- thanks!

bdeitte · 2018-07-16T23:22:21Z

Wow this is a lot of work, thank you! I reviewed everything and just had a few small comments. (I took awhile with the tests a bit earlier and think I reviewed it all but lots to cover.) . Once you can answer/fix the few comments, I can review/answer that and merge in with a minor release.

I will still make it minor in this case for anyone curious- I have made a major before with major code changes, but the changes just look at statsd.js itself look small enough (and not backwards breaking of course) with whitespace ignored to be a minor release.

bdeitte

Looks good- merging and releasing.

remie · 2018-07-18T07:26:49Z

Thanks @michalholasek for all the hard work you put into this!

michalholasek · 2018-07-20T13:02:19Z

@bdeitte Thank you for your time you've put into feedback/reviewing! @remie Thank you for the original work, it helped a lot 🙂

Michal Holasek added 2 commits July 9, 2018 17:45

Add TCP support

08b79b6

Add TCP test suite

c0b0d47

honzajavorek reviewed Jul 10, 2018

View reviewed changes

Unskip failing TCP test cases

265b222

Move setKeepAlive flag for TCP to createSocket function

69de8cb

Make TCP tests deterministic

b571d8c

honzajavorek reviewed Jul 11, 2018

View reviewed changes

michalholasek added 4 commits July 11, 2018 16:44

Simplify TCP server in tests

c87dcf3

Do not add bytes to sentBytes when it is undefined

957411a

Compare multiple metrics against an array, not single values

9c82ab4

Use correct global/local tag order in TCP test

13bd3ea

honzajavorek reviewed Jul 12, 2018

View reviewed changes

michalholasek added 5 commits July 12, 2018 14:47

Extract createTCP/UDPServer to test/helpers.js

456b2ac

Extract #init tests to separate file

e1c84bd

Move newline addition to sendMessage

eb7cac1

Add socket.destroy() to statsd.close() when using TCP

64d5c54

Handle TCP socket connection errors gracefully

39c25ac

michalholasek added 15 commits July 13, 2018 19:59

Extract stats functions tests to test/statsFunctions.js

4ce7a1b

Remove unnecessary test/timing.js

082f84b

Fix test/sendMessage.js export function name

014ab95

Whitespace fixes

c815723

Fix statsFunctions suite description

cce0708

Add pretest script back to package.json

74ab81c

Fix lint errors

ceddf7d

Extract #event tests to test/event.js

196dd6c

Extract #check tests to test/check.js

16634ff

Extract child client specific tests to test/childClient.js

29b7186

Do not run main client specific tests for child clients

1e55f02

Remove forgotten describe.only from test/childClient.js

2683b7b

Remove unnecessary test files

ba0ab7b

Add 'protocol' option to README.md

034b7b7

Fix hanging tests

e0dd22c

bdeitte reviewed Jul 16, 2018

View reviewed changes

michalholasek added 3 commits July 17, 2018 11:35

Pass instance as argument to createSocket function

dee23b3

Make sure protocol option is always lowercase

fe2d446

Fix lint issues in test/**.js

34215ff

bdeitte approved these changes Jul 18, 2018

View reviewed changes

bdeitte merged commit 306996f into brightcove:master Jul 18, 2018

michalholasek deleted the michalholasek/add-tcp-support branch July 20, 2018 13:02

bdeitte mentioned this pull request Jul 27, 2018

asyncTimer #81

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TCP support #79

Add TCP support #79

michalholasek commented Jul 9, 2018

bdeitte commented Jul 10, 2018

honzajavorek left a comment •

edited

Loading

honzajavorek Jul 10, 2018

honzajavorek Jul 10, 2018

michalholasek Jul 10, 2018

honzajavorek commented Jul 10, 2018

honzajavorek commented Jul 10, 2018 •

edited

Loading

michalholasek commented Jul 10, 2018 •

edited

Loading

honzajavorek commented Jul 10, 2018 •

edited

Loading

michalholasek commented Jul 10, 2018

honzajavorek commented Jul 10, 2018

michalholasek commented Jul 11, 2018

honzajavorek left a comment

honzajavorek Jul 11, 2018

honzajavorek Jul 11, 2018

honzajavorek commented Jul 11, 2018

michalholasek commented Jul 11, 2018

bdeitte commented Jul 12, 2018

honzajavorek Jul 12, 2018

honzajavorek Jul 12, 2018 •

edited

Loading

honzajavorek commented Jul 12, 2018

michalholasek commented Jul 15, 2018

bdeitte Jul 16, 2018

michalholasek Jul 17, 2018

bdeitte Jul 16, 2018

michalholasek Jul 17, 2018

bdeitte Jul 16, 2018

michalholasek Jul 17, 2018

bdeitte Jul 16, 2018

bdeitte commented Jul 16, 2018

bdeitte left a comment

remie commented Jul 18, 2018

michalholasek commented Jul 20, 2018

Add TCP support #79

Add TCP support #79

Conversation

michalholasek commented Jul 9, 2018

bdeitte commented Jul 10, 2018

honzajavorek left a comment • 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

honzajavorek commented Jul 10, 2018

honzajavorek commented Jul 10, 2018 • edited Loading

michalholasek commented Jul 10, 2018 • edited Loading

honzajavorek commented Jul 10, 2018 • edited Loading

michalholasek commented Jul 10, 2018

honzajavorek commented Jul 10, 2018

michalholasek commented Jul 11, 2018

honzajavorek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honzajavorek commented Jul 11, 2018

michalholasek commented Jul 11, 2018

bdeitte commented Jul 12, 2018

Choose a reason for hiding this comment

honzajavorek Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

honzajavorek commented Jul 12, 2018

michalholasek commented Jul 15, 2018

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

bdeitte commented Jul 16, 2018

bdeitte left a comment

Choose a reason for hiding this comment

remie commented Jul 18, 2018

michalholasek commented Jul 20, 2018

honzajavorek left a comment •

edited

Loading

honzajavorek commented Jul 10, 2018 •

edited

Loading

michalholasek commented Jul 10, 2018 •

edited

Loading

honzajavorek commented Jul 10, 2018 •

edited

Loading

honzajavorek Jul 12, 2018 •

edited

Loading