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

Extended Valkey client introspection functionality #668

Open
madolson opened this issue Jun 19, 2024 · 11 comments · May be fixed by #1467
Open

Extended Valkey client introspection functionality #668

madolson opened this issue Jun 19, 2024 · 11 comments · May be fixed by #1467
Assignees

Comments

@madolson
Copy link
Member

We want to standardize the way that admin users can describe and interact with the client commands. We are proposing to generalize the filters introduced in CLIENT KILL so that it also works to count and list clients as well.

In the end, we will support the following 4 commands.

CLIENT COUNT <filters>
CLIENT KILL <filters>
CLIENT LIST <filters>

The supported filters will be:

TYPE [NORMAL|MASTER|PRIMARY|SLAVE|REPLICA|PUBSUB]
USER username
ADDR ip:port
LADDR ip:port
SKIPME [yes|no]
ID client-id
ID client-id [client-id ...], for compatibility reasons with client list, client-id must be specified last.
PATTERN pattern the client must subscribe to
CHANNEL channel the client must subscribe to
SHARD-CHANNEL `<shard channel the client must subscribe to>`
FLAGS Client must include this flag, this option can be repeated
NAME client name
MIN-IDLE minimum idle time of the client
CLIENT DESCRIBE will allow you specify what fields you want from the client. The supported selectors will be the same as the above mentioned filters, in addition to the fields currently supported from https://redis.io/commands/client-list/.

In Redis we also discussed adding a new command, CLIENT DESCRIBE, will also introduce the notion of selectors. Selectors allow you to scope down the fields you are interested in.

CLIENT DESCRIBE SELECT ID ADDR FILTER MIN-AGE 100 
@madolson
Copy link
Member Author

Originally reference from redis/redis#12311.

@madolson madolson changed the title Extended Redis client functionality Extended Redis client introspection functionality Jun 19, 2024
@hpatro
Copy link
Contributor

hpatro commented Jun 21, 2024

Nit Title: Extended Client Introspection functionality

The default behavior for CLIENT LIST API is all filters, and with that the response payload can be pretty huge. I think valkey-cli kind of gets stuck. With CLIENT DESCRIBE we can look to move away from the text format and have RESP format and get paginated response or adding a default COUNT value to restrict the payload size. I had tried that here redis/redis#11907.

The downside of introducing the new API is maintaining two set of API(s) and no guarantee of administrator(s) of moving to this.

@madolson madolson changed the title Extended Redis client introspection functionality Extended Valkey client introspection functionality Jun 21, 2024
@madolson
Copy link
Member Author

madolson commented Jun 21, 2024

Nit Title: Extended Client Introspection functionality

Old habits die hard.

The default behavior for CLIENT LIST API is all filters, and with that the response payload can be pretty huge. I think valkey-cli kind of gets stuck. With CLIENT DESCRIBE we can look to move away from the text format and have RESP format and get paginated response or adding a default COUNT value to restrict the payload size. I had tried that here redis/redis#11907.

I think you raise a really good point. We could add a pagination functionality by doing client-list based off of ID, and add that to client list. I think we have seen that people generally don't migrate to new commands, so it probably makes more sense to enhanced the ones we have.

@hpatro hpatro self-assigned this Nov 20, 2024
@sarthakaggarwal97
Copy link
Contributor

@valkey-io/core-team I'm happy to take this issue up if this is an approved change. Please let me know if I can proceed.
We would be introducing some new filters to existing COUNT, KILL and LIST sub commands.
Also, would like to know if we would want to paginate the results in the current list command?

@hpatro hpatro assigned sarthakaggarwal97 and unassigned hpatro Nov 22, 2024
@sarthakaggarwal97
Copy link
Contributor

sarthakaggarwal97 commented Nov 26, 2024

We can abstract the current filtering logic out (and add new filters) and then use it in different commands as required. In my opinion, CLIENT DESCRIBE (if approved) should be taken in a separate PR.

@hpatro
Copy link
Contributor

hpatro commented Nov 26, 2024

I feel CLIENT LIST API would have the maximum benefit out of this filters extension as the response payload can be limited through it. @valkey-io/core-team Please add your thoughts on this. @sarthakaggarwal97 is trying to come up with a solution.

@zuiderkwast
Copy link
Contributor

CLIENT KILL has more filters than any other. Let's first add the missing ones of those to CLIENT LIST, to unify them. We can de-duplicate the parsing code too. That's a good PR to start with IMHO.

@hwware
Copy link
Member

hwware commented Nov 27, 2024

Sorry, is there the command CLIENT COUNT? new command?

@lyq2333
Copy link
Contributor

lyq2333 commented Dec 9, 2024

I like this idea. In #1398 , I hope we have a CLIENT KILL FLAGS command.

@sarthakaggarwal97
Copy link
Contributor

sarthakaggarwal97 commented Dec 10, 2024

I would like to summarize the issues that we are going to solve under #668. Please feel free to add any that I have missed.

  1. Implement KILL subcommand filters to LIST, and dedup parsing and filtering. (PR Adding Missing filters to CLIENT LIST and Dedup Parsing #1401 in progress)
  2. Extend the current filters to the list mentioned in the issue description.
  3. Introduce a new CLIENT COUNT command to return the count of clients based on the filters.
  4. Introduce a new CLIENT DESCRIBE command to scope down the fields from client info as requested by user.

@valkey-io/core-team are we good to proceed with all the above mentioned issues?

I can come with the separate issues for each, so this can be picked up by multiple people. We can even discuss in detail about each of these in their respective issues.

@zuiderkwast
Copy link
Contributor

To me, it seems safe to proceed with 1-3.

For 4, I think we should discuss it a bit more first. I don't want to add a new command that almost nobody will use (like CLUSTER SHARDS) so I want to have a good motivation about uses cases and the features (more fields? pagination?), the syntax, etc.

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 a pull request may close this issue.

6 participants