Setting client info does not work properly for re-connects and multiple connections

[atakavci]

NRedisStack sets the client info by calling ClientSetInfoAsync when initial connection is established.

the settings given below are captured via monitor command from the initial connection.

1743610438.580669 [0 192.168.0.1:56054] "CLIENT" "SETNAME" "application_host(SE.Redis-v2.8.24.3255)"
1743610438.580682 [0 192.168.0.1:56054] "CLIENT" "SETINFO" "lib-name" "SE.Redis"
1743610438.580684 [0 192.168.0.1:56054] "CLIENT" "SETINFO" "lib-ver" "2.8.24.3255"
1743610438.580688 [0 192.168.0.1:56054] "CLIENT" "ID"

The status for these settings are tracked with a static flag which holds the wrong value for re-connects and multiple connection scenarios like cluster etc.. This ends up with only initial connection gets the client info initialized.

[mgravell]

SE.Redis has a separate client configuration API that is ultimately going to squabble with NRedisStack, but on the plus side: the SE.Redis version knows about things like the cluster state, so can keep things correct.

In particular, DefaultOptionsProvider has:

public virtual string LibraryName {get;}

public string ClientName => defaultClientName ??= GetDefaultClientName(); // memoized
protected virtual string GetDefaultClientName() {...}

Changing the lib-ver is currently not supported, but that is proposed here

So this can basically be achieved using an options-provider (give or take an API extension for lib-ver). I will familiarize myself with the typical usage to see whether this works, vs a new API being necessary.

[mgravell]

I will note, however, a separate complication: there's an inherent difficulty if you overstomp the lib-ver, in that it may still be highly relevant and useful to know what the underlying SE.Redis version is (we can at least infer SE.Redis from NRedisStack, so overstomping the client is less of a concern). I wonder if this should be additive rather than replacement. It is not sufficient to infer the SE.Redis version, since this can be rev'd via package-ref, i.e. you could use NRedisStack 1.0.0 with SE.Redis vToday

There's already an AddLibraryNameSuffix API, which appends to the library name if needed and publishes relevant updates. I suspect an options provider is the better approach, though.

[atakavci]

the issue with existing implementation here is, until there is a call to NRedisStack API, we don't know of what is going on with client or IDatabase instance. By the time NRedisStack API is called, it's already too late to introduce an options provider for the initialization process.
So it's no use when you casually create an IDatabase from ConnectionMultiplexer .

[mgravell]

@atakavci an option you might not be aware of: IConnectionMultiplexer.AddLibraryNameSuffix, which:

• ✔ is available from IDatabase via .Multiplexer
• ✔ has a hash-set internally so it doesn't re-add duplicates (i.e. it is safe to call more than once)
• ✔ knows about clusters, reconnects, etc
• ✔ can be called at any time - additive only
• ❌ (doesn't work on existing pub/sub RESP2 connections due to mode switch - at least until they reconnect next)

We could call AddLibraryNameSuffix("NRedisStack") (or "NRS", whatever), and that gets combined into the client's string, for example as SE.Redis-DC-HC-NRS-OC (for a connection using ASP.NET output-caching, distributed-caching, hybrid-caching and NRedisStack - ordering is alphabetical). Thoughts?

Note: right now there's a lock around the hash-check, but if we don't have a better option than "call it every time", I could replace that with an immutable hashset approach in < 5 minutes.

[atakavci]

@mgravell,
for sure its better to have something than nothing but unless we have the same impact that we get with;

1743610438.580669 [0 192.168.0.1:56054] "CLIENT" "SETNAME" "application_host(SE.Redis-v2.8.24.3255)"
1743610438.580682 [0 192.168.0.1:56054] "CLIENT" "SETINFO" "lib-name" "SE.Redis"
1743610438.580684 [0 192.168.0.1:56054] "CLIENT" "SETINFO" "lib-ver" "2.8.24.3255"
1743610438.580688 [0 192.168.0.1:56054] "CLIENT" "ID"

its doesnt fully qualify for what is intended here.
i dont know why and how an application would rely on client info but i like to have this without a breaking change.
Like having SetLibraryName/SetLibraryVersion is provided in similar fashion with AddLibraryNameSuffix from SE.Redis, how about that?

[mgravell]

I mean... it is *possible*, but it seems ... kinda weird, and subverts the existing APIs for controlling the client and version, which can already be set via ConfigurationOptions and (separately) the default options provider. At what point do we say a connection "becomes" NRedisStack vs SE.Redis or SomeOtherRandomModuleProvider - or (worst case), a caller using two different module providers, each of which keeps changing the client. That's what I was trying to guard against via the suffix API. The "when" on the existing API is... odd, with static state and whatnot. Let's grab a moment next week to go over what the expected behaviour is here, and why. If there's an explicit intent from the caller to change name, then... fine I guess, but there's a few places where it just changes magically and somewhat randomly. If I understand the intent better, I can help more.

[mgravell]

Basically, if we're talking about an explicit external caller suddenly wanting to change the client name to something specific, then sure: we can add an API to SE.Redis that makes it know what is going on, because right now it thinks *it* (SE.Redis) owns that, and it is going to squabble. If we're talking about implicit magic that just happens: I think we should find a way to make that work nicely with other tools that might be sharing the connection(s), and something *like* the suffix API achieves that.

[mgravell]

As an example of an outcome that is *friendly* to shared connections, consider maybe: AddLibrary(string name, string version) which *composes* a set of (presumably alphabetical by name) pairs, so the library name might end up as "NRedisStack/SE.Redis/XenoModule" and the version might end up as "1.2/2.7.2/0.4-alpha2", but which is safe and efficient with no over-stomping. Is that an acceptable outcome?

[mgravell]

Relevant SE.Redis design document