Extended Valkey client introspection functionality

[madolson]

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.

The following commands could work with the same set of filters.

• CLIENT COUNT [filters...] (New command suggested)
• CLIENT KILL [filters...]
• CLIENT LIST [filters...]

The supported filters are to be decided. Suggestions, including already existing filters:

• TYPE [NORMAL|MASTER|PRIMARY|SLAVE|REPLICA|PUBSUB] (existing)
• USER username (Adding Missing filters to CLIENT LIST and Dedup Parsing #1401)
• ADDR ip:port (Adding Missing filters to CLIENT LIST and Dedup Parsing #1401)
• LADDR ip:port (Adding Missing filters to CLIENT LIST and Dedup Parsing #1401)
• SKIPME [yes|no] (Adding Missing filters to CLIENT LIST and Dedup Parsing #1401)
• MAXAGE time (Adding Missing filters to CLIENT LIST and Dedup Parsing #1401)
• ID client-id (existing)
• ID client-id [client-id ...] (Adding Missing filters to CLIENT LIST and Dedup Parsing #1401) for compatibility reasons with client list, client-id must be specified last.
• PATTERN pattern the client must subscribe to (Needed?)
• CHANNEL channel the client must subscribe to (Needed?)
• SHARD-CHANNEL <shard channel the client must subscribe to> (Needed?)
• FLAGS Client must include this flag, this option can be repeated, or the argument can be a string of multiple flags. Example: CLIENT LIST FLAGS "bPt".
• NAME client name
• IDLE time (or MIN-IDLE) minimum idle time of the client.

More filter suggestions that came up during discussions:

• IP or CLIENT-IP, or allow overloaded ADDR ip. (The existing filter syntax ADDR ip:port is not very useful, because the client's port is ephemeral and will change for each connection.)
• LIB-NAME
• LIB-VER
• DB number
• TOT-NET-IN bytes
• TOT-NET-OUT bytes

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]

Originally reference from redis/redis#12311.

[hpatro]

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]

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.

[sarthakaggarwal97]

@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?

[sarthakaggarwal97]

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]

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]

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]

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

[lyq2333]

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

[sarthakaggarwal97]

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]

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.

[zuiderkwast]

The PR for unifying LIST and KILL is almost ready to be merged and there are already PRs for adding more filters and adding a CLIENT COUNT command.

@valkey-io/core-team Let's make a decision about which filters we actually want to add before we spend too much time on the PRs.

1. I think CLIENT LIST PATTERN x* isn't very obvious that it's about PSUBSCRIBE patterns. Maybe we shall call it SUBSCRIBED-PATTERN instead, and similar for the other pubsub filters?

   SUBSCRIBED-PATTERN
   SUBSCRIBED-CHANNEL
   SUBSCRIBED-SHARD-CHANNEL
   

2. Zhao mentioned that it can be useful to filter for negated flags, like client list flags !b, or maybe NOT-FLAGS b. (Mentioned in Adding Missing filters to CLIENT LIST and Dedup Parsing #1401.)

3. Add LIB-NAME and LIB-VER filters? To detect clients from a particular application or part of application.

Can we also make a decision about adding CLIENT COUNT at the same time. With unified code for filters, it seems like very little extra code, apart from the JSON metadata of each command.

[soloestoy]

IP or CLIENT-IP is also a very useful filter.

[zuiderkwast]

IP or CLIENT-IP is also a very useful filter.

We have ADDR but it is ip:port. The client port is random, not useful for filtering I think. Should we have IP instead of ADDR? Or let ADDR be used without port?

[hwware]

I propose the following candidates:

ADDR ip:port
LADDR ip:port
IDLE time
FLAG
DB
LIB-Name
tot-net-in
tot-net-out

[PingXie]

I am directionally aligned with the proposal, more specifically, I like the following proposals

1. COUNT is a good idea. I am surprised that we don't have that basic stat today and we have to resort to making the expensive INFO call.
2. Lib name and version are very useful, which is essentially the browser's user-agent
3. using RESP format for the client info is the right thing to do. I am not very concerned about the adoption of a new management comment. Valkey-cli is a sufficiently good distribution channel IMO.
4. pagination for LIST or the new LIST like command is must have. The only caveat that I can think of is that we will be working with a dynamic client list across the calls.
5. +1 on aligning the filters across the sub commands. The consistency will be a good boost for productivity and user-friendliness.
6. negative filtering makes sense to me.

[soloestoy]

We have ADDR but it is ip:port. The client port is random, not useful for filtering I think. Should we have IP instead of ADDR? Or let ADDR be used without port?

IMHO, ADDR(ip:port) is not a filter, but since it already exists, we have to continue maintaining it. We could consider adding a new filter, such as ADDR-IP.

[zuiderkwast]

IMHO, ADDR(ip:port) is not a filter, but since it already exists, we have to continue maintaining it. We could consider adding a new filter, such as ADDR-IP.

How about overloading it and allow ADDR ip and ADDR ip:port? We can document ADDR ip as the main syntax and mention that for backward compatbibility ADDR ip:port is also supported.

Confusing? New filter is better?

[sarthakaggarwal97]

In my opinion, new filter sounds better for ip. I have updated the PR with rest of the discussed filters. I can update with ip filter as well.