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

feat: implement monitor command #483

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

Conversation

ArturSharapov
Copy link
Contributor

Reference

Proposed changes

First implemented with a few lines of code, but then extended taking PubSub as an example to meet the project structure/style, reconnection policy, etc.

@uki00a I would appreciate your assistance with covering it with tests, as I couldn't get them running locally.

Example:

const monitor = await redis.monitor()

await redis.ping()
await redis.ping()
await redis.xrange('foo', '-', '+', 3)
await redis.ping()

for await (const { timestamp, args } of monitor.receive()) {
  console.log(`[${timestamp}]`, ...args)
  if (args.length > 1) monitor.close()
}
console.log('Done')

Expected output:

[1735139990.6913076] PING
[1735139990.6922036] PING
[1735139990.6932956] XRANGE foo - + COUNT 3
Done

@uki00a uki00a self-requested a review December 26, 2024 01:05
@uki00a uki00a added the enhancement New feature or request label Dec 26, 2024
Copy link
Member

@uki00a uki00a left a comment

Choose a reason for hiding this comment

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

@ArturSharapov Thanks for opening the PR! The implementation looks good overall. Just one comment.

monitor() {
throw new Error("not supported yet");
async monitor() {
const connection = this.executor.connection.duplicate();
Copy link
Member

@uki00a uki00a Dec 26, 2024

Choose a reason for hiding this comment

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

I think it would be better not to duplicate the connection here for the following reasons:

  1. to keep consistency with subscribe()
  2. node-redis also works that way

What do you think about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been using ioredis historically (which also seems to be 1.5+ times more popular than node-redis on npm).

Unlike node-redis, it does duplicate the connection by default on monitor() which I think was rational because all other commands become unavailable then.

The only positive side of using a single connection I can see is to cover the case when one would want to create redis connection only to perform monitoring and nothing else.

But this comes with a cost, making it dangerous, as it easily leads to unexpected behavior, blocking execution of all other redis commands. For those who are not experienced with redis much or for those coming from ioredis, it would become unclear, why some redis commands just get stuck and take forever to execute; in a large codebase it would be especially challenging to debug. It's also unclear whether the commands you call are accumulated and executed once you terminate the monitor (which can only be done manually), or they are just ignored entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would advocate for the duplication of connection as it make it simpler and doesn't "break all things", as well as eliminates potential issues and unforeseen behavior.

That said, what about brining an argument (option) to the monitor() that could make it use the same connection?
To cover the case when one needs to have redis only for monitoring and doesn't need a regular working redis instance to use the rest of the API.

Copy link
Member

@uki00a uki00a Dec 27, 2024

Choose a reason for hiding this comment

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

@ArturSharapov I'm in favor of adding an option to the monitor() 👍

We are planning to support RESP3 in the future, and it would be ideal to have an option of not duplicating a connection to monitor().

I have two suggestions:

  • Add reuseConnection option to monitor().
  • Make Connection#duplicate a private API for now
    • I'll implement a connection pool in this library and would like to keep Connection#duplicate private until then
    • e.g.)
      [kUnstableReadReply](returnsUint8Arrays?: boolean): Promise<RedisReply>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call 👍

@uki00a uki00a linked an issue Dec 26, 2024 that may be closed by this pull request
@uki00a
Copy link
Member

uki00a commented Dec 26, 2024

@ArturSharapov To run tests locally, you'll need to install redis-cli and redis-server binaries. Your implementation looks good, so if installation is troublesome, I'll add tests in another PR.

Alternatively, CI will also run tests, so you can take advantage of that.

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

Successfully merging this pull request may close these issues.

Implement MONITOR command
2 participants