Allow clients to report name and version

[uglide]

This PR allows clients to send information about the client library to redis to be displayed in CLIENT LIST and CLIENT INFO.

Currently supports:
CLIENT [lib-name | lib-ver] <value>
Client libraries are expected to pipeline these right after AUTH, and ignore the failure in case they're talking to an older version of redis.

These will be shown in CLIENT LIST and CLIENT INFO as:

• lib-name - meant to hold the client library name.
• lib-ver - meant to hold the client library version.

The values cannot contain spaces, newlines and any wild ASCII characters, but all other normal chars are accepted, e.g ., = etc (same as CLIENT NAME).

The RESET command does NOT clear these, but they can be cleared to the default by sending a command with a blank string.

Docs PR: redis/redis-doc#2362

[hpatro]

I've seen some client (e.g. Lettuce) use the name field to indicate the client library and operation name=lettuce#ClusterTopologyRefresh. Similarly, maybe they can add libname and version as prefix in the client name itself. What do you think about this @uglide ?

[chayim]

We'll invariably want to pack more (somehow) than name and version into the client connection details. Any chance we can also have some other string based field - something general purpose, for the client's user's need.

[mgravell]

I don't think the HELLO addition is safe; if I issue, TODAY:

HELLO 3 SETLIB foo 1

I get:

ERR Syntax error in HELLO option 'SETLIB'

which means I can't safely use HELLO SETLIB until I know what the server version is, and I can't know the server version until I've said HELLO (since we would normally expect HELLO to be the first command, potentially including the auth etc)

This seems like a fundamental barrier to extending HELLO in any way, since it errors with unexpected tokens.

In theory I guess we could try to make HELLO more forgiving going forward, but for that we'd need to enforce simple pair tokens, so some future version of redis knows how to ignore options it doesn't expect (i.e. skip the next value), so (for example)

HELLO 3 SETLIB foo SETLIBV 1

but that still doesn't help with the "what if you run that against a vToday server that doesn't know about that pattern", plus the result of HELLO wouldn't currently allow the server to indicate which tokens it didn't handle. Being pragmatic, maybe it is better to just consider HELLO closed to addition? Ultimately, once the client has negotiated a protocol and knows the server version, it can always just pipeline a few other handshake commands for things like SETLIB.

[yossigo]

I agree with @mgravell, HELLO is not extensible, so any enhancement to the connection setup needs to rely on additional commands that are pipelined for performance and have errors ignored for backward compatibility.

One way to address that is to introduce HELLOEXT that can be used in addition to HELLO and carry the extensions we can't add to HELLO. We can also make it forward-compatible by ignoring arguments we don't recognize. I'm not sure it has any advantage over ad-hoc extensions like CLIENT SETLIB and anything similar that may come up in the future.

[oranagra]

Following some discussions, i wanna make the following proposal:

1. We add a new command (we can decide on the name later, for all i care it can be HELLO2, HELLOEXT, GREETINGS), which can be sent right after HELLO, or even after AUTH (in the absence of HELLO), or without AUTH (on a non password protected redis).
2. The semantics of this command is that it's ok that it'll fail (e.g. when speaking to an old redis version), and the client should ignore its failure, which means they can send it with a pipeline (without waiting for its response).
3. The command takes multiple key+value argument pairs, and is accepted (returning a simple OK) even if redis doesn't recognize or can't process all of them (some may only be valid for future versions of redis and are ignored).
4. The arguments that this command will handle are always just about setting the current connection state, and never about changing anything in the server state or keyspace. if the command is re-issued or arguments are repeating, the last one wins.
5. If we want (that's open for discussion), we can define that it returns an array with one element for each argument pair, so it can be an array of OK, or it can possibly provide error feedback on the unrecognized arguments. if we do that, it'll also be possible for the client to query some specific server capabilities via this handshake (like get the TLS port, or some other boolean capability). on the other hand, maybe we should drop that idea since the client can also pipeline other command if it want to query specific server capabilities during the handshake.
6. Currently in 7.2 we'll only handle lib-name and lib-ver, and lib-env (can provide OS information or Cloud information for cloud-native code). these are shown in CLIENT LIST and nothing more (they don't affect the server's behavior).
7. conceptually, there's no problem backporting this command to older versions if we (or someone) wants (the client should not make any assumption on whether it is handled or not, just fire and forget)

if we can agree on the semantics quickly, and have some degree of certainty that this design is future proof, we can probably implement it within a day or two!

[oranagra]

This was just discussed in a core-team meeting and was conceptually approved for 7.2 RC1 (next week).
Decided to have it return an array of strings and to avoid adding any querying arguments.
Considering calling it HELLOEXT (hoping someone can suggest something better)

[uglide]

@oranagra My option for the new command name is BREAKTHEICE or MEET :)

[yangbodong22011]

Currently in 7.2 we'll only handle lib-name and lib-ver, and lib-env (can provide OS information or Cloud information for cloud-native code). these are shown in CLIENT LIST and nothing more (they don't affect the server's behavior).

In addition, I want to add a useful information is the client ip, especially in the cloud network, when the client IP passes through the router, Load Balancer, Redis usually cannot see the real IP of the client, which is very important for locating the problem of.

Considering calling it HELLOEXT (hoping someone can suggest something better)

As an SDK developer, I suggest continuing to expand the CLIENT command, like CLIENT SETLIB mentioned in this PR, instead of introducing HELLOEXT:

1. CLIENT SETNAME has been used by a large number of SDK developers and is familiar with it. Continuing to expand CLIENT will obviously reduce the cost of understanding.
2. The current design of HELLOEXT has no reason why it should be used.
3. In any case, do not support HELLOEXT SETLIB and CLIENT SETLIB at the same time, it will be more confusing.

[oranagra]

@yangbodong22011 we prefer one handshake command instead of extending CLIENT.
ideally, we would have been able to extend HELLO, but we can't since it'll be rejected by older versions.

[yangbodong22011]

@yangbodong22011 we prefer one handshake command instead of extending CLIENT. ideally, we would have been able to extend HELLO, but we can't since it'll be rejected by older versions.

For current requirements (allow client report name, version, ip), it is more suitable to be done in CLIENT command.
It is not currently clear what the handshake needs to do? I think we can put that idea on hold for now.

[oranagra]

I agree it's not currently clear. We wanna make it clearer.
Like HELLO command tried to do (return server info etc), but sadly that command didn't consider future extensions..

[mgravell]

we prefer one handshake command instead of extending CLIENT

That ship has already sailed. You already can't necessarily use HELLO, since you don't know the server version. The way we've side-stepped this is basically to pipeline all of HELLO, AUTH and CLIENT, which gets you to the right result regardless of the server, without needing to add latency.

I do kinda agree that simply adding another CLIENT sub-command achieves the same aim without complications. And means we can just keep pipelining.

[oranagra]

i agree there's no practical difference between pipelining CLIENT commands and a HELLOEXT, but the later is cleaner IMHO.

the CLIENT command is a set of very different sub-commands.
SETNAME is innocent (kinds like SELECT only modifies the connection), but it also includes KILL, LIST and others, i'd rather not keep extending it, specifically not with non-admin sub-commands, and certainly not with just metadata.

If i could i would have extend HELLO, but since we can't, the next best thing is to add HELLO2*.
also, since we explicitly document this command as one that can be ignored / extended (by both the client and the server), i think it makes things cleaner than adding a bunch of new CLIENT subcommands.

[oranagra]

@redis/core-team i implemented the changes we discussed and updated the top comment, please take a look
please comment about adding a client-ip field proposed here.
please suggest a better name for the command if you can (i actually like GREETINGS or MEET)

[NickCraver]

@oranagra I'm not sure adding a bunch of commands on one vs. pipelining is actually cleaner. What happens in the exception case? Let's say the client supplies an incorrect field for something, will it throw? Does everything except that set? Does the entire command fail? What's the response the client gets to parse to figure out which subset of the commands succeeded or failed?

I'd argue monolithic commands using a bunch of extensions for separate actions (e.g. setting each piece of metadata) has its own set of issues. If we pipelined n client commands it's very easy to reason about what worked, what didn't, and handle that reasonably - each being isolated and everything that can succeed does. In one big command? I have to do a lot of reasoning to figure that out, if the server response even makes that possible.

Please reconsider extending client instead of setting some pieces of metadata in one spot, others in another, and both handled in very different ways due to the reasons above.

[oranagra]

@NickCraver First, what makes it cleaner is that we're trying to create some mechanism which designed and documented to be used for handshake, and trying to plan ahead and think of how it's gonna be extended.
Throwing each feature in a different / random command is at the very least a source of confusion, specifically when it's part of the messy CLIENT command group. Again, ideally this should have been part of HELLO.

Secondly, as documented above, this new command is specifically aimed to pass metadata, or other arguments which are optional and the client shouldn't usually care if the server accepted them or rejected them. we won't use it for switching protocol or anything like that. these can be extensions to the HELLO command, or additional CLIENT sub-command like NO-TOUCH which we just added.

For the purpose of discussion, i can argue that i agree with you and pipelining several is indeed cleaner, and it's just that i want a dedicated command for these metadata fields, rather than a sub-command of CLIENT.
If i keep this PR exactly the same, but change it from HELLOEXT to CLIENT MEET would that be any different?

[NickCraver]

If I keep this PR exactly the same, but change it from HELLOEXT to CLIENT MEET would that be any different?

Good question. Looking at latest code, it looks to me like a client could choose to pipeline n calls to HELLOEXT here and get the behavior they wanted in terms of per-call error handling (thinking here: it's not only whether the server accepts it as to why a command can fail) so in that it is only the name that seems off. In total, if we're trying to plan ahead for handshake extensions and I can think of 2 categories: per-connection metadata which can fail and continue like is currently in the discussion here, or things that do matter and shouldn't be bundled together with anything else, need dedicated handling, and possibly a round-trip to enforce before things can continue. That second set would hurt perf in startup scenarios like Functions or Lambda where it really matters...so I'm having trouble imaging examples of those but maybe the team has in discussions here. Either way though, if the second category exists, then by intent/criteria wouldn't it not be bundled with this command anyway? Unless that's off - we're back to per-client metadata that doesn't affect server behavior.

For the moment, the best suggestion I have is calling it something like CLIENT SETMETADATA, with the behavior handling in the PR already being good. The reason I disagree with "MEET" or similar (or that these are handshake items) is that there's an assumption (or suggestion) in play that none of these would change during runtime while a connection is active. For example though, someone may promote an environment from failover to active and want to update lib-env to be accurate at any time, the same may be true for any future metadata. Or a server can be patched - that doesn't always mean a reboot, for example a live Linux patch may change versions without reconnecting to Redis and the client may update this on some interval.

My overall suggestion would be to classify this as what it is: client metadata, but not n commands for each. I totally agree the alternative is a mess over time. I'm hoping we can get the same cleanliness goals of a single command while also being clearer about it. If a library wants to just throw this at the wall and not care about any results: that's fine, they can take that route with either option. In case it helps at all - I'm imaging what this looks like for our client, and to the user I'd likely expose this as something like .SetClientMetadata(MetadataType.Environment, "foo"), providing 1 non-variadic API that's pipelined in usage just because that's cleaner both in the hidden handshake code they don't see and at the runtime top level API they will see/use.

Let me just say I really appreciate both the back and forth and willingness to adjust as things as they go in across the board here. It makes me so happy that a ton of care is being taken to get these set-in-stone things as good as possible and that's awesome. Cheers!

[oranagra]

we certainly wanna take the time to make the right decision here, and not just add something we may later regret.

I think the key differences between CLIENT SETMETADATA (or CLIENT SETINFO which i'd prefer), and HELLOEXT (or CLIENT MEET) is whether we wanna let it handle some non-metadata settings in the future or not, so i guess that's what the discussion should focus on.

the fact that it is variadic or not, is not that critical, specifically if it's just metadata, people can just call it multiple times. if it's a handshake, it might be more important to be variadic, but actually the fact we said each of the tuples can be accepted / handled separately, means that they're already meant to be independent.

what i do like about both approaches, is that it's one (or sub) command that handles it all in one place (better for clarity and documentation).

when i was thinking of a HELLO extension with non-binding arguments (in which the command would succeed even if some are unsupported), i was also thinking on non-metadata features, ones that do not change anything significant like breaking the protocol / app (reminds me of possix_fadvise).

Maybe CLIENT NO-TOUCH does fit into that category in some cases (i.e. for redis-cli --bigkeys it'll want to use it if supported, or proceed otherwise if not), while other apps, may want to explicitly abort if it's missing.

anyway, i think this should be the focus of discussion here (metadata only, vs other client specific setting), i'm not certain i see exactly what we wanna do with it, but i do wanna try to plan ahead and avoid any future regrets, breaking changes, or awkward / messy API. that's why i wanted to introduce a command that's flexible and a little bit vague.

[NickCraver]

Agreed! FWIW, I think CLIENT SETINFO is great too.

I think the key differences between CLIENT SETMETADATA (or CLIENT SETINFO which i'd prefer), and HELLOEXT (or CLIENT MEET) is whether we wanna let it handle some non-metadata settings in the future or not, so i guess that's what the discussion should focus on.

when i was thinking of a HELLO extension with non-binding arguments (in which the command would succeed even if some are unsupported), i was also thinking on non-metadata features, ones that do not change anything significant like breaking the protocol / app (reminds me of possix_fadvise).

On the above: what would help me a ton is any examples of non-metadata on the table for the future - are there some y'all have discussed? I'm having trouble coming up with any and that's certainly framing current thoughts around it.

[oranagra]

we discussed this in the core-team and we lean towards CLIENT SETINFO [lib-name|lib-ver] <value>
we would not like to promote remote-ip for now since it requires a broader scope (reflecting SLOWLOG and such).

we're on the fence regarding lib-env since we're not certain how it'll be used.
will it be automatically filled by the client library in some way, or exposed to the application?
should there be any strict format of what we expect there, or left completely open and vague?

let's have a quick async discussion around that and move forward...

[NickCraver]

@oranagra I think that makes a lot of sense - if we don't have some concrete use cases then best to leave it off and not guess. I have some ideas of how it'd be used, basically what is the app environment: dev/stage/prod, or the environment name, or state...lots of variety, but those may be something few others share. I'm curious what others picture as the environment and if it matters that much - the client name is generally the role/system which is unique and mappable to those environments in most cases so it may be entirely superfluous to include. Generally speaking, I'd imagine a lot of the Redis servers that are being connected to are per-environment anyway, with global type of things being the exception to that rule.

Super curious what others had in mind there if anything, just adding some thoughts for down the road :)

[yangbodong22011]

we would not like to promote remote-ip for now since it requires a broader scope (reflecting SLOWLOG and such).

Agree, just share some remote-ip actual scenarios:

• We have a proxy component. The user's link is connected to redis through the proxy. At this time, the real user ip cannot be obtained through the client list (at the same time, our web link monitoring is also invalid).
• Some users start 3 pods on the k8s node. After connecting to Redis, they can only see the node-ip, but cannot distinguish the pod-ip.

If more users are facing the same problem and feedback in this comment, then we can consider a separate PR to do this.

[madolson]

Random comment change, but I'm happy with the API and how it functions.