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

added amqps (ssl) support. #12

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

Conversation

kmacpher67
Copy link

added ssl support, stable retry for unstable socket connections (that may go down time-to-time).
keepalive is pretty important.

@codecov
Copy link

codecov bot commented Jan 10, 2020

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #12   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          79     92   +13     
=====================================
+ Hits           79     92   +13
Impacted Files Coverage Δ
lib/index.js 100% <100%> (ø) ⬆️

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 9e23e38...cb5b6fe. Read the comment docs.

@kmacpher67
Copy link
Author

It looks like got them working, except something crazy happened on the node 6 test:

Job Node.js State
36.1 12 passed
36.2 10 passed
36.3 8 passed
36.4 6 failed

test/tap/index-test.js .............................. 17/18 5s
log4js rabbitmqAppender > shutdown should only wait until promises have been resolved
not ok expect truthy value
at:
line: 185
column: 9
file: test/tap/index-test.js
function: setup.appender.shutdown
stack: |
setup.appender.shutdown (test/tap/index-test.js:185:9)
source: |
t.ok(timeWaiting > 200);
total ............................................... 17/18

@kmacpher67
Copy link
Author

@nomiddlename I got this to pass the tests.
And I didn't even delete any!
I believe that the tests pass as expected and kept the spirit of the tests including adding the new functional attributes for ssl and retry etc.

Copy link
Contributor

@nomiddlename nomiddlename left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, much appreciated. I've made a couple of comments, mostly regarding documentation - but there is a cut-and-paste layout from the tests in the main code that is a bit odd.

@@ -11,6 +11,7 @@ If you want to be sure that all messages have been sent before your programme ex
## Configuration

* `type` - `@log4js-ndoe/rabbitmq`
* `protocol` - `string` (optional, defaults to `amqp`) - the port the rabbitmq protocol option: `amqps`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just read the rabbitmq protocol option?

@@ -22,6 +23,9 @@ If you want to be sure that all messages have been sent before your programme ex
* `vhost` - `string` (optional, defaults to `/`) - vhost to use
* `layout` - `object` (optional, defaults to `messagePassThroughLayout`) - the layout to use for log events (see [layouts](layouts.md)).
* `shutdownTimeout` - `integer` (optional, defaults to `10000`) - maximum time in milliseconds to wait for messages to be sent during log4js shutdown.
* `frameMax` - `integer` (optional, defaults to `0`) - The size in bytes of the maximum frame allowed over the connection. 0 means no limit (but since frames have a size field which is an unsigned 32 bit integer, it’s perforce 2^32 - 1); I default it to 0x1000, i.e. 4kb, which is the allowed minimum, will fit many purposes, and not chug through Node.JS’s buffer pooling. [reference](https://www.squaremobius.net/amqp.node/channel_api.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to set the default to your 4kb value.

const username = config.username || 'guest';
const password = config.password || 'guest';
const exchange = config.exchange || 'log';
const type = config.mq_type || 'direct';
const durable = config.durable || false;
const routingKey = config.routing_key || 'logstash';
const vhost = config.vhost || '/';
const heartbeat = config.heartbeat || 60;
const locale = config.locale || 'en_US';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably add locale to the docs as a configuration option.

const locale = config.locale || 'en_US';
const frameMax = config.frameMax || 0;
const keepAliveDelay = config.keepAliveDelay || 0;
const connectionTimeout = config.connection_timeout || 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think connectionTimeout is mentioned in the docs either.

connection_timeout: connectionTimeout,
layout: {
type: 'pattern',
pattern: 'cheese %m'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really the pattern you want to use?

t.equal(result.fakeRabbitmq.state.msgs.length, 1, 'should be one message only');
t.equal(result.fakeRabbitmq.state.msgs[0].toString(), 'cheese %m');
t.end();
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're talking to a fake RabbitMQ you shouldn't need a setTimeout, because it won't be doing anything asynchronous.

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.

2 participants