opentelemetry-dotnet-contrib icon indicating copy to clipboard operation
opentelemetry-dotnet-contrib copied to clipboard

Support Redis instrumentation when IConnectionMultiplexer is added with keyed service

Open Kahbazi opened this issue 1 year ago • 3 comments

Issue with OpenTelemetry.Instrumentation.StackExchangeRedis

Is this a feature request or a bug?

  • [x] Feature Request
  • [ ] Bug

Currently there's two way to have Redis instrumentation. With passing the IConnectionMultiplexer instance to the method or By registering IConnectionMultiplexer to the IoC Container.

In .Net 8 there's support to keyed services in IoC Container. It would be nice to have support for instrumentation for IConnectionMultiplexer that is registered as a keyed service.

I suggest adding this API

public class TracerProviderBuilderExtensions
{
   public static TracerProviderBuilder AddRedisInstrumentation(
        this TracerProviderBuilder builder,
        string? name,
        IConnectionMultiplexer? connection,
        Action<StackExchangeRedisInstrumentationOptions>? configure,
        object? serviceKey)
}

And this line need to change so that if serviceKey is available get the IConnectionMultiplexer with servicekey.

https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/c3a4f335f5a1f4fc784d44cc99b5bf44e872a66c/src/OpenTelemetry.Instrumentation.StackExchangeRedis/TracerProviderBuilderExtensions.cs#L135

Also to support this the OpenTelemetry.Instrumentation.StackExchangeRedis project need to add .NET 8 to its target framekwork.

I'm willing to send a PR for this feature if approved.

Kahbazi avatar Nov 27 '23 15:11 Kahbazi

The package does not have the component owner. I think that it will be fine to extend the API. The .NET8 can take some time as we need to merge at least 2 PRs first: #1446 and #1437 to make our pipeline ready for this.

@CodeBlanch, what is your perspective on this? I think you have the biggest experience on SIG with defining new API contracts.

Kielek avatar Nov 29 '23 17:11 Kielek

I'm fine with the API addition. Feel free to open a PR @Kahbazi and I will review it.

Also to support this the OpenTelemetry.Instrumentation.StackExchangeRedis project need to add .NET 8 to its target framekwork.

Do we need net8 target or do we need Microsoft.Extensions.DependencyInjection.Abstractions v8? If it is the later that should come with the OpenTelemetry.Api.ProviderBuilderExtensions package reference once it is updated for 1.7 (>1.7.0-alpha.1 we haven't released new versions yet with the new package dependencies all bumped to 8).

CodeBlanch avatar Nov 29 '23 18:11 CodeBlanch

Do we need net8 target or do we need Microsoft.Extensions.DependencyInjection.Abstractions v8?

The latter.

eerhardt avatar Nov 30 '23 17:11 eerhardt

@Kahbazi are you still planning on helping with this? your PR went stale.

martinjt avatar Jun 02 '24 16:06 martinjt

@martinjt Sorry for the delay. Yes, I will re-open the PR.

Kahbazi avatar Jun 07 '24 13:06 Kahbazi