Fix Name and InstanceName not being set for reloadable pipelines (#1555)

App-vNext · Sep 6, 2023 · bc07c7c · bc07c7c
1 parent e1b49f2
commit bc07c7c
Show file tree

Hide file tree

Showing 7 changed files with 101 additions and 22 deletions.
diff --git a/src/Polly.Core/Registry/RegistryPipelineComponentBuilder.cs b/src/Polly.Core/Registry/RegistryPipelineComponentBuilder.cs
@@ -33,10 +33,6 @@ public RegistryPipelineComponentBuilder(
     internal PipelineComponent CreateComponent()
     {
         var builder = CreateBuilder();
-        var telemetry = new ResilienceStrategyTelemetry(
-            new ResilienceTelemetrySource(_builderName, _instanceName, null),
-            builder.Listener);
-
         var component = builder.ComponentFactory();
 
         if (builder.ReloadTokens.Count == 0)
@@ -45,13 +41,12 @@ internal PipelineComponent CreateComponent()
         }
 
         return PipelineComponentFactory.CreateReloadable(
-            new ReloadableComponent.Entry(component, builder.ReloadTokens),
+            new ReloadableComponent.Entry(component, builder.ReloadTokens, builder.Telemetry),
             () =>
             {
                 var builder = CreateBuilder();
-                return new ReloadableComponent.Entry(builder.ComponentFactory(), builder.ReloadTokens);
-            },
-            telemetry);
+                return new ReloadableComponent.Entry(builder.ComponentFactory(), builder.ReloadTokens, builder.Telemetry);
+            });
     }
 
     private Builder CreateBuilder()
@@ -62,13 +57,20 @@ private Builder CreateBuilder()
         builder.InstanceName = _instanceName;
         _configure(builder, context);
 
+        var telemetry = new ResilienceStrategyTelemetry(
+            new ResilienceTelemetrySource(builder.Name, builder.InstanceName, null),
+            builder.TelemetryListener);
+
         return new(
             () => PipelineComponentFactory.WithDisposableCallbacks(
                     builder.BuildPipelineComponent(),
                     context.DisposeCallbacks),
             context.ReloadTokens,
-            builder.TelemetryListener);
+            telemetry);
     }
 
-    private record Builder(Func<PipelineComponent> ComponentFactory, List<CancellationToken> ReloadTokens, TelemetryListener? Listener);
+    private record Builder(
+        Func<PipelineComponent> ComponentFactory,
+        List<CancellationToken> ReloadTokens,
+        ResilienceStrategyTelemetry Telemetry);
 }
diff --git a/src/Polly.Core/Utils/Pipeline/PipelineComponentFactory.cs b/src/Polly.Core/Utils/Pipeline/PipelineComponentFactory.cs
@@ -31,6 +31,5 @@ public static PipelineComponent CreateComposite(
 
     public static PipelineComponent CreateReloadable(
         ReloadableComponent.Entry initial,
-        Func<ReloadableComponent.Entry> factory,
-        ResilienceStrategyTelemetry telemetry) => new ReloadableComponent(initial, factory, telemetry);
+        Func<ReloadableComponent.Entry> factory) => new ReloadableComponent(initial, factory);
 }
diff --git a/src/Polly.Core/Utils/Pipeline/ReloadableComponent.cs b/src/Polly.Core/Utils/Pipeline/ReloadableComponent.cs
@@ -13,18 +13,18 @@ internal sealed class ReloadableComponent : PipelineComponent
     public const string OnReloadEvent = "OnReload";
 
     private readonly Func<Entry> _factory;
-    private readonly ResilienceStrategyTelemetry _telemetry;
+    private ResilienceStrategyTelemetry _telemetry;
     private CancellationTokenSource _tokenSource = null!;
     private CancellationTokenRegistration _registration;
     private List<CancellationToken> _reloadTokens;
 
-    public ReloadableComponent(Entry entry, Func<Entry> factory, ResilienceStrategyTelemetry telemetry)
+    public ReloadableComponent(Entry entry, Func<Entry> factory)
     {
         Component = entry.Component;
 
         _reloadTokens = entry.ReloadTokens;
         _factory = factory;
-        _telemetry = telemetry;
+        _telemetry = entry.Telemetry;
 
         TryRegisterOnReload();
     }
@@ -61,7 +61,7 @@ private void TryRegisterOnReload()
             try
             {
                 _telemetry.Report(new(ResilienceEventSeverity.Information, OnReloadEvent), context, new OnReloadArguments());
-                (Component, _reloadTokens) = _factory();
+                (Component, _reloadTokens, _telemetry) = _factory();
             }
             catch (Exception e)
             {
@@ -107,5 +107,5 @@ internal record DisposedFailedArguments(Exception Exception);
 
     internal record OnReloadArguments();
 
-    internal record Entry(PipelineComponent Component, List<CancellationToken> ReloadTokens);
+    internal record Entry(PipelineComponent Component, List<CancellationToken> ReloadTokens, ResilienceStrategyTelemetry Telemetry);
 }
diff --git a/test/Polly.Core.Tests/Utils/Pipeline/ReloadablePipelineComponentTests.cs b/test/Polly.Core.Tests/Utils/Pipeline/ReloadablePipelineComponentTests.cs
@@ -54,8 +54,9 @@ public async Task ChangeTriggered_StrategyReloaded()
     [Fact]
     public async Task ChangeTriggered_EnsureOldStrategyDisposed()
     {
+        var telemetry = TestUtilities.CreateResilienceTelemetry(_listener);
         var component = Substitute.For<PipelineComponent>();
-        await using var sut = CreateSut(component, () => new(Substitute.For<PipelineComponent>(), new List<CancellationToken>()));
+        await using var sut = CreateSut(component, () => new(Substitute.For<PipelineComponent>(), new List<CancellationToken>(), telemetry));
 
         for (var i = 0; i < 10; i++)
         {
@@ -130,12 +131,11 @@ public async Task DisposeError_EnsureReported()
 
     private ReloadableComponent CreateSut(PipelineComponent? initial = null, Func<ReloadableComponent.Entry>? factory = null)
     {
-        factory ??= () => new ReloadableComponent.Entry(PipelineComponent.Empty, new List<CancellationToken>());
+        factory ??= () => new ReloadableComponent.Entry(PipelineComponent.Empty, new List<CancellationToken>(), _telemetry);
 
         return (ReloadableComponent)PipelineComponentFactory.CreateReloadable(
-            new ReloadableComponent.Entry(initial ?? PipelineComponent.Empty, new List<CancellationToken> { _cancellationTokenSource.Token }),
-            factory,
-            _telemetry);
+            new ReloadableComponent.Entry(initial ?? PipelineComponent.Empty, new List<CancellationToken> { _cancellationTokenSource.Token }, _telemetry),
+            factory);
     }
 
     public void Dispose() => _cancellationTokenSource.Dispose();

diff --git a/test/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs b/test/Polly.Extensions.Tests/DependencyInjection/PollyServiceCollectionExtensionTests.cs
@@ -1,5 +1,6 @@
 using System.Globalization;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using Polly.DependencyInjection;
@@ -271,6 +272,55 @@ public void AddResiliencePipelineRegistry_ConfigureCallback_Ok()
         provider.GetRequiredService<IOptions<ResiliencePipelineRegistryOptions<string>>>().Value.InstanceNameFormatter.Should().Be(formatter);
     }
 
+    [InlineData(true)]
+    [InlineData(false)]
+    [Theory]
+    public void AddResiliencePipeline_CustomInstanceName_EnsureReported(bool usingBuilder)
+    {
+        // arrange
+        using var loggerFactory = new FakeLoggerFactory();
+
+        var context = ResilienceContextPool.Shared.Get("my-operation-key");
+        var services = new ServiceCollection();
+        var listener = new FakeTelemetryListener();
+        var registry = services
+            .AddResiliencePipeline("my-pipeline", ConfigureBuilder)
+            .Configure<TelemetryOptions>(options => options.TelemetryListeners.Add(listener))
+            .AddSingleton((ILoggerFactory)loggerFactory)
+            .BuildServiceProvider()
+            .GetRequiredService<ResiliencePipelineRegistry<string>>();
+
+        var pipeline = usingBuilder ?
+            registry.GetPipeline("my-pipeline") :
+            registry.GetOrAddPipeline("my-pipeline", ConfigureBuilder);
+
+        // act
+        pipeline.Execute(_ => { }, context);
+
+        // assert
+        foreach (var ev in listener.Events)
+        {
+            ev.Source.PipelineInstanceName.Should().Be("my-instance");
+            ev.Source.PipelineName.Should().Be("my-pipeline");
+        }
+
+        var record = loggerFactory.FakeLogger.GetRecords(new EventId(0, "ResilienceEvent")).First();
+
+        record.Message.Should().Contain("my-pipeline/my-instance");
+
+        static void ConfigureBuilder(ResiliencePipelineBuilder builder)
+        {
+            builder.Name.Should().Be("my-pipeline");
+            builder.InstanceName = "my-instance";
+            builder.AddRetry(new()
+            {
+                ShouldHandle = _ => PredicateResult.True(),
+                MaxRetryAttempts = 3,
+                Delay = TimeSpan.Zero
+            });
+        }
+    }
+
     private void AddResiliencePipeline(string key, Action<StrategyBuilderContext>? onBuilding = null)
     {
         _services.AddResiliencePipeline(key, builder =>

diff --git a/test/Polly.Extensions.Tests/ReloadableResiliencePipelineTests.cs b/test/Polly.Extensions.Tests/ReloadableResiliencePipelineTests.cs
@@ -3,6 +3,7 @@
 using NSubstitute;
 using Polly.DependencyInjection;
 using Polly.Registry;
+using Polly.Telemetry;
 
 namespace Polly.Extensions.Tests;
 
@@ -20,6 +21,7 @@ public void AddResiliencePipeline_EnsureReloadable(string? name)
         var reloadableConfig = new ReloadableConfiguration();
         reloadableConfig.Reload(new() { { "tag", "initial-tag" } });
         var builder = new ConfigurationBuilder().Add(reloadableConfig);
+        var fakeListener = new FakeTelemetryListener();
 
         var services = new ServiceCollection();
 
@@ -32,8 +34,11 @@ public void AddResiliencePipeline_EnsureReloadable(string? name)
             services.Configure<ReloadableStrategyOptions>(name, builder.Build());
         }
 
+        services.Configure<TelemetryOptions>(options => options.TelemetryListeners.Add(fakeListener));
         services.AddResiliencePipeline("my-pipeline", (builder, context) =>
         {
+            builder.InstanceName = "my-instance";
+
             var options = context.GetOptions<ReloadableStrategyOptions>(name);
             context.EnableReloads<ReloadableStrategyOptions>(name);
 
@@ -76,6 +81,11 @@ public void AddResiliencePipeline_EnsureReloadable(string? name)
         resList.Last().Received(1).Dispose();
         pipeline.Invoking(p => p.Execute(() => { })).Should().Throw<ObjectDisposedException>();
 
+        foreach (var ev in fakeListener.Events)
+        {
+            ev.Source.PipelineName.Should().Be("my-pipeline");
+            ev.Source.PipelineInstanceName.Should().Be("my-instance");
+        }
     }
 
     public class ReloadableStrategy : ResilienceStrategy, IDisposable

diff --git a/test/Polly.TestUtils/FakeLoggerFactory.cs b/test/Polly.TestUtils/FakeLoggerFactory.cs
@@ -0,0 +1,18 @@
+using Microsoft.Extensions.Logging;
+
+namespace Polly.TestUtils;
+
+public sealed class FakeLoggerFactory : ILoggerFactory
+{
+    public FakeLogger FakeLogger { get; } = new FakeLogger();
+
+    public void AddProvider(ILoggerProvider provider)
+    {
+    }
+
+    public ILogger CreateLogger(string categoryName) => FakeLogger;
+
+    public void Dispose()
+    {
+    }
+}