OpenTelemetry Integration

[roma-glushko]

Following up our previous conversation with Ali from #158 (comment)

[roma-glushko]

Hey @alisaifee let's keep this discussion going 🚀

I think this is fine as long as it's not a hard requirement for installation. We could provide an install extra so that users could do for example pip install coredis[otel] and automatically get the dependency that is tested against and the part of the code that enables instrumentation could silently be disabled when the dependency is not available. Something like this already exists for runtime type checking via beartype - here.

Do you believe this is a good fit for tracing as well? Tracing is going to be called in a lot of places, so we may end up having a lot of try-except or ifs there which in turn could make code less readable.

How do you feel about the following way: we could define a tracer class that would be always used in coredis codebase (here is an example of that in https://github.com/encode/httpcore/blob/master/httpcore/_trace.py#L8). That class would do nothing by default but it would provide extensible points into places where it was called. Then any instrumentation (whether it's OpenTelemetry, Prometheus, StatsD, etc) could patch (or simply provide a new implementation of that tracing component with the same API) that with some specific logic of handling traces.

I feel like it's the way to keep codebase simple while still extensible. How do you feel about this idea?

Let's settle down on the approach and I will be able to start working on the draft PR for this change.

[alisaifee]

Thanks for restarting the discussion @roma-glushko!

I prefer your suggestion of having a base telemetry implementation that could be extended for different collectors.

How this will be configured could be done either by:

1. Adding a new parameter to the Redis, RedisCluster, Sentinel constructors
2. Using a global configuration via the Config singleton (reference)

The minor advantage of using approach 2 is that we don't have to pass the instance through the stack and it can be simply accessed anywhere in the codebase as Config.tracer for example. The argument against this ofcourse is that you wouldn't be able to configure different types of tracers per client in a running process.

[roma-glushko]

That's great Ali 👍

The minor advantage of using approach 2 is that we don't have to pass the instance through the stack and it can be simply accessed anywhere in the codebase as Config.tracer for example.

I don't believe we need to choose between those two approaches. They are complementary to me. We could allow both Configs and concrete instances of Redis to receive the tracer instance, but the later will have a precedence if both are configured. I have not delved into the codebase deeply yet, but I hope this is something we could do.