Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support redis instrumentation for keyed service #1157

Merged
merged 6 commits into from
Jan 13, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,22 @@ private static void AddRedis(IHostApplicationBuilder builder, string configurati
if (settings.Tracing)
{
// Supports distributed tracing
builder.Services.AddOpenTelemetry()
if (serviceKey is null)
{
builder.Services.AddOpenTelemetry()
.WithTracing(t =>
{
t.AddRedisInstrumentation();
});
}
else
{
builder.Services.AddOpenTelemetry()
.WithTracing(t =>
{
t.AddRedisInstrumentationWithKeyedService(serviceKey);
});
}
}

if (settings.HealthChecks)
Expand Down
62 changes: 62 additions & 0 deletions src/Components/Aspire.StackExchange.Redis/TempOpenTelemetry.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Reflection;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Options;
using OpenTelemetry.Instrumentation.StackExchangeRedis;
using OpenTelemetry.Trace;
using StackExchange.Redis;

namespace Aspire.StackExchange.Redis;

// This is a temporary solution to add instrumentation for keyed service.
// This class needs to get removed once this issue got solved and published.
// https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1451

// This code has been copied from here
//https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/c3a4f335f5a1f4fc784d44cc99b5bf44e872a66c/src/OpenTelemetry.Instrumentation.StackExchangeRedis/TracerProviderBuilderExtensions.cs#L109
internal static class TempOpenTelemetry
{
public static TracerProviderBuilder AddRedisInstrumentationWithKeyedService(
this TracerProviderBuilder builder,
object serviceKey)
{
ArgumentNullException.ThrowIfNull(builder);

builder.ConfigureServices(services =>
{
services.TryAddSingleton(sp => Create(sp));
});

return builder
.AddSource("OpenTelemetry.Instrumentation.StackExchangeRedis")
.AddInstrumentation(sp =>
{
var instrumentation = sp.GetRequiredService<StackExchangeRedisInstrumentation>();

var connection = sp.GetKeyedService<IConnectionMultiplexer>(serviceKey);

if (connection != null)
{
instrumentation.AddConnection(Options.DefaultName, connection);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
instrumentation.AddConnection(Options.DefaultName, connection);
instrumentation.AddConnection(serviceKey.ToString(), connection);

? What happens if there are more than 1 keyed Redis connections?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation for non-keyed redis connection uses Options.DefaultName I wanted to keep the implementation consistence with it.

I think the name is used to get named options for StackExchangeRedisInstrumentationOptions
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/fbfd66495359a88738bc0f5611bc7728c568ce5a/src/OpenTelemetry.Instrumentation.StackExchangeRedis/StackExchangeRedisInstrumentation.cs#L44

and for the Thread name responsible for creating redis Activity.
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/fbfd66495359a88738bc0f5611bc7728c568ce5a/src/OpenTelemetry.Instrumentation.StackExchangeRedis/StackExchangeRedisConnectionInstrumentation.cs#L56

? What happens if there are more than 1 keyed Redis connections?
The same thing that if there are more than 1 non-keyed redis connections. I don't see any problem. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name is used to get named options

This is the part I was missing.

I think this fine for now, but when we take open-telemetry/opentelemetry-dotnet-contrib#1457, we should probably be specifying the name, so someone can configure these options separately from the other Redis connections.

}

return instrumentation;
});
}

private static StackExchangeRedisInstrumentation Create(IServiceProvider sp)
{
// StackExchangeRedisInstrumentation has an internal constructor
var instrumentation = Activator.CreateInstance(
typeof(StackExchangeRedisInstrumentation),
BindingFlags.NonPublic | BindingFlags.Instance,
null,
new object[] { sp.GetRequiredService<IOptionsMonitor<StackExchangeRedisInstrumentationOptions>>() },
null) as StackExchangeRedisInstrumentation;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use UnsafeAccessor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


return instrumentation!;
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Reflection;
using Microsoft.AspNetCore.OutputCaching;
using Microsoft.Extensions.Caching.Distributed;
using Microsoft.Extensions.Caching.StackExchangeRedis;
Expand All @@ -9,6 +10,7 @@
using Microsoft.Extensions.Diagnostics.HealthChecks;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Options;
using OpenTelemetry.Trace;
using StackExchange.Redis;
using Xunit;

Expand Down Expand Up @@ -223,4 +225,25 @@ public void MultipleRedisComponentsCanBeAdded(bool useKeyed)
var healthCheckService = host.Services.GetRequiredService<HealthCheckService>();
Assert.NotNull(healthCheckService);
}

[Fact]
public void KeyedServiceRedisInstrumentation()
{
var builder = Host.CreateEmptyApplicationBuilder(null);

builder.AddKeyedRedis("redis", settings =>
{
settings.ConnectionString = "localhost";
settings.Tracing = true;
}, options => { });
Kahbazi marked this conversation as resolved.
Show resolved Hide resolved
var host = builder.Build();

//This will add the instrumentations.
var tracerProvider = host.Services.GetRequiredService<TracerProvider>();

var connectionMultiplexer = host.Services.GetRequiredKeyedService<IConnectionMultiplexer>("redis");
var profiler = connectionMultiplexer.GetType().GetField("_profilingSessionProvider", BindingFlags.NonPublic | BindingFlags.Instance)!.GetValue(connectionMultiplexer);

Assert.NotNull(profiler);
}
}