Avoid console.error when unable to connect to redis

[c100k]

When new Queue is unable to connect to Redis, it prints lots of errors in the console like below:

Error: connect ECONNREFUSED 127.0.0.1:6379
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1161:16) {
  errno: -61,
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 6379
}

I guess it's from this line : https://github.com/taskforcesh/bullmq/blob/master/src/classes/redis-connection.ts#L71.

Instead of logging the error, like any other library, it needs to be thrown in order to let the calling code handle it.

This might break existing implementations that do not expect an exception to be thrown from the constructor, though.

[manast]

This error is notifying the caller that this is a deprecated feature that will throw an exception in the future. So for the next major version of the library it will be like you say but for now it must just show a text to avoid breaking current deployments.

[manast]

Btw, the error you are pasting would not come from the line that you referred to but probably from this one: https://github.com/taskforcesh/bullmq/blob/6fedc0a03bdb217ef0dbae60d49fccb0f2a5dbdb/src/classes/worker.ts#L206

Since the error is emitted by ioredis there is no way to throw an exception and the best we can do is to show the error, the alternative is doing nothing of course but then we get unhandled exceptions and many people have been complained about that along the years so this is like the least of all evils regarding event based error handling...

[hmvs]

Should we in this case provide the ability to pass a logger class or function? To be able to log in a format that the app wants. For instance, we log only in JSON, and this console.error breaks our logs.

[manast]

@hmvs yes. This has been on our minds for a long time but never prioritized highly enough. Just having an option "logger" that allows defining the logger functions (warn and error are the only ones we use).

[adriana-olszak]

@manast Hey, do you have in mind what this logger implementation would look like to fit the lib? If there is interest in adding this feature to BullMQ, I would be more than happy to contribute and help with the implementation.

Issue at Hand: We're currently experiencing significant issues with DataDog log ingestion. The problems originate from incorrect log formatting, which causes an overabundance of unnecessary error logs. Moreover, logs in this incorrect format are rapidly depleting our Quota.

Specific Cause: Our services occasionally face trouble when connecting to a Redis instance (ElastiCache) inside of k8s. These interruptions last for only 1 or 2 seconds after bootstrap. In response to this, IORedis emits an error event, which BullMQ then prints to the console. The issue here is that each line of the error message is printed as a separate error.

Impact: There are two major consequences we're dealing with:

1. Ineffective Metrics and Alerts: The Error threshold-based metrics and alerts are becoming unreliable due to the inability to filter out console errors printed by BullMQ.
2. Inability to Alert on Persistent Redis Connection Failure: We are unable to set up alerts for scenarios where the Redis connection fails for an extended period.

How does it look like? Here you have a screenshot of how our DD logs look like when bull uses console.error to print them instead of our logger implementation.

[manast]

@adriana-olszak I do not have any particular implementation in mind. I think that just an option that can be passed to the Worker, Queue, and QueueEvents constructors where you define an alternative logger function instead of the default console would work fine. Not sure but I think Pino (https://www.npmjs.com/package/pino) is one of the most popular loggers for Node, so maybe mimicking their API is also a good idea.