IProfiledCommand data expansion

[CodeBlanch]

Resolves #1448

For consideration...

• Added Script, Keys, and Values to the IProfiledCommand interface so that profilers/tracers have more data to play with.
• Added IsFaulted and Exception to the IProfiledCommand and fixed up the ConnectionMultiplexer so that it will send messages to the profiler that fail because a connection is unavailable.

/cc @SergeyKanzhelev @MikeGoldsmith @cijothomas

[SergeyKanzhelev]

LGTM

[ejsmith]

Yes, please! This would be super helpful.

[NickCraver]

I profiled here and agreed we're fairly minimal on overhead - what we want to discuss is "does this look like the path to OpenTelemetry?" (and should we, be on such a path?) If we're aiming to support an adapter (the Azure crew has asked us about this as well), let's make sure this adds the bits to do that in an efficient way. Going to be talking to them coming up and want to cover this area.

[ejsmith]

Well, I think ideally, the library would be using ActivitySource and generating activities efficiently that don't incur any perf hit when not being listened to. But since that would be a substantial effort, it seems like this would be a really nice stop gap.

[NickCraver]

Talked about this with the Azure Redis crew yesterday, the plan is to spike what OpenTelemetry/OpenTracing would look like on top of this and ensure that it's the correct interface changes to make (or investigate additional ones).

Why? Because we're going to break an interface here to do this, so let's be sure and do it once. This may be just fine as is, but we're not sure until trying a consumer. We're getting some help on time there and then we'll proceed with profiling updates here :)

[ejsmith]

Would love to see the profiled command stuff go away completely and just use activities with ActivitySource. Could probably use an ActivitySourceListener internally to get IProfiledCommand to still work for backwards compatibility without having to have both things throughout the code.

[NickCraver]

@ejsmith If it was as low in allocations, I'd agree, but that's a) breaking, and b) not realistic given Activity source's structure. If someone wants to spike it and try to get allocations close I'd be super curious, but given my experience thus far with it...I'm not sure how that's possible. It's designed to be much more generic and that's a big tradeoff.

I'd also like to point out bias there, ActivitySource may be what you use for example, for others it's a desire for Open Telemetry output, etc. Everyone wants their thing - as long as we have something that's low overhead anyone can adapt (and there are adapters already out there), that's probably where we'll sit. Activity and tracking gets even more complicated on retries and such, which is another item to consider.

[ejsmith]

@ejsmith If it was as low in allocations, I'd agree, but that's a) breaking, and b) not realistic given Activity source's structure. If someone wants to spike it and try to get allocations close I'd be super curious, but given my experience thus far with it...I'm not sure how that's possible. It's designed to be much more generic and that's a big tradeoff.

I'd also like to point out bias there, ActivitySource may be what you use for example, for others it's a desire for Open Telemetry output, etc. Everyone wants their thing - as long as we have something that's low overhead anyone can adapt (and there are adapters already out there), that's probably where we'll sit. Activity and tracking gets even more complicated on retries and such, which is another item to consider.

ActivitySource is exactly what OpenTelemetry uses. I haven't done perf tests, but I would imagine that ActivitySource by itself has to be very performant or the .NET team wouldn't be able to add it throughout the code base. @CodeBlanch @cijothomas thoughts?

[CodeBlanch]

Basically, if there are no listeners ActivitySource will no-op (return null Activity). If something isn't sampled, same. So you only pay for what you use 😄 If we code StackExchange.Redis to use ActivitySource then the hosting application can use OpenTelemetry or whatever to listen to the source and it will magically begin emitting Activity instances at that time.

[aKzenT]

There has not been any activity on this. Is there something missing to get this merged or an alternative for it? Having more information on profiled commands would be super helpful for us, regardless of what interface provides it. ActivitySource would be nice, but we could just adapt to whatever is available.

[NickCraver]

@aKzenT What information are you looking for? As far as I know, the team working on this hasn't had the cycles to come back to an ActivitySource version for a proposal.

[aKzenT]

Basically what would be really helpful would be to get at least the key argument for the most common commands. We have a web application with different components using redis. To understand and differentiate between the different commands the key would be most helpful. If other arguments are available as well that would be an added benefit.

[aKzenT]

@NickCraver The OpenTelemetry infrastructure for .NET seems to be a lot more stable now than it was a year ago. Is there any chance to revisit this topic? There is a contrib library that works quite well for us, but it has to do some ugly reflection/IL stuff to make it work:
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Instrumentation.StackExchangeRedis/Implementation/RedisProfilerEntryToActivityConverter.cs

[NickCraver]

@aKzenT I'm revisiting the metrics stuff now, but haven't looked at tracing at all - I'm not sure where we'll land here (but we wouldn't take a dependency in the main library in either case).