Skip to content

Commit

Permalink
API Review Feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
martintmk committed Aug 22, 2023
1 parent 6bec2c6 commit 2c02990
Show file tree
Hide file tree
Showing 31 changed files with 60 additions and 201 deletions.
2 changes: 1 addition & 1 deletion bench/Polly.Core.Benchmarks/BridgeBenchmark.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ public class BridgeBenchmark
public void Setup()
{
_policy = Policy.NoOpAsync<string>();
_policyWrapped = ResiliencePipeline<string>.Null.AsAsyncPolicy();
_policyWrapped = ResiliencePipeline<string>.Empty.AsAsyncPolicy();
}

[Benchmark(Baseline = true)]
Expand Down
14 changes: 7 additions & 7 deletions bench/Polly.Core.Benchmarks/ResiliencePipelineBenchmark.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,48 +10,48 @@ public class ResiliencePipelineBenchmark
public async ValueTask ExecuteOutcomeAsync()
{
var context = ResilienceContextPool.Shared.Get();
await ResiliencePipeline.Null.ExecuteOutcomeAsync((_, _) => Outcome.FromResultAsTask("dummy"), context, "state").ConfigureAwait(false);
await ResiliencePipeline.Empty.ExecuteOutcomeAsync((_, _) => Outcome.FromResultAsTask("dummy"), context, "state").ConfigureAwait(false);
ResilienceContextPool.Shared.Return(context);
}

[Benchmark]
public async ValueTask ExecuteAsync_ResilienceContextAndState()
{
var context = ResilienceContextPool.Shared.Get();
await ResiliencePipeline.Null.ExecuteAsync((_, _) => new ValueTask<string>("dummy"), context, "state").ConfigureAwait(false);
await ResiliencePipeline.Empty.ExecuteAsync((_, _) => new ValueTask<string>("dummy"), context, "state").ConfigureAwait(false);
ResilienceContextPool.Shared.Return(context);
}

[Benchmark]
public async ValueTask ExecuteAsync_CancellationToken()
{
await ResiliencePipeline.Null.ExecuteAsync(_ => new ValueTask<string>("dummy"), CancellationToken.None).ConfigureAwait(false);
await ResiliencePipeline.Empty.ExecuteAsync(_ => new ValueTask<string>("dummy"), CancellationToken.None).ConfigureAwait(false);
}

[Benchmark]
public async ValueTask ExecuteAsync_GenericStrategy_CancellationToken()
{
await ResiliencePipeline<string>.Null.ExecuteAsync(_ => new ValueTask<string>("dummy"), CancellationToken.None).ConfigureAwait(false);
await ResiliencePipeline<string>.Empty.ExecuteAsync(_ => new ValueTask<string>("dummy"), CancellationToken.None).ConfigureAwait(false);
}

[Benchmark]
public void Execute_ResilienceContextAndState()
{
var context = ResilienceContextPool.Shared.Get();
ResiliencePipeline.Null.Execute((_, _) => "dummy", context, "state");
ResiliencePipeline.Empty.Execute((_, _) => "dummy", context, "state");
ResilienceContextPool.Shared.Return(context);
}

[Benchmark]
public void Execute_CancellationToken()
{
ResiliencePipeline.Null.Execute(_ => "dummy", CancellationToken.None);
ResiliencePipeline.Empty.Execute(_ => "dummy", CancellationToken.None);
}

[Benchmark]
public void Execute_GenericStrategy_CancellationToken()
{
ResiliencePipeline<string>.Null.Execute(_ => "dummy", CancellationToken.None);
ResiliencePipeline<string>.Empty.Execute(_ => "dummy", CancellationToken.None);
}

public class NonGenericStrategy
Expand Down
8 changes: 0 additions & 8 deletions src/Polly.Core/Hedging/Controller/HedgingExecutionContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,6 @@ private void UpdateOriginalContext()
if (Tasks.FirstOrDefault(static t => t.IsAccepted) is TaskExecution<T> acceptedExecution)
{
originalContext.Properties.Replace(acceptedExecution.Properties);

if (acceptedExecution.Type == HedgedTaskType.Secondary)
{
foreach (var @event in acceptedExecution.Context.ResilienceEvents)
{
originalContext.AddResilienceEvent(@event);
}
}
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/Polly.Core/Outcome.TResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,12 @@ private Outcome(ExceptionDispatchInfo exceptionDispatchInfo)
/// <remarks>
/// Returns <see langword="true"/> even if the result is void. Use <see cref="IsVoidResult"/> to check for void results.
/// </remarks>
public bool HasResult => ExceptionDispatchInfo is null;
internal bool HasResult => ExceptionDispatchInfo is null;

/// <summary>
/// Gets a value indicating whether the operation produced a void result.
/// </summary>
public bool IsVoidResult => Result is VoidResult;
internal bool IsVoidResult => Result is VoidResult;

/// <summary>
/// Throws an exception if the operation produced an exception.
Expand All @@ -64,7 +64,7 @@ private Outcome(ExceptionDispatchInfo exceptionDispatchInfo)
/// </summary>
/// <param name="result">Output parameter for the result.</param>
/// <returns><see langword="true"/> if the result is available; <see langword="false"/> otherwise.</returns>
public bool TryGetResult(out TResult? result)
internal bool TryGetResult(out TResult? result)
{
if (HasResult && !IsVoidResult)
{
Expand Down
8 changes: 2 additions & 6 deletions src/Polly.Core/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,8 @@ Polly.Outcome
Polly.Outcome<TResult>
Polly.Outcome<TResult>.EnsureSuccess() -> void
Polly.Outcome<TResult>.Exception.get -> System.Exception?
Polly.Outcome<TResult>.HasResult.get -> bool
Polly.Outcome<TResult>.IsVoidResult.get -> bool
Polly.Outcome<TResult>.Outcome() -> void
Polly.Outcome<TResult>.Result.get -> TResult?
Polly.Outcome<TResult>.TryGetResult(out TResult? result) -> bool
Polly.OutcomeArguments<TResult, TArgs>
Polly.OutcomeArguments<TResult, TArgs>.Arguments.get -> TArgs
Polly.OutcomeArguments<TResult, TArgs>.Context.get -> Polly.ResilienceContext!
Expand Down Expand Up @@ -193,7 +190,6 @@ Polly.ResilienceContext.CancellationToken.get -> System.Threading.CancellationTo
Polly.ResilienceContext.ContinueOnCapturedContext.get -> bool
Polly.ResilienceContext.OperationKey.get -> string?
Polly.ResilienceContext.Properties.get -> Polly.ResilienceProperties!
Polly.ResilienceContext.ResilienceEvents.get -> System.Collections.Generic.IReadOnlyList<Polly.Telemetry.ResilienceEvent>!
Polly.ResilienceContextCreationArguments
Polly.ResilienceContextCreationArguments.CancellationToken.get -> System.Threading.CancellationToken
Polly.ResilienceContextCreationArguments.ContinueOnCapturedContext.get -> bool?
Expand Down Expand Up @@ -406,7 +402,7 @@ static Polly.RetryResiliencePipelineBuilderExtensions.AddRetry(this Polly.Resili
static Polly.RetryResiliencePipelineBuilderExtensions.AddRetry<TResult>(this Polly.ResiliencePipelineBuilder<TResult>! builder, Polly.Retry.RetryStrategyOptions<TResult>! options) -> Polly.ResiliencePipelineBuilder<TResult>!
static Polly.TimeoutResiliencePipelineBuilderExtensions.AddTimeout<TBuilder>(this TBuilder! builder, Polly.Timeout.TimeoutStrategyOptions! options) -> TBuilder!
static Polly.TimeoutResiliencePipelineBuilderExtensions.AddTimeout<TBuilder>(this TBuilder! builder, System.TimeSpan timeout) -> TBuilder!
static readonly Polly.ResiliencePipeline.Null -> Polly.ResiliencePipeline!
static readonly Polly.ResiliencePipeline<T>.Null -> Polly.ResiliencePipeline<T>!
static readonly Polly.ResiliencePipeline.Empty -> Polly.ResiliencePipeline!
static readonly Polly.ResiliencePipeline<T>.Empty -> Polly.ResiliencePipeline<T>!
virtual Polly.Registry.ResiliencePipelineProvider<TKey>.GetPipeline(TKey key) -> Polly.ResiliencePipeline!
virtual Polly.Registry.ResiliencePipelineProvider<TKey>.GetPipeline<TResult>(TKey key) -> Polly.ResiliencePipeline<TResult>!
19 changes: 0 additions & 19 deletions src/Polly.Core/ResilienceContext.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System.Diagnostics.CodeAnalysis;
using Polly.Telemetry;

namespace Polly;

Expand All @@ -14,8 +13,6 @@ namespace Polly;
/// </remarks>
public sealed class ResilienceContext
{
private readonly List<ResilienceEvent> _resilienceEvents = new();

internal ResilienceContext()
{
}
Expand Down Expand Up @@ -66,23 +63,13 @@ internal ResilienceContext()
/// </summary>
public ResilienceProperties Properties { get; internal set; } = new();

/// <summary>
/// Gets the collection of resilience events that occurred while executing the resilience strategy.
/// </summary>
/// <remarks>
/// If the number of resilience events is greater than zero it's an indication that the execution was unhealthy.
/// </remarks>
public IReadOnlyList<ResilienceEvent> ResilienceEvents => _resilienceEvents;

internal void InitializeFrom(ResilienceContext context)
{
OperationKey = context.OperationKey;
ResultType = context.ResultType;
IsSynchronous = context.IsSynchronous;
CancellationToken = context.CancellationToken;
ContinueOnCapturedContext = context.ContinueOnCapturedContext;
_resilienceEvents.Clear();
_resilienceEvents.AddRange(context.ResilienceEvents);
}

[ExcludeFromCodeCoverage]
Expand All @@ -100,11 +87,6 @@ internal ResilienceContext Initialize<TResult>(bool isSynchronous)
return this;
}

internal void AddResilienceEvent(ResilienceEvent @event)
{
_resilienceEvents.Add(@event);
}

internal bool Reset()
{
OperationKey = null;
Expand All @@ -113,7 +95,6 @@ internal bool Reset()
ContinueOnCapturedContext = false;
CancellationToken = default;
Properties.Options.Clear();
_resilienceEvents.Clear();
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/Polly.Core/ResiliencePipeline.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public sealed partial class ResiliencePipeline
/// <summary>
/// Resilience pipeline that executes the user-provided callback without any additional logic.
/// </summary>
public static readonly ResiliencePipeline Null = new(PipelineComponent.Null, DisposeBehavior.Ignore);
public static readonly ResiliencePipeline Empty = new(PipelineComponent.Empty, DisposeBehavior.Ignore);

internal ResiliencePipeline(PipelineComponent component, DisposeBehavior disposeBehavior)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Polly.Core/ResiliencePipelineBuilderBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ internal PipelineComponent BuildPipelineComponent()

if (components.Count == 0)
{
return PipelineComponent.Null;
return PipelineComponent.Empty;
}

var source = new ResilienceTelemetrySource(Name, InstanceName, null);
Expand Down
2 changes: 1 addition & 1 deletion src/Polly.Core/ResiliencePipelineT.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public sealed partial class ResiliencePipeline<T>
/// <summary>
/// Resilience pipeline that executes the user-provided callback without any additional logic.
/// </summary>
public static readonly ResiliencePipeline<T> Null = new(PipelineComponent.Null, DisposeBehavior.Ignore);
public static readonly ResiliencePipeline<T> Empty = new(PipelineComponent.Empty, DisposeBehavior.Ignore);

internal ResiliencePipeline(PipelineComponent component, DisposeBehavior disposeBehavior)
{
Expand Down
4 changes: 0 additions & 4 deletions src/Polly.Core/Telemetry/ResilienceStrategyTelemetry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ public void Report<TArgs>(ResilienceEvent resilienceEvent, ResilienceContext con
{
Guard.NotNull(context);

context.AddResilienceEvent(resilienceEvent);

if (Listener is null || resilienceEvent.Severity == ResilienceEventSeverity.None)
{
return;
Expand All @@ -49,8 +47,6 @@ public void Report<TArgs>(ResilienceEvent resilienceEvent, ResilienceContext con
/// <param name="args">The event arguments.</param>
public void Report<TArgs, TResult>(ResilienceEvent resilienceEvent, OutcomeArguments<TResult, TArgs> args)
{
args.Context.AddResilienceEvent(resilienceEvent);

if (Listener is null || resilienceEvent.Severity == ResilienceEventSeverity.None)
{
return;
Expand Down
2 changes: 1 addition & 1 deletion src/Polly.Core/Utils/Pipeline/PipelineComponent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
/// </remarks>
internal abstract class PipelineComponent : IDisposable, IAsyncDisposable
{
public static PipelineComponent Null { get; } = new NullComponent();
public static PipelineComponent Empty { get; } = new NullComponent();

internal ResilienceStrategyOptions? Options { get; set; }

Expand Down
1 change: 0 additions & 1 deletion src/Polly.Extensions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ Dimensions:
|`pipeline-instance`| The instance name of the pipeline corresponding to the resilience pipeline.|
|`operation-key`| The operation key associated with the call site. |
|`exception-name`| The full name of the exception assigned to the execution result (`System.InvalidOperationException`). |
|`execution-health`| Indicates whether the execution was healthy or not (`Healthy`, `Unhealthy`). |

### Logs

Expand Down
2 changes: 0 additions & 2 deletions src/Polly.Extensions/Telemetry/Log.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ public static partial void PipelineExecuting(
"Source: '{PipelineName}/{PipelineInstance}', " +
"Operation Key: '{OperationKey}', " +
"Result: '{Result}', " +
"Execution Health: '{ExecutionHealth}', " +
"Execution Time: {ExecutionTime}ms",
EventName = "StrategyExecuted")]
public static partial void PipelineExecuted(
Expand All @@ -56,7 +55,6 @@ public static partial void PipelineExecuted(
string pipelineInstance,
string? operationKey,
object? result,
string executionHealth,
double executionTime,
Exception? exception);

Expand Down
19 changes: 0 additions & 19 deletions src/Polly.Extensions/Telemetry/ResilienceContextExtensions.cs

This file was deleted.

2 changes: 0 additions & 2 deletions src/Polly.Extensions/Telemetry/ResilienceTelemetryTags.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ internal class ResilienceTelemetryTags

public const string ExceptionName = "exception-name";

public const string ExecutionHealth = "execution-health";

public const string AttemptNumber = "attempt-number";

public const string AttemptHandled = "attempt-handled";
Expand Down
6 changes: 1 addition & 5 deletions src/Polly.Extensions/Telemetry/TelemetryListenerImpl.cs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ private void MeterEvent<TResult, TArgs>(in TelemetryEventArguments<TResult, TArg
var tags = TagsList.Get();
var context = new EnrichmentContext<TResult, TArgs>(in args, tags.Tags);
UpdateEnrichmentContext(in context);
tags.Tags.Add(new(ResilienceTelemetryTags.ExecutionHealth, args.Context.GetExecutionHealth()));
ExecutionDuration.Record(executionFinished.Duration.TotalMilliseconds, tags.TagsSpan);
TagsList.Return(tags);
}
Expand Down Expand Up @@ -165,15 +164,12 @@ private void LogEvent<TResult, TArgs>(in TelemetryEventArguments<TResult, TArgs>
}
else if (GetArgs<TArgs, PipelineExecutedArguments>(args.Arguments, out var pipelineExecuted))
{
var logLevel = args.Context.IsExecutionHealthy() ? LogLevel.Debug : LogLevel.Warning;

_logger.PipelineExecuted(
logLevel,
LogLevel.Debug,
args.Source.PipelineName.GetValueOrPlaceholder(),
args.Source.PipelineInstanceName.GetValueOrPlaceholder(),
args.Context.OperationKey,
GetResult(args.Context, args.Outcome),
args.Context.GetExecutionHealth(),
pipelineExecuted.Duration.TotalMilliseconds,
args.Outcome?.Exception);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using Microsoft.Extensions.Time.Testing;
using NSubstitute;
using Polly.CircuitBreaker;
using Polly.Telemetry;

namespace Polly.Core.Tests.CircuitBreaker.Controller;

Expand All @@ -11,7 +10,7 @@ public class CircuitStateControllerTests

private readonly CircuitBreakerStrategyOptions<int> _options = new();
private readonly CircuitBehavior _circuitBehavior = Substitute.For<CircuitBehavior>();
private readonly Action<TelemetryEventArguments<object, object>> _onTelemetry = _ => { };
private readonly FakeTelemetryListener _telemetryListener = new();

[Fact]
public void Ctor_EnsureDefaults()
Expand Down Expand Up @@ -60,7 +59,7 @@ public async Task IsolateAsync_Ok()
await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get());

_circuitBehavior.Received().OnCircuitClosed();
context.ResilienceEvents.Should().Contain(new ResilienceEvent(ResilienceEventSeverity.Error, "OnCircuitOpened"));
_telemetryListener.GetArgs<OnCircuitOpenedArguments>().Should().NotBeEmpty();
}

[Fact]
Expand Down Expand Up @@ -93,7 +92,7 @@ public async Task BreakAsync_Ok()

await controller.OnActionPreExecuteAsync(ResilienceContextPool.Shared.Get());
_circuitBehavior.Received().OnCircuitClosed();
context.ResilienceEvents.Should().Contain(new ResilienceEvent(ResilienceEventSeverity.Information, "OnCircuitClosed"));
_telemetryListener.GetArgs<OnCircuitClosedArguments>().Should().NotBeEmpty();
}

[Fact]
Expand Down Expand Up @@ -467,5 +466,5 @@ private async Task OpenCircuit(CircuitStateController<int> controller, Outcome<i
_options.OnHalfOpened,
_circuitBehavior,
_timeProvider,
TestUtilities.CreateResilienceTelemetry(_onTelemetry.Invoke));
TestUtilities.CreateResilienceTelemetry(_telemetryListener));
}
Original file line number Diff line number Diff line change
Expand Up @@ -320,12 +320,10 @@ public async Task Complete_EnsureOriginalContextPreparedWithAcceptedOutcome(bool
if (primary)
{
_resilienceContext.Properties.Options.Should().HaveCount(1);
_resilienceContext.ResilienceEvents.Should().HaveCount(1);
}
else
{
_resilienceContext.Properties.Options.Should().HaveCount(2);
_resilienceContext.ResilienceEvents.Should().HaveCount(4);
}
}

Expand Down Expand Up @@ -460,8 +458,6 @@ private void ConfigureSecondaryTasks(params TimeSpan[] delays)
return null;
}

args.ActionContext.AddResilienceEvent(new ResilienceEvent(ResilienceEventSeverity.Information, "dummy-event"));

return async () =>
{
args.ActionContext.Properties.Set(new ResiliencePropertyKey<int>(attempt.ToString(CultureInfo.InvariantCulture)), attempt);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -912,8 +912,8 @@ public async Task ExecuteAsync_EnsureOnHedgingTelemetry()
var strategy = Create();
await strategy.ExecuteAsync((_, _) => new ValueTask<string>(Failure), context, "state");

context.ResilienceEvents.Should().HaveCount(_options.MaxHedgedAttempts + 3);
context.ResilienceEvents.Select(v => v.EventName).Distinct().Should().HaveCount(4);
_events.Should().HaveCount(_options.MaxHedgedAttempts + 4);
_events.Select(v => v.Event.EventName).Distinct().Should().HaveCount(2);
}

private void ConfigureHedging()
Expand Down
Loading

0 comments on commit 2c02990

Please sign in to comment.