Support redis instrumentation for keyed service #1157
Conversation
dotnet-issue-labeler
bot
added
the
area-codeflow
| var instrumentation = Activator.CreateInstance( | ||
| typeof(StackExchangeRedisInstrumentation), | ||
| BindingFlags.NonPublic | BindingFlags.Instance, | ||
| null, | ||
| new object[] { sp.GetRequiredService<IOptionsMonitor<StackExchangeRedisInstrumentationOptions>>() }, | ||
| null) as StackExchangeRedisInstrumentation; |
| internal static class TempOpenTelemetry | ||
| { | ||
| [UnsafeAccessor(UnsafeAccessorKind.Constructor)] | ||
| static extern StackExchangeRedisInstrumentation CreateInstrumentation(IOptionsMonitor<StackExchangeRedisInstrumentationOptions> options); |
danmoseley
added
area-integrations
tests/Aspire.StackExchange.Redis.Tests/AspireRedisExtensionsTests.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (connection != null) | ||
| { | ||
| instrumentation.AddConnection(Options.DefaultName, connection); |
There was a problem hiding this comment.
| instrumentation.AddConnection(Options.DefaultName, connection); | |
| instrumentation.AddConnection(serviceKey.ToString(), connection); |
? What happens if there are more than 1 keyed Redis connections?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
joperezr
added
the
community-contribution
Temporary fixes #1045