[Instrumentation.StackExchangeRedis] Connection registration changes

[lachmatt]

Addresses open-telemetry/opentelemetry-dotnet#1900 (comment)

Changes

Add an ability to track connections created after tracer is initialized.

• Appropriate CHANGELOG.md updated for non-trivial changes
  * [ ] Design discussion issue #
  * [ ] Changes in public API reviewed

[codecov]

Codecov Report

Merging #1139 (9f159b9) into main (5907573) will increase coverage by 0.08%.
The diff coverage is 94.28%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1139      +/-   ##
==========================================
+ Coverage   72.13%   72.22%   +0.08%     
==========================================
  Files         230      231       +1     
  Lines        8448     8474      +26     
==========================================
+ Hits         6094     6120      +26     
  Misses       2354     2354              

Impacted Files	Coverage Δ
...ckExchangeRedis/TracerProviderBuilderExtensions.cs	92.15% <93.33%> (+3.69%)	⬆️
...ation.StackExchangeRedis/ConnectionInstrumenter.cs	100.00% <100.00%> (ø)
...ngeRedis/StackExchangeRedisCallsInstrumentation.cs	96.82% <100.00%> (-0.10%)	⬇️

... and 1 file with indirect coverage changes

[Kielek]

There is no package owner for StackExchangeRedis.

[pjanotti]

The main question is why is necessary to return an instrumenter, since it still has usability problems. Ideally one should be able to just use an extension method on the connection, something like connection.WithOpenTelemetryInstrumentation().

[pjanotti]

Returning an object that is needed to instrument the connection is still an usability problem. Is that really required?

[lachmatt]

I think this is a minimal solution to the problem linked in description. Currently, some kind of reference to an instrumentation instance is required to instrument a connection.

[pjanotti]

nit: remove blank line

[CodeBlanch]

@lachmatt

Hey it looks like we already have IServiceProvider support in OpenTelemetry.Instrumentation.StackExchangeRedis. Couldn't you achieve essentially what this PR is seeking to enable by using IServiceCollection \ IServiceProvider?

Manual creation

using var provider = Sdk.CreateTracerProviderBuilder()
   .ConfigureServices(services => services.AddSingleton<IConnectionMultiplexer>(sp => RedisHelper.InitializeConnection(sp)))
   .AddRedisInstrumentation()
   .Build();

Hosting creation

appBuilder.Services.AddSingleton<IConnectionMultiplexer>(sp => RedisHelper.InitializeConnection(sp));
appBuilder.Services.AddOpenTelemetry()
   .WithTracing(builder => builder.AddRedisInstrumentation());

[lachmatt]

@CodeBlanch
Issue linked in the description mentions a scenario when connection is not available at startup, but is created on (first) demand later in an app lifecycle. Changes from this PR would help with that - instrumentation no longer has connection as a dependency, and ConnectionInstrumenter is returned that allows you to reference created instrumentation instance.
I don't think that changes from your suggestion alone would work in that scenario (as connection still would have to be created when instrumentation is being initialized at startup).

[CodeBlanch]

@lachmatt It is really up to the app. In that snippet I sent above I'm registering a factory for the connection into the service collection. So the connection won't actually be started until that factory is executed (when it is first requested from the IServiceProvider).

[pjanotti]

@CodeBlanch let me check if I understand your suggestion. You propose adding a static method, RedisHelper.InitializeConnection, to OpenTelemetry.Instrumentation.StackExchangeRedis, correct? This method will take care of "instrumenting" the connection. If that is the case I agree that it will make the usability of the package much better: it removes the need to pass the connection and also the out parameter currently being added in this PR.

In retrospect, it is not a good practice to pass instances created using the types from the targeted library in the extension methods for the OTel builders, see open-telemetry/opentelemetry-dotnet#1900 and open-telemetry/opentelemetry-dotnet#4347 (comment)

A step further will be to mark the AddInstrumentation overload receiving the connection as obsolete, per opentelemetry-dotnet/VERSIONING.md.

[CodeBlanch]

let me check if I understand your suggestion. You propose adding a static method, RedisHelper.InitializeConnection, to OpenTelemetry.Instrumentation.StackExchangeRedis, correct?

Not exactly. What I'm saying is this code is already hooked up to check the IServiceProvider for the IConnectionMultiplexer so if users don't want to supply the instance when they call AddRedisInstrumentation they can use the IServiceCollection to manage it.

Let's take the example from the README showing the new API in use:

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

        // Connect to the server.
        using var connection = ConnectionMultiplexer.Connect("localhost:6379");
        connectionInstrumenter.Instrument(connection);

Today you could do this to accomplish the same:

        using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .ConfigureServices(services => services.AddSingleton(sp => ConnectionMultiplexer.Connect("localhost:6379")))
            .AddRedisInstrumentation()
            .AddConsoleExporter()
            .Build();

A more useful real-world example would probably be more like...

appBuilder.Services.AddSingleton(sp => {
   var myRedisOptions = sp.GetRequiredService<IOptions<RedisOptions>>().Value;
   return ConnectionMultiplexer.Connect(myRedisOptions.Endpoint);
});

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

[pjanotti]

Thanks, now I see what you are saying. This should satisfy many (most?) use cases since typical server applications are expected to be using an IServiceProvider. Let's hear what people affected by #1900 think: @tbolon @philipbawn @adamhathcock @lucasoares @Norbe3t, please, let's know if the usage of IServiceProvider as suggested above fixes the issue for your case.

From my perspective, it still forces the call to ConnectionMultiplexer.Connect to happen in specific places. What I really would like is to decouple the connection instrumentation from the tracer builder extension method. Perhaps, not as important for StackExchangeRedis which typically uses a singleton for the connection object but developers may see it as a template and repeat the pattern.

[lucasoares]

Thanks, now I see what you are saying. This should satisfy many (most?) use cases since typical server applications are expected to be using an IServiceProvider. Let's hear what people affected by #1900 think: @tbolon @philipbawn @adamhathcock @lucasoares @Norbe3t, please, let's know if the usage of IServiceProvider as suggested above fixes the issue for your case.

From my perspective, it still forces the call to ConnectionMultiplexer.Connect to happen in specific places. What I really would like is to decouple the connection instrumentation from the tracer builder extension method. Perhaps, not as important for StackExchangeRedis which typically uses a singleton for the connection object but developers may see it as a template and repeat the pattern.

Yeah, for me this will not work. I have several applications where I use a custom-made code to create different redis connections and we don't use a service provider. The major reason for this is because of many applications we migrated from very old dotnet versions.

I would like to enable the instrumentation statically anywhere in my code and I really think this would be the best case scenario and work for all use cases...

[CodeBlanch]

@lucasoares

and we don't use a service provider

So with OTel .NET SDK there is always an IServiceCollection \ IServiceProvider! Either one shared with the host (services.AddOpenTelemetry() style) or the SDK creates its own (Sdk.Create* style).

I think if we want to add an extension point that is fine, but we should use the DI API(s) because they are more standard.

I took a stab at what it might look like: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/compare/main...CodeBlanch:stackexchangeredis-creation-api?expand=1

Using that design we can accomplish the example on this PR as...

       StackExchangeRedisInstrumentation? redisInstrumentation = null;

       using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .AddRedisInstrumentation()
            .AddConsoleExporter()
            .ConfigureServices(services => services.AddRedisInstrumentationCreatedAction(i => redisInstrumentation = i))
            .Build();

        // Connect to the server.
        using var connection = ConnectionMultiplexer.Connect("localhost:6379");
        redisInstrumentation?.AddConnection(connection);

Now I know what you are thinking right now. That may seem more complex than what you already have on this PR. Maybe! But I'm not really that concerned with your "old" style of doing things (sorry 🤣). What I want to make sure is that new styles work really well. Old styles should also work, but can be ugly.

What the new design allows us to do...

builder.Services.AddOpenTelemetry();
builder.Services.AddMyRedisLibrary();

With some implementation like...

public static class RedisExtensions
{
    public static IServiceCollection AddMyRedisLibrary(this IServiceCollection services)
    {
        services.AddSingleton<IRedisConnectionManager, RedisConnectionManager>();
        services.AddRedisInstrumentationCreatedAction((sp, name, instrumentation) =>
        {
            var redisConnectionManager = (RedisConnectionManager)sp.GetRequiredService<IRedisConnectionManager>();
            redisConnectionManager.AddInstrumentation(name, instrumentation);
        });
        services.ConfigureOpenTelemetryTracerProvider(
            (sp, builder) => builder.AddRedisInstrumentation());

        return services;
    }
}

public interface IRedisConnectionManager
{
    IConnectionMultiplexer GetConnection(string name);
}

internal sealed class RedisConnectionManager : IRedisConnectionManager
{
    private readonly Dictionary<string, StackExchangeRedisInstrumentation> instrumentation = new();

    public IConnectionMultiplexer GetConnection(string name)
    {
        IConnectionMultiplexer connection = this.FindOrCreateConnectionForName(name);

        if (this.instrumentation.TryGetValue(name, out var instrumentation))
        {
            instrumentation.AddConnection(connection);
        }
        else if (this.instrumentation.TryGetValue(string.Empty, out instrumentation))
        {
            instrumentation.AddConnection(connection);
        }

        return connection;
    }

    public void AddInstrumentation(
        string name,
        StackExchangeRedisInstrumentation instrumentation)
    {
        this.instrumentation.Add(name, instrumentation);
    }

    private IConnectionMultiplexer FindOrCreateConnectionForName(string name)
    {
        throw new NotImplementedException();
    }
}

Maybe that example is more compelling? Let me know what you think 😄

[lucasoares]

@CodeBlanch I don't think it is complex at all. If can make it static I think this should be enough..

I'm like with 1 month using C# and dotnet and I'm not aware of the 'ecosystem-way' of doing things haha

I'm helping a team to instrument their applications, which happened to be kind old so they couldn't use the services.AddOpenTelemetry() style but the other one. The only feature missing for them is to be able to instrument many StackExchange.Redis connections in the code they have, which are created on-demand later after startup.

[lachmatt]

@CodeBlanch
I think your suggestion from #1139 (comment) enables the scenarios that my changes from this PR tried to enable. Would you be willing to create a new PR with your changes, so that this one can be closed?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[lucasoares]

@CodeBlanch any update?