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

Complete re-implementation #22

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ronkorving
Copy link
Collaborator

@ronkorving ronkorving commented Jan 13, 2017

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.

cc @stelcheck @slang800

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

@ronkorving ronkorving force-pushed the refactor2 branch 2 times, most recently from d2407e5 to 7656ec9 Compare January 13, 2017 09:13
@@ -1,4 +1,4 @@
node-graylog (C) 2011 Egor Egorov <[email protected]>
Copyright (c) 2017 Wizcorp
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@stelcheck
Copy link
Member

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

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

@ronkorving
Copy link
Collaborator Author

ronkorving commented Jan 13, 2017

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.

Copy link
Member

@stelcheck stelcheck left a comment

Choose a reason for hiding this comment

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

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.

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

bench:
Copy link
Member

Choose a reason for hiding this comment

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

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

var Graylog = require('.').graylog;
var fs = require('fs');
var client;
var servers = [
Copy link
Member

Choose a reason for hiding this comment

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

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

facility: 'node-graylog benchmark'
});

client.on('error', function (error) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed.

message = JSON.parse(zlib.inflateSync(message));

t.equal(message.short_message, 'short message');
t.equal(message.full_message, 'full message'.repeat(1000));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Meh, it's just a test.

Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. 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).

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

var Graylog = require('..');

test('options', function (t) {
t.throws(function () {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

@@ -17,6 +17,9 @@ install:
test:
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Happens because you have a test folder

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

We could remove the Makefile tbh.

Copy link
Member

@stelcheck stelcheck left a comment

Choose a reason for hiding this comment

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

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.


console.log('');

function log(str, label, i, n, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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


// state
this._client = null;
this._serverIterator = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Indent (spaces instead of tabs)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

};


var count = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What is this used for?

Copy link
Collaborator Author

@ronkorving ronkorving Jan 17, 2017

Choose a reason for hiding this comment

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

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

};


Graylog.prototype._getHeadersFromPool = function (n) {
Copy link
Member

Choose a reason for hiding this comment

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

Spaces and tabs mix-up all across this function.

@@ -17,6 +17,9 @@ install:
test:
Copy link
Member

Choose a reason for hiding this comment

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

We could remove the Makefile tbh.

}
});

for (var i = 0; i < levelNames.length; i += 1) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

message = JSON.parse(zlib.inflateSync(message));

t.equal(message.short_message, 'short message');
t.equal(message.full_message, 'full message'.repeat(1000));
Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. 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).

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

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
Copy link
Member

Choose a reason for hiding this comment

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

Available events should also be documented.



module.exports = Graylog;
module.exports.graylog = Graylog; // deprecated
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

@navihtot
Copy link

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

@ronkorving
Copy link
Collaborator Author

@navihtot Understood. Thanks for checking in though :)

@ronkorving ronkorving mentioned this pull request Mar 15, 2017
@Fl4m3Ph03n1x
Copy link

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

@ronkorving
Copy link
Collaborator Author

@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
Copy link

@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
Copy link
Collaborator Author

@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
Copy link

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
Copy link
Collaborator Author

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

@Fl4m3Ph03n1x
Copy link

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

@ronkorving
Copy link
Collaborator Author

Good luck!

@DomiiBunn
Copy link

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

my script ends and i'm missing some messages on graylog
5 participants