Complete re-implementation #22

ronkorving · 2017-01-13T09:06:07Z

Improvements:

Performance improvements (seems to be up to one order of magnitude).
Clean new architecture.
Clearer separation between public and private properties.
Fewer buffer allocations.
More graceful client.close behavior.
The UDP socket is unref'ed to allow automatic process shutdown.
You can now var Graylog = require('graylog');, no more need (although still supported) for a property lookup.
You can track how much has been compressed and sent, through the logger.compressed and logger.sent integers (undocumented, but quite useful during tests).
ReadMe updates.
Test coverage (all but 3 lines are covered).

Changes:

logger.config is no longer exposed.
Requires Node 5.7.0 or later (ie: major version bump)

TODO:

JSHint is not aggressive enough. Maybe switch to ESLint while we're at it.

Closes #10
Closes #21
This should also make #19 behave better (please confirm, @jasonmcaffee).
This should fix #16 (please confirm, @navihtot)

ronkorving · 2017-01-13T09:17:41Z

LICENSE

@@ -1,4 +1,4 @@
-node-graylog (C) 2011 Egor Egorov <[email protected]> 
+Copyright (c) 2017 Wizcorp


I have changed this, since there's literally nothing left of the original implementation.

stelcheck · 2017-01-13T09:19:26Z

The UDP socket is unref'ed to allow automatic process shutdown.

Wouldn't this also mean that buffered messages might not get sent?

ronkorving · 2017-01-13T09:20:05Z

No, all messages in the queue will get sent (since there are events preventing the process from shutting down for as long as there are messages that haven't yet been sent). The moment the queue is empty, the logger emits a drain event to which it responds itself by closing the socket.

stelcheck

Test output isn't very informative:

chunked

✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal
✔ should be equal

I'll try to do a more thorough review next week.

stelcheck · 2017-01-13T09:20:55Z

Makefile

@@ -17,6 +17,9 @@ install:
 test:
 	npm test

+bench:


Would suggest to allow the destination server as an argument (so to allow testing against an arbitrary Graylog/Logstash server).

stelcheck · 2017-01-13T09:23:16Z

bench.js

+var Graylog = require('.').graylog;
+var fs = require('fs');
+var client;
+var servers = [


Would suggest to make this accept CLI arguments, so that you can test against an existing Graylog setup

stelcheck · 2017-01-13T09:24:01Z

bench.js

+	    facility: 'node-graylog benchmark'
+	});
+
+	client.on('error', function (error) {


I don't think this is required - wouldn't on('error') trigger a throw anyhow?

stelcheck · 2017-01-13T09:26:38Z

test/compressed.js

+		message = JSON.parse(zlib.inflateSync(message));
+
+		t.equal(message.short_message, 'short message');
+		t.equal(message.full_message, 'full message'.repeat(1000));


You could store 'full message'.repeat(1000) in a variable instead of regenerating it.

Meh, it's just a test.

True, but that pattern specifically is one you reuse frequently (see test/chunked.js, that line is everywhere).

Right now, you have the following data constructs repeated across numerous test files:

short short message ('short mesage')

short full message ('full message')

long full message ('full message'.repeat(1000))

Which seems to indicate you wish to test against those specific data constructs. But if you wanted at a later time, say, to change that to random-generated strings, or to add different data constructs (to test string messages that break the current implementation so to ensure future implementations won't suffer from regression), then you'd end up doing a lot of manual work.

One thing which can be done to avoid that is to add those often used data constructs in their own file instead. This has the advantage of:

Documenting what data constructs should be used to test future features. Even if you'd have 100% code coverage, you could still request that all new tests for new features be executed against those data constructs (at the very least).

Easier to alter if the currently used value is deemed inadequate at some point

stelcheck · 2017-01-13T09:33:32Z

test/options.js

+var Graylog = require('..');
+
+test('options', function (t) {
+	t.throws(function () {


Recommend that each throw cases be in its own test, and group related throws into describes. This way, if something starts to throw, it should display a bit more nicely.

Tape doesn't have describe() like mocha does. But you can make sub-tests. In any case, more important I think that I add messages to all assertions to make the test suite output more readable.

stelcheck · 2017-01-13T09:34:32Z

Makefile

@@ -17,6 +17,9 @@ install:
 test:


> make test make: `test' is up to date.

Happens because you have a test folder

Ah lol, nice catch. I always just run npm test, so didn't run into that. Will fix.

We could remove the Makefile tbh.

stelcheck

Indent is all over the place (wondering if linting configuration is working correctly?). Note that the initial indent of this project was space, not tabs; if you wish to change this as part of your rewrite however, I have no problem with that.

stelcheck · 2017-01-17T03:34:04Z

bench.js

+
+console.log('');
+
+function log(str, label, i, n, cb) {


What are str, i and n? Variables could use a bit of renaming here.

stelcheck · 2017-01-17T03:36:27Z

lib/Queue.js

@@ -0,0 +1,37 @@
+function Queue() {


Just a thought, but why not use caolan/async queue instead? It would add one dependency, but I assume it would also make things more easily extensible in the future (and probably sturdier than maintaining our own queue implementation).

stelcheck · 2017-01-17T03:36:56Z

lib/Graylog.js

+
+    // state
+	this._client = null;
+    this._serverIterator = 0;


Indent (spaces instead of tabs)

Ugh, I had a ton of that during this rewrite. Not sure what's up. I'll add an editorconfig file.

stelcheck · 2017-01-17T03:38:44Z

lib/Graylog.js

+};
+
+
+var count = 0;


What is this used for?

I think it's a leftover, and I need to do something about JSHint (I think it's currently running with minimal default config).

stelcheck · 2017-01-17T03:39:28Z

lib/Graylog.js

+};
+
+
+Graylog.prototype._getHeadersFromPool = function (n) {


Spaces and tabs mix-up all across this function.

stelcheck · 2017-01-17T03:56:11Z

Makefile

@@ -17,6 +17,9 @@ install:
 test:


We could remove the Makefile tbh.

stelcheck · 2017-01-17T03:59:21Z

test/levels.js

+		}
+	});
+
+	for (var i = 0; i < levelNames.length; i += 1) {


Actually, seeing how you are using the enum, you could also just make a reverse array and run a .forEach(). Again, less readable, but possibly less error-prone.

stelcheck · 2017-01-17T04:09:44Z

test/compressed.js

+		message = JSON.parse(zlib.inflateSync(message));
+
+		t.equal(message.short_message, 'short message');
+		t.equal(message.full_message, 'full message'.repeat(1000));


True, but that pattern specifically is one you reuse frequently (see test/chunked.js, that line is everywhere).

Right now, you have the following data constructs repeated across numerous test files:

short short message ('short mesage')

short full message ('full message')

long full message ('full message'.repeat(1000))

Which seems to indicate you wish to test against those specific data constructs. But if you wanted at a later time, say, to change that to random-generated strings, or to add different data constructs (to test string messages that break the current implementation so to ensure future implementations won't suffer from regression), then you'd end up doing a lot of manual work.

One thing which can be done to avoid that is to add those often used data constructs in their own file instead. This has the advantage of:

Documenting what data constructs should be used to test future features. Even if you'd have 100% code coverage, you could still request that all new tests for new features be executed against those data constructs (at the very least).

Easier to alter if the currently used value is deemed inadequate at some point

stelcheck · 2017-01-17T04:11:15Z

README.md

+uses Graylog's [GELF](http://docs.graylog.org/en/latest/pages/gelf.html) protocol, you can also use this with other
+logging software that has GELF support.
+
+## Usage

 ### Available functions


Available events should also be documented.

stelcheck · 2017-01-17T04:13:05Z

lib/Graylog.js

+
+
+module.exports = Graylog;
+module.exports.graylog = Graylog; // deprecated


Just to make sure, the rewrite does remains backward-compatible? If not, we might just remove this line flat-out.

It remains backward compatible because of this line. Although it does require a newer version of Node now.

navihtot · 2017-01-17T17:09:04Z

@ronkorving I'm not using this lib anymore in my project, so I don't have an simple way to check for #16

ronkorving · 2017-01-18T10:43:47Z

@navihtot Understood. Thanks for checking in though :)

Fl4m3Ph03n1x · 2018-06-01T15:53:21Z

I am using this lib, is there a way I can test out or somehow help?

ronkorving · 2018-06-05T01:59:26Z

@Fl4m3Ph03n1x I'm no longer using it these days, so am afraid I don't have the tools at my disposal nor the time to take this PR to completion. If you want, you could pull in this branch and take ownership by taking it to its conclusion.

Fl4m3Ph03n1x · 2018-06-05T06:32:29Z

@ronkorving Ahhh, this is confusing to me. Who will then update the project on NPM?
I don't have the permissions to do it, even if I complete the PR, it will be useless.

Can I at least be added as a collaborator for NPM if I take ownership of the project?
( that's what we are talking about here, right ? )

ronkorving · 2018-06-05T06:50:38Z

@stelcheck can update the project on NPM, as can I.

I'm perfectly happy with more maintainers (a landable PR will sell that :)), but @stelcheck will also have to be in agreement with adding maintainers. I was talking about taking ownership of this PR.

Fl4m3Ph03n1x · 2018-06-08T20:02:32Z

I will see what I can do.
So far the one thing I don't get is how to get a hold on the PR. I suppose I would fork the project and then what?

ronkorving · 2018-06-12T09:48:37Z

Make a fork of this project.
git checkout -b ronkorving-refactor2 master
Make any changes that need to be made, test, commit.
git push origin ronkorving-refactor2
Make a new pull request from your ronkorving-refactor2 branch to this repository.

Fl4m3Ph03n1x · 2018-06-14T20:43:57Z

Thanks, I'll see what I can do!
Hopefully I am up for the task!

ronkorving · 2018-06-18T03:08:53Z

Good luck!

DomiiBunn · 2023-11-28T21:57:42Z

I think you should just publish the fork as just graylog package.

At this point your version seems better and this is unmaintained.

Ron Korving added 5 commits January 5, 2017 19:33

Added benchmark script

9ceae36

Rewrote the library for clarity and performance

304d5d1

Merge branch 'bench' into refactor2

9e0f1c1

Enhanced benchmark

9bf0cfc

Refactoring 2

d01904f

ronkorving force-pushed the refactor2 branch 2 times, most recently from d2407e5 to 7656ec9 Compare January 13, 2017 09:13

Unit tests and virtually full coverage

552de88

ronkorving force-pushed the refactor2 branch from 7656ec9 to 552de88 Compare January 13, 2017 09:17

ronkorving commented Jan 13, 2017

View reviewed changes

stelcheck reviewed Jan 13, 2017

View reviewed changes

stelcheck reviewed Jan 17, 2017

View reviewed changes

ronkorving mentioned this pull request Mar 15, 2017

fix README #24

Merged

		@@ -1,4 +1,4 @@
		node-graylog (C) 2011 Egor Egorov <[email protected]>
		Copyright (c) 2017 Wizcorp



		module.exports = Graylog;
		module.exports.graylog = Graylog; // deprecated

		};


		var count = 0;

Complete re-implementation #22

Are you sure you want to change the base?

Complete re-implementation #22

Conversation

ronkorving commented Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

stelcheck commented Jan 13, 2017

ronkorving commented Jan 13, 2017 • edited Loading

stelcheck left a 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

stelcheck left a 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

ronkorving Jan 17, 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

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

navihtot commented Jan 17, 2017

ronkorving commented Jan 18, 2017

Fl4m3Ph03n1x commented Jun 1, 2018

ronkorving commented Jun 5, 2018

Fl4m3Ph03n1x commented Jun 5, 2018

ronkorving commented Jun 5, 2018

Fl4m3Ph03n1x commented Jun 8, 2018

ronkorving commented Jun 12, 2018

Fl4m3Ph03n1x commented Jun 14, 2018

ronkorving commented Jun 18, 2018

DomiiBunn commented Nov 28, 2023

ronkorving commented Jan 13, 2017 •

edited

Loading

ronkorving commented Jan 13, 2017 •

edited

Loading

ronkorving Jan 17, 2017 •

edited

Loading