Configuration: An idea on separation by NickCraver · Pull Request #1987

Configuration: An idea on separation by NickCraver · Pull Request #1987 · StackExchange/StackExchange.Redis

This is just an idea pitch to see if the direction even works. The intent is for a consumer or wrapper library to specify a default options provider (or extend the built-in ones). For example Azure could override IsMatch and maintain its own domain list, as could AWS, etc.

Problems with this:
- Modifying the `KnownProviders` list on `DefaultOptionsProvider` is awkward, but ConfigurationOptions can also get a `.Defaults` set directly, or...other ideas? Maybe `GetForEndpoints` should get on `ConfigurationOptions` as a `public static` instead?
- Maintenance events are not fully decoupled: they're calling `internal` bits on `ConnectionMultiplexer`, so we'd either need to expose those in some way (e.g. being able to trigger a reconfigure) or...something else.
- The client name including Azure role bits are still on `DefaultOptionsProvider`, so that client names showing role instances are still global of where the client runs, and not narrowed down to only when connecting to Azure endpoints.

Overall I think the `ConfigurationOptions` bits are net cleaner but maintenance events aren't capable of being moved out. If we exposed `ReconfigureAsync` via `ServerMaintenanceEvent` base in some way, it could be extensible in that way without over-exposing the deep internals.

Needs eyes and thoughts, this may not be the way we go at all.

NickCraver added a commit that referenced this pull request

Feb 11, 2022

There seems to be just 1 case not covered by the IncludeDetailsInExceptions option on ConnectionMultiplexer - this remedies that. The option is on by default so this shouldn't break people like I thought initially.

Overall, we should probably also move this option to ConfigurationOptions and defaults if we do that (#1987).

NickCraver added a commit that referenced this pull request

Feb 15, 2022

There seems to be just 1 case not covered by the `IncludeDetailsInExceptions` option on `ConnectionMultiplexer` - this remedies that. The option is on by default so this shouldn't break people like I thought initially.

Overall, we should probably also move this option to `ConfigurationOptions` and defaults if we do that (#1987).

I'm not 100% sure on the helper exposure here but figured it was handy - thoughts?

@philon-msft here's what I had in mind for extensibility. We probably don't _need_ to override it with these defaults not, but a provider could if they wanted. IMO, no need for "lib" because _all_ connections are from a library, just felt more natural to spell our SE.Redis when doing it but we can tweak whatever - main thing is the layout here - does this make more/less sense?

Needs more, but getting initial coverage.

This makes a ServerMaintenanceEvent actually something you can implement outside the lib (TODO: assess naming), fixes Defaults cloning, and makes AfterConnectAsync more extensible (no internals anymore).

Misc fixes are tidying other found things.