StackExchange.Redis instrumentation for .NET Core 3.1+

[Kielek]

Why

Fixes #947

What

StackExchange.Redis instrumentation for .NET Core 3.1+

Tests

CI test.
Tested also locally on StackExchange.Redis 2.6.48 and StackExchange.Redis 2.0.495

Checklist

• CHANGELOG.md is updated.
• Documentation is updated.
• New features are covered by tests.

[pellared]

I think it is worth to update the dependabot config. More: #969

[Kielek]

I think it is worth to update the dependabot config. More: #969

d331a75

[dszmigielski]

LGTM

[nrcventura]

One thing that we should consider before enabling this instrumentation library out of the box, is to see how it affects performance of the application. A problem that I have seen with other Redis instrumentation is that when the instrumentation is enabled, it can cause the application to have a significant increase in network usage as compared to when no instrumentation is enabled. This can happen if too much code from the instrumentation is running between the different calls to Redis. If the performance impact is not acceptable, we should consider not enabling this instrumentation by default, or to wait until a later time to support Redis (until we determine a better approach).

[Kielek]

Have you tested the behavior when StackExchange.Redis version is 2.0.0 <= ver < 2.1.58?

It was really good question. It was working only partially. It should be fully supported after aad6c78

Some important information

• OpenTelemetry.Intrumentation.StackExchangeRedis references StackExchange.Redis v. 2.1.58+
• All 2+ StackExchange.Redis` releases are released with AssemblyVersion = 2.0.0.0
• If the AsemblyVersion is not changed, it can be replaced without any issues - https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/versioning#assembly-version

Previously I have tried to instrument public API, but it is much harder. For some versions public methods calls other publics method, so there were a chance to register instrumentation twice.

Follow up candidate - as AssemblyVersion is stable for this library, we should be able to add support also for .NET Framework without any changes (until 3.0 version is released).

[Kielek]

One thing that we should consider before enabling this instrumentation library out of the box, is to see how it affects performance of the application. A problem that I have seen with other Redis instrumentation is that when the instrumentation is enabled, it can cause the application to have a significant increase in network usage as compared to when no instrumentation is enabled. This can happen if too much code from the instrumentation is running between the different calls to Redis. If the performance impact is not acceptable, we should consider not enabling this instrumentation by default, or to wait until a later time to support Redis (until we determine a better approach).

@nrcventura, could you please share the reproduction app or verify this instrumentation yourself?

I have executed following code in console application with "StackExchange.Redis" Version="2.6.48, redis:7.0.4, .NET 6.0 hosted on docker on the same machine as console app.

        var connectionString = $@"127.0.0.1:{redisPort}";

        using (var connection = await ConnectionMultiplexer.ConnectAsync(connectionString))
        {
            for (int i = 0; i < 500; i++)
            {
                var db = connection.GetDatabase();
                await db.StringGetAsync("zzzzz");
            }
        }

Number of captured packets send/received to/from redis port is around 2050. Both for instrumented and normal application.

[nrcventura]

could you please share the reproduction app or verify this instrumentation yourself

The reproduction app was just an aspnetcore app that used the code snippet in the linked issue. To reproduce the issue, we just called the endpoint multiple times, and checked the number of packets.

Number of captured packets send/received to/from redis port is around 2050. Both for instrumented and normal application.

It's good that we're not seeing a difference in packets with the code that you used. However, your code is not really attempting to execute these requests in parallel. I think that the easiest way to test the situation, with enough of a natural delay (so that not everything is executed in parallel immediately), is to create an aspnetcore app with an endpoint containing the following code. Then you can just hit the endpoint in a browser and refresh a few times.

private static StackExchange.Redis.ConnectionMultiplexer _connection = StackExchange.Redis.ConnectionMultiplexer.Connect(connectionString);

public async Task<string> GetValueAsync(string key)
{
    if (!_connection.IsConnected)
    {
        return null;
    }

    var db = _connection.GetDatabase();
    var val = await db.StringGetAsync(key, flags: CommandFlags.PreferSlave);

    if (val.HasValue)
    {
        return val;
    }

    return null;
}

[Kielek]

@nrcventura, for the application based on your code, the number packets is on the same level.

BTW application even without any activity is producing a lot of packets related to Keeping connection alive.