Add more error information when the cast fails

[rkarthick]

When redis/envoy proxy servers return error, currently we do not propagate the error string to the client. Client gets an errors

Cannot convert to RedisValue: Error

It will be nice to propagate the error that is sent by the upstream servers here (redis or proxy) to the client for better observability.

After this PR we should see the errors similar to the following:

Cannot convert to RedisValue: Error: upstream link failure

[mgravell]

I think this is probably OK for error scenarios, but: if I understand the PR, this could also potentially include the value in the case of non-error types. That is undesirable, as it could leak data into exception logs - or at the very least, it can only be done if the relevant "include detail in exceptions" flag is set. IMO we should explicitly limit this change to error types. If I've misunderstood: let me know!

[mgravell]

Thanks for tweaking that.

Secondary question: how can we observe this behaviour? Normally, I would have expected the library to have already converted this to an exception, so I'm a little surprised that we're seeing this happen at all. Is there any specific scenario where you're seeing this? And: is this testable?

[rkarthick]

Thank you for feedback and request for more details.

I think the error message from server is getting converted rightly as error here: https://github.com/StackExchange/StackExchange.Redis/blob/main/src/StackExchange.Redis/Message.cs#L539

May be the library throws an error only when it is not able to RESP decode? https://github.com/StackExchange/StackExchange.Redis/blob/main/src/StackExchange.Redis/PhysicalConnection.cs#L1883

Repro steps below:

1. Connect to redis envoy proxy which fronts 3 redis masters and 3 redis replicas
2. Bring down one of the masters and its replica
3. Try to get a key that belongs to that master
4. Envoy proxy returns an error upstream link failure

https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/other_protocols/redis#failure-modes

$ redis-cli MGET a b c d e
1) "alpha"
2) "bravo"
3) (error) upstream failure
4) (error) upstream failure
5) "echo"

Testing would be not straightforward using the current CI setup (technically we could modify supervisord to disable restarts and kill -9 one of the redis servers to test it). However I am not sure if we wanted to do that for the exception scenario.

I am pushing in a testcase for the error too. Do let me know if that makes sense

[NickCraver]

@rkarthick We still have a fundamental issue here in that this isn't checking if detailed exceptions are enabled - it's unconditionally logging the information which could be sensitive. For instance if ConfigurationOptions.IncludeDetailInExceptions is false, this shouldn't log. But...it's not easy to get that here, without adding overhead (e.g. a ref which gets very tricky/expensive/dangerous here). Without this, we can't add the information to the exception since that'll cause problems with users in the multi-tenant/security-sensitive environments.

[rkarthick]

Closing this out as per @NickCraver's comment