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

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

Merged
merged 7 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
22 changes: 12 additions & 10 deletions src/Polly.Core/Registry/RegistryPipelineComponentBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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()
Expand All @@ -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);
}
3 changes: 1 addition & 2 deletions src/Polly.Core/Utils/Pipeline/PipelineComponentFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
10 changes: 5 additions & 5 deletions src/Polly.Core/Utils/Pipeline/ReloadableComponent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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++)
{
Expand Down Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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 =>
Expand Down
10 changes: 10 additions & 0 deletions test/Polly.Extensions.Tests/ReloadableResiliencePipelineTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using NSubstitute;
using Polly.DependencyInjection;
using Polly.Registry;
using Polly.Telemetry;

namespace Polly.Extensions.Tests;

Expand All @@ -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();

Expand All @@ -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);

Expand Down Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions test/Polly.TestUtils/FakeLoggerFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
using Microsoft.Extensions.Logging;

namespace Polly.TestUtils;

public sealed class FakeLoggerFactory : ILoggerFactory
{
public FakeLogger FakeLogger { get; } = new FakeLogger();
martincostello marked this conversation as resolved.
Show resolved Hide resolved

public void AddProvider(ILoggerProvider provider)
{
}

public ILogger CreateLogger(string categoryName) => FakeLogger;

public void Dispose()
{
}
}