[redis] Support manual connection management

[CodeBlanch]

[This is an alternative design to what was proposed on #1139]

/cc @lucasoares @lachmatt

Changes

• Expose StackExchangeRedisInstrumentation to allow for manual management of the instrumented Redis connections

Usage

StackExchangeRedisInstrumentation redisInstrumentation = null;

using var tracerProvider = Sdk.CreateTracerProviderBuilder()
    .AddRedisInstrumentation()
    .ConfigureRedisInstrumentation(instrumentation => redisInstrumentation = instrumentation)
    .Build();

using var connection1 = ConnectionMultiplexer.Connect("localhost:6379");
redisInstrumentation.AddConnection(connection1);

using var connection2 = ConnectionMultiplexer.Connect("localhost:6380");
redisInstrumentation.AddConnection(connection2);

Public API Changes

namespace OpenTelemetry.Instrumentation.StackExchangeRedis
{
   // Renamed options class. Technically breaking, but should be low impact
   // assuming people are using untyped syntax: AddRedisInstrumentation(options => ... )
-  public class StackExchangeRedisCallsInstrumentationOptions {}
+  public class StackExchangeRedisInstrumentationOptions {}

+  public class StackExchangeRedisInstrumentation : IDisposable
   {
+    public IDisposable AddConnection(IConnectionMultiplexer connection) {}
+    public IDisposable AddConnection(string name, IConnectionMultiplexer connection) {}
+    public void Dispose() {}
   }
}

namespace OpenTelemetry.Trace
{
   public static class TracerProviderBuilderExtensions
   {
        // Not shown: AddRedisInstrumentation overloads modified for options rename above

+      public static TracerProviderBuilder ConfigureRedisInstrumentation(this TracerProviderBuilder builder, Action<StackExchangeRedisInstrumentation> configure) {}
+      public static TracerProviderBuilder ConfigureRedisInstrumentation(this TracerProviderBuilder builder, Action<IServiceProvider, StackExchangeRedisInstrumentation> configure) {}
   }
}

TODOs

• Appropriate CHANGELOG.md updated for non-trivial changes
• Unit tests
• Changes in public API reviewed

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (main@22f7c56). Click here to learn what that means.
The diff coverage is 98.68%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1193   +/-   ##
=======================================
  Coverage        ?   73.16%           
=======================================
  Files           ?      260           
  Lines           ?     9130           
  Branches        ?        0           
=======================================
  Hits            ?     6680           
  Misses          ?     2450           
  Partials        ?        0           

Impacted Files	Coverage Δ
...eRedis/StackExchangeRedisInstrumentationOptions.cs	100.00% <ø> (ø)
...ckExchangeRedis/TracerProviderBuilderExtensions.cs	91.48% <96.55%> (ø)
...ry.Exporter.Geneva/Metrics/GenevaMetricExporter.cs	74.55% <100.00%> (ø)
...mentation/RedisProfilerEntryToActivityConverter.cs	79.79% <100.00%> (ø)
...dis/StackExchangeRedisConnectionInstrumentation.cs	96.92% <100.00%> (ø)
...ExchangeRedis/StackExchangeRedisInstrumentation.cs	100.00% <100.00%> (ø)

[alanwest]

Approved, but just sharing an idea. Curious about your thoughts.

[alanwest]

I don't have strong feelings against this pattern, but it does seem a bit awkward to me. Given that we add instrumentation to a tracer provider it might feel more natural if we had a way to ask the tracer provider for a handle to the instrumentation. Something like

using var tracerProvider = Sdk.CreateTracerProviderBuilder()
    .AddRedisInstrumentation()
    .Build();

using var connection1 = ConnectionMultiplexer.Connect("localhost:6379");
tracerProvider.GetInstrumentation<StackExchangeRedisInstrumentation>().AddConnection(connection1);

But I also don't feel strongly about adding a GetInstrumentation method to TracerProvider either since this Redis instrumentation is the only instrumentation where this seems to make sense today - and of course most of our instrumentation classes are not public anyways.

This is just food for thought, at the end of the day I understand this is the advanced scenario.

[CodeBlanch]

It is a very interesting idea. We had the need somewhere (PrometheusExporter?) to find the registered exporter so this type of thing does seem to have some utility. But I'm not sure we have seen enough need to justify a public API.

This particular API (ConfigureRedisInstrumentation) isn't actually needed at all. Users could do this...

        var builder = Sdk.CreateTracerProviderBuilder()
            .AddRedisInstrumentation();

        StackExchangeRedisInstrumentation redisInstrumentation = null;

        ((IDeferredTracerProviderBuilder)builder).Configure((sp, builder) =>
            redisInstrumentation = sp.GetRequiredService<StackExchangeRedisInstrumentation>());

        using var tracerProvider = builder.Build();

I just thought it was so nasty we should have a helper to clean it up 🤣

Using IServiceCollection style it is much nicer:

appBuilder.Services.AddOpenTelemetry().WithTracing(builder => builder.AddRedisInstrumentation());

var app = appBuilder.Build();
var instrumentation = app.Services.GetRequiredService<StackExchangeRedisInstrumentation>();

The reason the first one is so nasty is in the "Sdk.Create" style we don't expose the IServiceProvider directly so it is kind of hard to get at. Might also be a useful public API addition to expose the IServiceProvider behind the provider instance?

[utpilla]

Do we want to allow multiple connections? If yes, we should probably update the thread name to be distinct when instantiating multiple instances of StackExchangeRedisConnectionInstrumentation.

[CodeBlanch]

Yes one of the goals was to support multiple connections. You can actually have multiple connections today by calling AddRedisInstrumentation repeatedly. Let me tackle this as a follow-up. Thinking maybe I can move the thread up a level so we don't have as many.

[YayBurritos]

When our application calls AddRedisInstrumentation multiple times (to instrument different connections), we get duplicate spans in our traces (one for each instrumented connection even though the Redis call is only being directed to one of those connections). Is there a way to avoid the duplicate spans?

Seeing this duplicate span behavior so far in the following versions:

• OpenTelemetry.Instrumentation.StackExchangeRedis v1.0.0-rc9.9
• OpenTelemetry.Instrumentation.StackExchangeRedis v1.0.0-rc9.8
• OpenTelemetry.Instrumentation.StackExchangeRedis v1.0.0-rc9.7

Can continue testing up to the current version if that helps.

I submitted this PR (#2001) in an attempt to address what we are seeing in our application, but the changes haven't been well received, so they may not be merged.

Open to other solutions, but so far coming up empty.

[CodeBlanch]

This feature is now available in OpenTelemetry.Instrumentation.StackExchangeRedis v1.0.0-rc9.9