From 451ae9d65091a6b5f4d8438d49e355bfbab98f25 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Fri, 10 Mar 2023 13:33:50 -0800 Subject: [PATCH 01/22] Create IDelegatingHandlerFactory for HTTP retry This commit introduces several changes to improve the reliability and usability of the HTTP requests in the SemanticKernel. It adds a CancellationToken parameter to the CompleteAsync method of ITextCompletionClient and its implementations, allowing callers to cancel the completion request. It also adds an optional IDelegatingHandlerFactory parameter to the constructors of OpenAI client classes and memory configuration classes, enabling the injection of custom retry logic for HTTP requests. The default retry logic is provided by the DefaultHttpRetryHandlerFactory class, which uses a Polly policy to handle transient errors and respects the RetryAfter header when present. The commit also adds a new option to the kernel configuration and the kernel builder to specify a custom retry handler factory for the backends that use HTTP requests. The commit removes the unused IRetryMechanism interface and the PassThroughWithoutRetry class, and replaces them with the NullHttpRetryHandler and NullHttpRetryHandlerFactory, which do not retry on failure. The commit also makes some minor code style improvements and fixes a typo in a comment. The commit updates the documentation comments to reflect the new parameters and features. --- .../AI/ITextCompletionClient.cs | 4 +- .../Clients/AzureOpenAIClientAbstract.cs | 8 +- .../AI/OpenAI/Clients/OpenAIClientAbstract.cs | 25 +- .../AI/OpenAI/Services/AzureTextCompletion.cs | 13 +- .../AI/OpenAI/Services/AzureTextEmbeddings.cs | 7 +- .../OpenAI/Services/OpenAITextCompletion.cs | 13 +- .../OpenAI/Services/OpenAITextEmbeddings.cs | 7 +- .../Configuration/KernelConfig.cs | 97 ++++++- dotnet/src/SemanticKernel/Kernel.cs | 11 +- dotnet/src/SemanticKernel/KernelBuilder.cs | 12 + .../KernelExtensions/MemoryConfiguration.cs | 6 +- .../Orchestration/SKFunction.cs | 2 +- .../Reliability/DefaultHttpRetryHandler.cs | 256 ++++++++++++++++++ .../DefaultHttpRetryHandlerFactory.cs | 22 ++ .../Reliability/IDelegatingHandlerFactory.cs | 14 + .../Reliability/IRetryMechanism.cs | 23 -- .../Reliability/NullHttpRetryHandler.cs | 21 ++ .../Reliability/PassThroughWithoutRetry.cs | 31 --- 18 files changed, 478 insertions(+), 94 deletions(-) create mode 100644 dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs create mode 100644 dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs create mode 100644 dotnet/src/SemanticKernel/Reliability/IDelegatingHandlerFactory.cs delete mode 100644 dotnet/src/SemanticKernel/Reliability/IRetryMechanism.cs create mode 100644 dotnet/src/SemanticKernel/Reliability/NullHttpRetryHandler.cs delete mode 100644 dotnet/src/SemanticKernel/Reliability/PassThroughWithoutRetry.cs diff --git a/dotnet/src/SemanticKernel/AI/ITextCompletionClient.cs b/dotnet/src/SemanticKernel/AI/ITextCompletionClient.cs index a4118375d8db..833c6b495c6f 100644 --- a/dotnet/src/SemanticKernel/AI/ITextCompletionClient.cs +++ b/dotnet/src/SemanticKernel/AI/ITextCompletionClient.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. +using System.Threading; using System.Threading.Tasks; namespace Microsoft.SemanticKernel.AI; @@ -14,6 +15,7 @@ public interface ITextCompletionClient /// /// The prompt to complete. /// Request settings for the completion API + /// Cancellation token /// Text generated by the remote model - public Task CompleteAsync(string text, CompleteRequestSettings requestSettings); + public Task CompleteAsync(string text, CompleteRequestSettings requestSettings, CancellationToken cancellationToken = default); } diff --git a/dotnet/src/SemanticKernel/AI/OpenAI/Clients/AzureOpenAIClientAbstract.cs b/dotnet/src/SemanticKernel/AI/OpenAI/Clients/AzureOpenAIClientAbstract.cs index f3ab80244db7..3ac693ed051f 100644 --- a/dotnet/src/SemanticKernel/AI/OpenAI/Clients/AzureOpenAIClientAbstract.cs +++ b/dotnet/src/SemanticKernel/AI/OpenAI/Clients/AzureOpenAIClientAbstract.cs @@ -6,6 +6,7 @@ using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.SemanticKernel.AI.OpenAI.HttpSchema; +using Microsoft.SemanticKernel.Reliability; using Microsoft.SemanticKernel.Text; namespace Microsoft.SemanticKernel.AI.OpenAI.Clients; @@ -25,7 +26,7 @@ public abstract class AzureOpenAIClientAbstract : OpenAIClientAbstract /// protected string AzureOpenAIApiVersion { - get { return this._azureOpenAIApiVersion; } + get => this._azureOpenAIApiVersion; set { if (string.IsNullOrWhiteSpace(value)) @@ -48,7 +49,8 @@ protected string AzureOpenAIApiVersion /// Construct an AzureOpenAIClientAbstract object /// /// Logger - protected AzureOpenAIClientAbstract(ILogger? log = null) : base(log) + /// Retry handler factory + protected AzureOpenAIClientAbstract(ILogger? log = null, IDelegatingHandlerFactory? handlerFactory = null) : base(log, handlerFactory) { } @@ -161,5 +163,5 @@ protected async Task CacheDeploymentsAsync() private string _azureOpenAIApiVersion = DefaultAzureAPIVersion; - #endregion + #endregion private ================================================================================ } diff --git a/dotnet/src/SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs b/dotnet/src/SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs index 869cb9de2e41..022f35ce15f0 100644 --- a/dotnet/src/SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs +++ b/dotnet/src/SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs @@ -7,11 +7,13 @@ using System.Net.Http; using System.Net.Mime; using System.Text; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.SemanticKernel.AI.Embeddings; using Microsoft.SemanticKernel.AI.OpenAI.HttpSchema; +using Microsoft.SemanticKernel.Reliability; using Microsoft.SemanticKernel.Text; namespace Microsoft.SemanticKernel.AI.OpenAI.Clients; @@ -33,14 +35,19 @@ public abstract class OpenAIClientAbstract : IDisposable protected HttpClient HTTPClient { get; } private readonly HttpClientHandler _httpClientHandler; + private readonly IDelegatingHandlerFactory _handlerFactory = new DefaultHttpRetryHandlerFactory(); + private readonly DelegatingHandler _retryHandler; - internal OpenAIClientAbstract(ILogger? log = null) + internal OpenAIClientAbstract(ILogger? log = null, IDelegatingHandlerFactory? handlerFactory = null) { - if (log != null) { this.Log = log; } + this.Log = log ?? this.Log; + this._handlerFactory = handlerFactory ?? this._handlerFactory; - // TODO: allow injection of retry logic, e.g. Polly this._httpClientHandler = new() { CheckCertificateRevocationList = true }; - this.HTTPClient = new HttpClient(this._httpClientHandler); + this._retryHandler = this._handlerFactory.Create(this.Log); + this._retryHandler.InnerHandler = this._httpClientHandler; + + this.HTTPClient = new HttpClient(this._retryHandler); this.HTTPClient.DefaultRequestHeaders.Add("User-Agent", HTTPUseragent); } @@ -49,15 +56,16 @@ internal OpenAIClientAbstract(ILogger? log = null) /// /// URL for the completion request API /// Prompt to complete + /// Cancellation token /// The completed text /// AIException thrown during the request. - protected async Task ExecuteCompleteRequestAsync(string url, string requestBody) + protected async Task ExecuteCompleteRequestAsync(string url, string requestBody, CancellationToken cancellationToken = default) { try { this.Log.LogDebug("Sending completion request to {0}: {1}", url, requestBody); - var result = await this.ExecutePostRequestAsync(url, requestBody); + var result = await this.ExecutePostRequestAsync(url, requestBody, cancellationToken); if (result.Completions.Count < 1) { throw new AIException( @@ -124,6 +132,7 @@ protected virtual void Dispose(bool disposing) { this.HTTPClient.Dispose(); this._httpClientHandler.Dispose(); + this._retryHandler.Dispose(); } } @@ -132,14 +141,14 @@ protected virtual void Dispose(bool disposing) // HTTP user agent sent to remote endpoints private const string HTTPUseragent = "Microsoft Semantic Kernel"; - private async Task ExecutePostRequestAsync(string url, string requestBody) + private async Task ExecutePostRequestAsync(string url, string requestBody, CancellationToken cancellationToken = default) { string responseJson; try { using HttpContent content = new StringContent(requestBody, Encoding.UTF8, MediaTypeNames.Application.Json); - HttpResponseMessage response = await this.HTTPClient.PostAsync(url, content); + HttpResponseMessage response = await this.HTTPClient.PostAsync(url, content, cancellationToken); if (response == null) { diff --git a/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextCompletion.cs b/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextCompletion.cs index 38e8055a47c4..aa4cbff440d9 100644 --- a/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextCompletion.cs +++ b/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextCompletion.cs @@ -1,10 +1,12 @@ // Copyright (c) Microsoft. All rights reserved. +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.SemanticKernel.AI.OpenAI.Clients; using Microsoft.SemanticKernel.AI.OpenAI.HttpSchema; using Microsoft.SemanticKernel.Diagnostics; +using Microsoft.SemanticKernel.Reliability; using Microsoft.SemanticKernel.Text; namespace Microsoft.SemanticKernel.AI.OpenAI.Services; @@ -22,8 +24,10 @@ public sealed class AzureTextCompletion : AzureOpenAIClientAbstract, ITextComple /// Azure OpenAI API key, see https://learn.microsoft.com/azure/cognitive-services/openai/quickstart /// Azure OpenAI API version, see https://learn.microsoft.com/azure/cognitive-services/openai/reference /// Application logger - public AzureTextCompletion(string modelId, string endpoint, string apiKey, string apiVersion, ILogger? log = null) - : base(log) + /// Retry handler factory for HTTP requests. + public AzureTextCompletion(string modelId, string endpoint, string apiKey, string apiVersion, ILogger? log = null, + IDelegatingHandlerFactory? handlerFactory = null) + : base(log, handlerFactory) { Verify.NotEmpty(modelId, "The ID cannot be empty, you must provide a Model ID or a Deployment name."); this._modelId = modelId; @@ -43,9 +47,10 @@ public AzureTextCompletion(string modelId, string endpoint, string apiKey, strin /// /// Text to complete /// Request settings for the completion API + /// Cancellation token /// The completed text. /// AIException thrown during the request - public async Task CompleteAsync(string text, CompleteRequestSettings requestSettings) + public async Task CompleteAsync(string text, CompleteRequestSettings requestSettings, CancellationToken cancellationToken = default) { Verify.NotNull(requestSettings, "Completion settings cannot be empty"); @@ -72,7 +77,7 @@ public async Task CompleteAsync(string text, CompleteRequestSettings req Stop = requestSettings.StopSequences is { Count: > 0 } ? requestSettings.StopSequences : null, }); - return await this.ExecuteCompleteRequestAsync(url, requestBody); + return await this.ExecuteCompleteRequestAsync(url, requestBody, cancellationToken); } #region private ================================================================================ diff --git a/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextEmbeddings.cs b/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextEmbeddings.cs index e7c2dfac1a51..283d56d5b3db 100644 --- a/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextEmbeddings.cs +++ b/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextEmbeddings.cs @@ -7,6 +7,7 @@ using Microsoft.SemanticKernel.AI.OpenAI.Clients; using Microsoft.SemanticKernel.AI.OpenAI.HttpSchema; using Microsoft.SemanticKernel.Diagnostics; +using Microsoft.SemanticKernel.Reliability; using Microsoft.SemanticKernel.Text; namespace Microsoft.SemanticKernel.AI.OpenAI.Services; @@ -26,8 +27,10 @@ public sealed class AzureTextEmbeddings : AzureOpenAIClientAbstract, IEmbeddingG /// Azure OpenAI API key, see https://learn.microsoft.com/azure/cognitive-services/openai/quickstart /// Azure OpenAI API version, see https://learn.microsoft.com/azure/cognitive-services/openai/reference /// Application logger - public AzureTextEmbeddings(string modelId, string endpoint, string apiKey, string apiVersion, ILogger? log = null) - : base(log) + /// An optional HTTP retry handler factory + public AzureTextEmbeddings(string modelId, string endpoint, string apiKey, string apiVersion, ILogger? log = null, + IDelegatingHandlerFactory? handlerFactory = null) + : base(log, handlerFactory) { Verify.NotEmpty(modelId, "The ID cannot be empty, you must provide a Model ID or a Deployment name."); this._modelId = modelId; diff --git a/dotnet/src/SemanticKernel/AI/OpenAI/Services/OpenAITextCompletion.cs b/dotnet/src/SemanticKernel/AI/OpenAI/Services/OpenAITextCompletion.cs index 00f6553f7ef2..95ec72da38da 100644 --- a/dotnet/src/SemanticKernel/AI/OpenAI/Services/OpenAITextCompletion.cs +++ b/dotnet/src/SemanticKernel/AI/OpenAI/Services/OpenAITextCompletion.cs @@ -1,11 +1,13 @@ // Copyright (c) Microsoft. All rights reserved. using System.Net.Http.Headers; +using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.SemanticKernel.AI.OpenAI.Clients; using Microsoft.SemanticKernel.AI.OpenAI.HttpSchema; using Microsoft.SemanticKernel.Diagnostics; +using Microsoft.SemanticKernel.Reliability; using Microsoft.SemanticKernel.Text; namespace Microsoft.SemanticKernel.AI.OpenAI.Services; @@ -27,8 +29,10 @@ public sealed class OpenAITextCompletion : OpenAIClientAbstract, ITextCompletion /// OpenAI API key, see https://platform.openai.com/account/api-keys /// OpenAI organization id. This is usually optional unless your account belongs to multiple organizations. /// Logger - public OpenAITextCompletion(string modelId, string apiKey, string? organization = null, ILogger? log = null) : - base(log) + /// Retry handler + public OpenAITextCompletion(string modelId, string apiKey, string? organization = null, ILogger? log = null, + IDelegatingHandlerFactory? handlerFactory = null) : + base(log, handlerFactory) { Verify.NotEmpty(modelId, "The OpenAI model ID cannot be empty"); this._modelId = modelId; @@ -47,9 +51,10 @@ public OpenAITextCompletion(string modelId, string apiKey, string? organization /// /// The prompt to complete. /// Request settings for the completion API + /// Cancellation token /// The completed text /// AIException thrown during the request - public async Task CompleteAsync(string text, CompleteRequestSettings requestSettings) + public async Task CompleteAsync(string text, CompleteRequestSettings requestSettings, CancellationToken cancellationToken = default) { Verify.NotNull(requestSettings, "Completion settings cannot be empty"); @@ -74,6 +79,6 @@ public async Task CompleteAsync(string text, CompleteRequestSettings req Stop = requestSettings.StopSequences is { Count: > 0 } ? requestSettings.StopSequences : null, }); - return await this.ExecuteCompleteRequestAsync(url, requestBody); + return await this.ExecuteCompleteRequestAsync(url, requestBody, cancellationToken); } } diff --git a/dotnet/src/SemanticKernel/AI/OpenAI/Services/OpenAITextEmbeddings.cs b/dotnet/src/SemanticKernel/AI/OpenAI/Services/OpenAITextEmbeddings.cs index 6506f4710620..039f0d937c57 100644 --- a/dotnet/src/SemanticKernel/AI/OpenAI/Services/OpenAITextEmbeddings.cs +++ b/dotnet/src/SemanticKernel/AI/OpenAI/Services/OpenAITextEmbeddings.cs @@ -8,6 +8,7 @@ using Microsoft.SemanticKernel.AI.OpenAI.Clients; using Microsoft.SemanticKernel.AI.OpenAI.HttpSchema; using Microsoft.SemanticKernel.Diagnostics; +using Microsoft.SemanticKernel.Reliability; using Microsoft.SemanticKernel.Text; namespace Microsoft.SemanticKernel.AI.OpenAI.Services; @@ -30,8 +31,10 @@ public sealed class OpenAITextEmbeddings : OpenAIClientAbstract, IEmbeddingGener /// OpenAI API Key /// Optional OpenAI organization ID, usually required only if your account belongs to multiple organizations /// Application logger - public OpenAITextEmbeddings(string modelId, string apiKey, string? organization = null, ILogger? log = null) - : base(log) + /// Retry handler factory for HTTP requests. + public OpenAITextEmbeddings(string modelId, string apiKey, string? organization = null, ILogger? log = null, + IDelegatingHandlerFactory? handlerFactory = null) + : base(log, handlerFactory) { Verify.NotEmpty(modelId, "The OpenAI model ID cannot be empty"); this._modelId = modelId; diff --git a/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs b/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs index ef8cf8cff877..132b74de66c5 100644 --- a/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs +++ b/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs @@ -3,6 +3,9 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net; +using System.Net.Http; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.SemanticKernel.AI.OpenAI.Services; using Microsoft.SemanticKernel.Diagnostics; using Microsoft.SemanticKernel.Reliability; @@ -15,9 +18,76 @@ namespace Microsoft.SemanticKernel.Configuration; public sealed class KernelConfig { /// - /// Global retry logic used for all the backends + /// Retry configuration for IHttpRetryPolicy that uses RetryAfter header when present. /// - public IRetryMechanism RetryMechanism { get => this._retryMechanism; } + public sealed class HttpRetryConfig + { + /// + /// Maximum number of retries. + /// + /// Thrown when value is negative. + public int MaxRetryCount + { + get { return this._maxRetryCount; } + set + { + if (value < 0) + { + throw new ArgumentOutOfRangeException(nameof(this.MaxRetryCount), "Max retry count cannot be negative."); + } + + this._maxRetryCount = value; + } + } + + /// + /// Minimum delay between retries. + /// + public TimeSpan MinRetryDelay { get; set; } = TimeSpan.FromSeconds(2); + + /// + /// Maximum delay between retries. + /// + public TimeSpan MaxRetryDelay { get; set; } = TimeSpan.FromSeconds(60); + + /// + /// Maximum total time spent retrying. + /// + public TimeSpan MaxTotalRetryTime { get; set; } = TimeSpan.FromMinutes(2); + + /// + /// Whether to use exponential backoff or not. + /// + public bool UseExponentialBackoff { get; set; } + + /// + /// List of status codes that should be retried. + /// + public List RetryableStatusCodes { get; set; } = new() + { + HttpStatusCode.RequestTimeout, + HttpStatusCode.ServiceUnavailable, + HttpStatusCode.GatewayTimeout, + HttpStatusCode.TooManyRequests + }; + + /// + /// List of exception types that should be retried. + /// + public List RetryableExceptionTypes { get; set; } = new() + { + typeof(HttpRequestException) + }; + + private int _maxRetryCount = 1; + } + + /// + /// Global retry logic used for all the backends http calls + /// + public IDelegatingHandlerFactory HttpHandlerFactory { get; private set; } = new DefaultHttpRetryHandlerFactory(new HttpRetryConfig()); + + public HttpRetryConfig DefaultHttpRetryConfig { get; private set; } = new(); /// /// Adds an Azure OpenAI backend to the list. @@ -180,13 +250,27 @@ public bool HasEmbeddingsBackend(string label, Func? condi } /// - /// Set the retry mechanism to use for the kernel. + /// Set the http retry handler factory to use for the kernel. /// - /// Retry mechanism to use. + /// Http retry handler factory to use. /// The updated kernel configuration. - public KernelConfig SetRetryMechanism(IRetryMechanism? retryMechanism = null) + public KernelConfig SetHttpRetryHandlerFactory(IDelegatingHandlerFactory? httpHandlerFactory = null) + { + if (httpHandlerFactory != null) + { + this.HttpHandlerFactory = httpHandlerFactory; + } + + return this; + } + + public KernelConfig SetDefaultHttpRetryConfig(HttpRetryConfig? httpRetryConfig) { - this._retryMechanism = retryMechanism ?? new PassThroughWithoutRetry(); + if (httpRetryConfig != null) + { + this.DefaultHttpRetryConfig = httpRetryConfig; + } + return this; } @@ -397,7 +481,6 @@ public KernelConfig RemoveAllBackends() private Dictionary EmbeddingsBackends { get; set; } = new(); private string? _defaultCompletionBackend; private string? _defaultEmbeddingsBackend; - private IRetryMechanism _retryMechanism = new PassThroughWithoutRetry(); #endregion } diff --git a/dotnet/src/SemanticKernel/Kernel.cs b/dotnet/src/SemanticKernel/Kernel.cs index a9903586cb5f..4cec17592ea7 100644 --- a/dotnet/src/SemanticKernel/Kernel.cs +++ b/dotnet/src/SemanticKernel/Kernel.cs @@ -172,10 +172,7 @@ public async Task RunAsync(ContextVariables variables, CancellationTo try { cancellationToken.ThrowIfCancellationRequested(); - await this._config.RetryMechanism.ExecuteWithRetryAsync( - async () => { context = await f.InvokeAsync(context); }, - this._log, - cancellationToken); + context = await f.InvokeAsync(context); if (context.ErrorOccurred) { @@ -266,7 +263,8 @@ private ISKFunction CreateSemanticFunction( azureBackendConfig.Endpoint, azureBackendConfig.APIKey, azureBackendConfig.APIVersion, - this._log)); + this._log, + this._config.HttpHandlerFactory)); break; case OpenAIConfig openAiConfig: @@ -274,7 +272,8 @@ private ISKFunction CreateSemanticFunction( openAiConfig.ModelId, openAiConfig.APIKey, openAiConfig.OrgId, - this._log)); + this._log, + this._config.HttpHandlerFactory)); break; default: diff --git a/dotnet/src/SemanticKernel/KernelBuilder.cs b/dotnet/src/SemanticKernel/KernelBuilder.cs index d039eb10e671..6c5a79fd6c33 100644 --- a/dotnet/src/SemanticKernel/KernelBuilder.cs +++ b/dotnet/src/SemanticKernel/KernelBuilder.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. using System; +using System.Net.Http; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.SemanticKernel.AI.Embeddings; @@ -8,6 +9,7 @@ using Microsoft.SemanticKernel.Diagnostics; using Microsoft.SemanticKernel.KernelExtensions; using Microsoft.SemanticKernel.Memory; +using Microsoft.SemanticKernel.Reliability; using Microsoft.SemanticKernel.SkillDefinition; using Microsoft.SemanticKernel.TemplateEngine; @@ -23,6 +25,7 @@ public sealed class KernelBuilder private ISemanticTextMemory _memory = NullMemory.Instance; private ILogger _log = NullLogger.Instance; private IMemoryStore? _memoryStorage = null; + private IDelegatingHandlerFactory? _httpHandlerFactory = null; /// /// Create a new kernel instance @@ -40,6 +43,8 @@ public static IKernel Create() /// Kernel instance public IKernel Build() { + this._config.SetHttpRetryHandlerFactory(this._httpHandlerFactory ?? new DefaultHttpRetryHandlerFactory(this._config.DefaultHttpRetryConfig)); + var instance = new Kernel( new SkillCollection(this._log), new PromptTemplateEngine(this._log), @@ -108,6 +113,13 @@ public KernelBuilder WithMemoryStorageAndEmbeddingGenerator( return this; } + public KernelBuilder WithRetryHandlerFactory(IDelegatingHandlerFactory httpHandlerFactory) + { + Verify.NotNull(httpHandlerFactory, "The retry handler factory instance provided is NULL"); + this._httpHandlerFactory = httpHandlerFactory; + return this; + } + /// /// Use the given configuration with the kernel to be built. /// diff --git a/dotnet/src/SemanticKernel/KernelExtensions/MemoryConfiguration.cs b/dotnet/src/SemanticKernel/KernelExtensions/MemoryConfiguration.cs index eb54f57f5056..76430561e22c 100644 --- a/dotnet/src/SemanticKernel/KernelExtensions/MemoryConfiguration.cs +++ b/dotnet/src/SemanticKernel/KernelExtensions/MemoryConfiguration.cs @@ -51,7 +51,8 @@ public static void UseMemory(this IKernel kernel, string? embeddingsBackendLabel azureAIConfig.Endpoint, azureAIConfig.APIKey, azureAIConfig.APIVersion, - kernel.Log); + kernel.Log, + kernel.Config.HttpHandlerFactory); break; case OpenAIConfig openAIConfig: @@ -59,7 +60,8 @@ public static void UseMemory(this IKernel kernel, string? embeddingsBackendLabel openAIConfig.ModelId, openAIConfig.APIKey, openAIConfig.OrgId, - kernel.Log); + kernel.Log, + kernel.Config.HttpHandlerFactory); break; default: diff --git a/dotnet/src/SemanticKernel/Orchestration/SKFunction.cs b/dotnet/src/SemanticKernel/Orchestration/SKFunction.cs index e18a5496dbc7..466bbf9e4996 100644 --- a/dotnet/src/SemanticKernel/Orchestration/SKFunction.cs +++ b/dotnet/src/SemanticKernel/Orchestration/SKFunction.cs @@ -109,7 +109,7 @@ async Task LocalFunc( { string prompt = await functionConfig.PromptTemplate.RenderAsync(context); - string completion = await client.CompleteAsync(prompt, requestSettings); + string completion = await client.CompleteAsync(prompt, requestSettings, context.CancellationToken); context.Variables.Update(completion); } catch (Exception ex) when (!ex.IsCriticalException()) diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs new file mode 100644 index 000000000000..c0bf8bedb885 --- /dev/null +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -0,0 +1,256 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.IO; +using System.Net; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.SemanticKernel.Configuration; + +namespace Microsoft.SemanticKernel.Reliability; + +internal sealed class DefaultHttpRetryHandler : DelegatingHandler +{ + /// + /// Initializes a new instance of the class. + /// + /// The retry configuration. + /// The logger. + public DefaultHttpRetryHandler(KernelConfig.HttpRetryConfig? config = null, ILogger? log = null) : this(config ?? new KernelConfig.HttpRetryConfig(), log, + null, null) + { + } + + public readonly Guid Id = Guid.NewGuid(); + + internal DefaultHttpRetryHandler(KernelConfig.HttpRetryConfig config, ILogger? log = null, IDelayProvider? delayProvider = null, + ITimeProvider? timeProvider = null) + { + this._config = config; + this._log = log ?? NullLogger.Instance; + this._delayProvider = delayProvider ?? new TaskDelayProvider(); + this._timeProvider = timeProvider ?? new DefaultTimeProvider(); + } + + /// + /// Executes the action with retry logic + /// + /// + /// The request is retried if it throws an exception that is a retryable exception. + /// If the request throws an exception that is not a retryable exception, it is not retried. + /// If the request returns a response with a retryable error code, it is retried. + /// If the request returns a response with a non-retryable error code, it is not retried. + /// If the exception contains a RetryAfter header, the request is retried after the specified delay. + /// If configured to use exponential backoff, the delay is doubled for each retry. + /// + /// The request. + /// The cancellation token. + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + int retryCount = 0; + + var start = this._timeProvider.GetCurrentTime(); + while (true) + { + cancellationToken.ThrowIfCancellationRequested(); + + TimeSpan waitFor = default; + string reason = string.Empty; + HttpResponseMessage? response = null; + try + { + response = await base.SendAsync(request, cancellationToken); + + // If the request does not require a retry then we're done + if (!this.ShouldRetry(response.StatusCode)) + { + return response; + } + + // Drain response content to free connections. Need to perform this + // before retry attempt and before the TooManyRetries ServiceException. + if (response.Content != null) + { +#if NET5_0_OR_GREATER + await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false); +#else + await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false); +#endif + } + + reason = response.StatusCode.ToString(); + + if (retryCount >= this._config.MaxRetryCount) + { + this._log.LogError( + "Error executing request, max retry count reached. Reason: {0}", reason); + return response; + } + + // If the retry delay is longer than the total timeout, then we'll + // just return + if (!this.HasTimeForRetry(start, retryCount, response, out waitFor)) + { + this._log.LogError( + "Error executing request, max total retry time reached. Reason: {0}", reason); + return response; + } + } + catch (Exception e) when ((this.ShouldRetry(e) || this.ShouldRetry(e.InnerException)) && + retryCount < this._config.MaxRetryCount && + this.HasTimeForRetry(start, retryCount, response, out waitFor)) + { + reason = e.GetType().ToString(); + } + + // If the request requires a retry then we'll retry + this._log.LogWarning( + "Error executing action [attempt {0} of {1}]. Reason: {2}. Will retry after {3}ms", + retryCount + 1, + this._config.MaxRetryCount, + reason, + waitFor.TotalMilliseconds); + + // Clone request with CloneAsync before retrying + // Do not dispose this request as that breaks the request cloning +#pragma warning disable CA2000 + request = await CloneAsync(request); +#pragma warning restore CA2000 + + // Increase retryCount + retryCount++; + + // Delay + await this._delayProvider.DelayAsync(waitFor, cancellationToken).ConfigureAwait(false); + } + } + + private TimeSpan GetWaitTime(int retryCount, HttpResponseMessage? response) + { + var retryAfter = response?.Headers.RetryAfter?.Date.HasValue == true + ? response?.Headers.RetryAfter?.Date - DateTimeOffset.Now + : (response?.Headers.RetryAfter?.Delta) ?? this._config.MinRetryDelay; + retryAfter ??= this._config.MinRetryDelay; + + var timeToWait = retryAfter > this._config.MaxRetryDelay + ? this._config.MaxRetryDelay + : retryAfter < this._config.MinRetryDelay + ? this._config.MinRetryDelay + : retryAfter ?? default; + + if (this._config.UseExponentialBackoff) + { + for (var backoffRetryCount = 1; backoffRetryCount < retryCount + 1; backoffRetryCount++) + { + timeToWait = timeToWait.Add(timeToWait); + } + } + + return timeToWait; + } + + private bool HasTimeForRetry(DateTimeOffset start, int retryCount, HttpResponseMessage? response, out TimeSpan waitFor) + { + waitFor = this.GetWaitTime(retryCount, response); + var currentTIme = this._timeProvider.GetCurrentTime(); + var result = currentTIme - start + waitFor; + + return result < this._config.MaxTotalRetryTime; + } + + private bool ShouldRetry(HttpStatusCode statusCode) + { + return this._config.RetryableStatusCodes.Contains(statusCode); + } + + private bool ShouldRetry(Exception exception) + { + return this._config.RetryableExceptionTypes.Contains(exception.GetType()); + } + + /// + /// Create a new HTTP request by copying previous HTTP request's headers and properties from response's request message. + /// Copied from: https://github.com/microsoftgraph/msgraph-sdk-dotnet-core/blob/dev/src/Microsoft.Graph.Core/Extensions/HttpRequestMessageExtensions.cs + /// + /// The previous needs to be copy. + /// The . + /// + /// Re-issue a new HTTP request with the previous request's headers and properties + /// + internal static async Task CloneAsync(HttpRequestMessage originalRequest) + { + var newRequest = new HttpRequestMessage(originalRequest.Method, originalRequest.RequestUri); + + // Copy request headers. + foreach (var header in originalRequest.Headers) + { + newRequest.Headers.TryAddWithoutValidation(header.Key, header.Value); + } + + // Copy request properties. + foreach (var property in originalRequest.Properties) + { + newRequest.Properties.Add(property); + } + + // Set Content if previous request had one. + if (originalRequest.Content != null) + { + // HttpClient doesn't rewind streams and we have to explicitly do so. + await originalRequest.Content.ReadAsStreamAsync().ContinueWith(t => + { + if (t.Result.CanSeek) + { + t.Result.Seek(0, SeekOrigin.Begin); + } + + newRequest.Content = new StreamContent(t.Result); + }, TaskScheduler.Current).ConfigureAwait(false); + + // Copy content headers. + if (originalRequest.Content.Headers != null) + { + foreach (var contentHeader in originalRequest.Content.Headers) + { + newRequest.Content.Headers.TryAddWithoutValidation(contentHeader.Key, contentHeader.Value); + } + } + } + + return newRequest; + } + + private readonly KernelConfig.HttpRetryConfig _config; + private readonly ILogger _log; + private readonly IDelayProvider _delayProvider; + private readonly ITimeProvider _timeProvider; + + internal interface IDelayProvider + { + Task DelayAsync(TimeSpan delay, CancellationToken cancellationToken); + } + + internal class TaskDelayProvider : IDelayProvider + { + public async Task DelayAsync(TimeSpan delay, CancellationToken cancellationToken) + { + await Task.Delay(delay, cancellationToken); + } + } + + internal interface ITimeProvider + { + DateTimeOffset GetCurrentTime(); + } + + internal class DefaultTimeProvider : ITimeProvider + { + public DateTimeOffset GetCurrentTime() + { + return DateTimeOffset.UtcNow; + } + } +} diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs new file mode 100644 index 000000000000..5651bed728cd --- /dev/null +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Net.Http; +using Microsoft.Extensions.Logging; +using Microsoft.SemanticKernel.Configuration; + +namespace Microsoft.SemanticKernel.Reliability; + +internal class DefaultHttpRetryHandlerFactory : IDelegatingHandlerFactory +{ + internal DefaultHttpRetryHandlerFactory(KernelConfig.HttpRetryConfig? config = null) + { + this._config = config; + } + + public DelegatingHandler Create(ILogger log) + { + return new DefaultHttpRetryHandler(this._config, log); + } + + private readonly KernelConfig.HttpRetryConfig? _config; +} diff --git a/dotnet/src/SemanticKernel/Reliability/IDelegatingHandlerFactory.cs b/dotnet/src/SemanticKernel/Reliability/IDelegatingHandlerFactory.cs new file mode 100644 index 000000000000..1d4d4d29f8ae --- /dev/null +++ b/dotnet/src/SemanticKernel/Reliability/IDelegatingHandlerFactory.cs @@ -0,0 +1,14 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Net.Http; +using Microsoft.Extensions.Logging; + +namespace Microsoft.SemanticKernel.Reliability; + +/// +/// Factory for creating instances. +/// +public interface IDelegatingHandlerFactory +{ + DelegatingHandler Create(ILogger log); +} diff --git a/dotnet/src/SemanticKernel/Reliability/IRetryMechanism.cs b/dotnet/src/SemanticKernel/Reliability/IRetryMechanism.cs deleted file mode 100644 index cd92fc8e278c..000000000000 --- a/dotnet/src/SemanticKernel/Reliability/IRetryMechanism.cs +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -using System; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging; - -namespace Microsoft.SemanticKernel.Reliability; - -/// -/// Interface for retry mechanisms on AI calls. -/// -public interface IRetryMechanism -{ - /// - /// Executes the given action with retry logic. - /// - /// The action to retry on exception. - /// The logger to use. - /// The cancellation token. - /// An awaitable task. - Task ExecuteWithRetryAsync(Func action, ILogger log, CancellationToken cancellationToken = default); -} diff --git a/dotnet/src/SemanticKernel/Reliability/NullHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/NullHttpRetryHandler.cs new file mode 100644 index 000000000000..8749ace9ef51 --- /dev/null +++ b/dotnet/src/SemanticKernel/Reliability/NullHttpRetryHandler.cs @@ -0,0 +1,21 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Net.Http; +using Microsoft.Extensions.Logging; + +namespace Microsoft.SemanticKernel.Reliability; + +public class NullHttpRetryHandlerFactory : IDelegatingHandlerFactory +{ + public DelegatingHandler Create(ILogger log) + { + return new NullHttpRetryHandler(); + } +} + +/// +/// A http retry handler that does not retry. +/// +public class NullHttpRetryHandler : DelegatingHandler +{ +} diff --git a/dotnet/src/SemanticKernel/Reliability/PassThroughWithoutRetry.cs b/dotnet/src/SemanticKernel/Reliability/PassThroughWithoutRetry.cs deleted file mode 100644 index 8680722dae04..000000000000 --- a/dotnet/src/SemanticKernel/Reliability/PassThroughWithoutRetry.cs +++ /dev/null @@ -1,31 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -using System; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging; -using Microsoft.SemanticKernel.Diagnostics; - -namespace Microsoft.SemanticKernel.Reliability; - -/// -/// A retry mechanism that does not retry. -/// -internal class PassThroughWithoutRetry : IRetryMechanism -{ - public async Task ExecuteWithRetryAsync(Func action, ILogger log, CancellationToken cancellationToken = default) - { - try - { - if (!cancellationToken.IsCancellationRequested) - { - await action(); - } - } - catch (Exception ex) when (!ex.IsCriticalException()) - { - log.LogWarning(ex, "Error executing action, not retrying"); - throw; - } - } -} From fd335685ad7938d5e1294eee960d60ef73bd0434 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Fri, 10 Mar 2023 13:35:27 -0800 Subject: [PATCH 02/22] Update tests for Retry This commit adds and improves unit tests for the classes that handle HTTP retry logic for OpenAI requests. It also renames and removes some deprecated classes and adds a dependency on Moq, a testing framework that allows for mocking interfaces. The main changes are: - Rename PassThroughWithoutRetry to NullHttpRetryHandler and remove PassThroughWithoutRetryTests - Add unit tests for NullHttpRetryHandler and DefaultHttpRetryHandler, which implement different retry policies based on status codes, exceptions, and backoff strategies - Add the DynamicProxyGenAssembly2 assembly attribute to SemanticKernel.csproj, which is required for using Moq - Add a new feature to the kernel configuration that allows setting a custom HTTP retry policy for OpenAI requests - Add an integration test that verifies the retry behavior when using an invalid OpenAI API key - Refactor the existing kernel configuration tests and the RedirectOutput class to improve readability and test coverag --- .../AI/OpenAICompletionTests.cs | 30 + .../RedirectOutput.cs | 29 +- .../Configuration/KernelConfigTests.cs | 90 ++- .../DefaultHttpRetryHandlerTests.cs | 634 ++++++++++++++++++ .../Reliability/NullHttpRetryHandlerTests.cs | 107 +++ .../PassThroughWithoutRetryTests.cs | 75 --- .../src/SemanticKernel/SemanticKernel.csproj | 3 + 7 files changed, 879 insertions(+), 89 deletions(-) create mode 100644 dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs create mode 100644 dotnet/src/SemanticKernel.UnitTests/Reliability/NullHttpRetryHandlerTests.cs delete mode 100644 dotnet/src/SemanticKernel.UnitTests/Reliability/PassThroughWithoutRetryTests.cs diff --git a/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs b/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs index 524602496a85..58273c9ad92d 100644 --- a/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs +++ b/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs @@ -7,6 +7,7 @@ using System.Threading.Tasks; using Microsoft.Extensions.Configuration; using Microsoft.SemanticKernel; +using Microsoft.SemanticKernel.Configuration; using Microsoft.SemanticKernel.KernelExtensions; using Microsoft.SemanticKernel.Orchestration; using SemanticKernel.IntegrationTests.TestSettings; @@ -91,6 +92,35 @@ public async Task AzureOpenAITestAsync(string prompt, string expectedAnswerConta Assert.Contains(expectedAnswerContains, actual.Result, StringComparison.InvariantCultureIgnoreCase); } + [Theory] + [InlineData("Where is the most famous fish market in Seattle, Washington, USA?", + "Error executing action [attempt 1 of 1]. Reason: Unauthorized. Will retry after 2000ms")] + public async Task OpenAIHttpRetryPolicyTestAsync(string prompt, string expectedOutput) + { + // Arrange + var retryConfig = new KernelConfig.HttpRetryConfig(); + retryConfig.RetryableStatusCodes.Add(System.Net.HttpStatusCode.Unauthorized); + IKernel target = Kernel.Builder.WithLogger(this._testOutputHelper).Configure(c => c.SetDefaultHttpRetryConfig(retryConfig)).Build(); + + OpenAIConfiguration? openAIConfiguration = this._configuration.GetSection("OpenAI").Get(); + Assert.NotNull(openAIConfiguration); + + target.Config.AddOpenAICompletionBackend( + label: openAIConfiguration.Label, + modelId: openAIConfiguration.ModelId, + apiKey: "INVALID_KEY"); + + target.Config.SetDefaultCompletionBackend(openAIConfiguration.Label); + + IDictionary skill = GetSkill("SummarizeSkill", target); + + // Act + await target.RunAsync(prompt, skill["Summarize"]); + + // Assert + Assert.Contains(expectedOutput, this._testOutputHelper.GetLogs(), StringComparison.InvariantCultureIgnoreCase); + } + private static IDictionary GetSkill(string skillName, IKernel target) { string? currentAssemblyDirectory = Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location); diff --git a/dotnet/src/SemanticKernel.IntegrationTests/RedirectOutput.cs b/dotnet/src/SemanticKernel.IntegrationTests/RedirectOutput.cs index 4805d94dcf71..5aa902e6c9a2 100644 --- a/dotnet/src/SemanticKernel.IntegrationTests/RedirectOutput.cs +++ b/dotnet/src/SemanticKernel.IntegrationTests/RedirectOutput.cs @@ -1,18 +1,22 @@ // Copyright (c) Microsoft. All rights reserved. +using System; using System.IO; using System.Text; +using Microsoft.Extensions.Logging; using Xunit.Abstractions; namespace SemanticKernel.IntegrationTests; -public class RedirectOutput : TextWriter +public class RedirectOutput : TextWriter, ILogger { private readonly ITestOutputHelper _output; + private readonly StringBuilder _logs; public RedirectOutput(ITestOutputHelper output) { this._output = output; + this._logs = new StringBuilder(); } public override Encoding Encoding { get; } = Encoding.UTF8; @@ -20,5 +24,28 @@ public RedirectOutput(ITestOutputHelper output) public override void WriteLine(string? value) { this._output.WriteLine(value); + this._logs.AppendLine(value); + } + + public IDisposable? BeginScope(TState state) where TState : notnull + { + return null; + } + + public bool IsEnabled(LogLevel logLevel) + { + return true; + } + + public string GetLogs() + { + return this._logs.ToString(); + } + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + var message = formatter(state, exception); + this._output?.WriteLine(message); + this._logs.AppendLine(message); } } diff --git a/dotnet/src/SemanticKernel.UnitTests/Configuration/KernelConfigTests.cs b/dotnet/src/SemanticKernel.UnitTests/Configuration/KernelConfigTests.cs index 43d94bfbc2cb..c1b45623f980 100644 --- a/dotnet/src/SemanticKernel.UnitTests/Configuration/KernelConfigTests.cs +++ b/dotnet/src/SemanticKernel.UnitTests/Configuration/KernelConfigTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. using System.Linq; +using System.Threading.Tasks; using Microsoft.SemanticKernel; using Microsoft.SemanticKernel.AI.OpenAI.Services; using Microsoft.SemanticKernel.Configuration; @@ -16,55 +17,118 @@ namespace SemanticKernel.UnitTests.Configuration; public class KernelConfigTests { [Fact] - public void RetryMechanismIsSet() + public void HttpRetryHandlerFactoryIsSet() { // Arrange - var retry = new PassThroughWithoutRetry(); + var retry = new NullHttpRetryHandlerFactory(); var config = new KernelConfig(); // Act - config.SetRetryMechanism(retry); + config.SetHttpRetryHandlerFactory(retry); // Assert - Assert.Equal(retry, config.RetryMechanism); + Assert.Equal(retry, config.HttpHandlerFactory); } [Fact] - public void RetryMechanismIsSetWithCustomImplementation() + public void HttpRetryHandlerFactoryIsSetWithCustomImplementation() { // Arrange - var retry = new Mock(); + var retry = new Mock(); var config = new KernelConfig(); // Act - config.SetRetryMechanism(retry.Object); + config.SetHttpRetryHandlerFactory(retry.Object); // Assert - Assert.Equal(retry.Object, config.RetryMechanism); + Assert.Equal(retry.Object, config.HttpHandlerFactory); } [Fact] - public void RetryMechanismIsSetToPassThroughWithoutRetryIfNull() + public void HttpRetryHandlerFactoryIsSetToDefaultHttpRetryHandlerFactoryIfNull() { // Arrange var config = new KernelConfig(); // Act - config.SetRetryMechanism(null); + config.SetHttpRetryHandlerFactory(null); // Assert - Assert.IsType(config.RetryMechanism); + Assert.IsType(config.HttpHandlerFactory); } [Fact] - public void RetryMechanismIsSetToPassThroughWithoutRetryIfNotSet() + public void HttpRetryHandlerFactoryIsSetToDefaultHttpRetryHandlerFactoryIfNotSet() { // Arrange var config = new KernelConfig(); // Act // Assert - Assert.IsType(config.RetryMechanism); + Assert.IsType(config.HttpHandlerFactory); + } + + [Fact] + public async Task NegativeMaxRetryCountThrowsAsync() + { + // Act + await Assert.ThrowsAsync(() => + { + var httpRetryConfig = new KernelConfig.HttpRetryConfig() { MaxRetryCount = -1 }; + return Task.CompletedTask; + }); + } + + [Fact] + public void SetDefaultHttpRetryConfig() + { + // Arrange + var config = new KernelConfig(); + var httpRetryConfig = new KernelConfig.HttpRetryConfig() { MaxRetryCount = 1 }; + + // Act + config.SetDefaultHttpRetryConfig(httpRetryConfig); + + // Assert + Assert.Equal(httpRetryConfig, config.DefaultHttpRetryConfig); + } + + [Fact] + public void SetDefaultHttpRetryConfigToDefaultIfNotSet() + { + // Arrange + var config = new KernelConfig(); + + // Act + // Assert + var defaultConfig = new KernelConfig.HttpRetryConfig(); + Assert.Equal(defaultConfig.MaxRetryCount, config.DefaultHttpRetryConfig.MaxRetryCount); + Assert.Equal(defaultConfig.MaxRetryDelay, config.DefaultHttpRetryConfig.MaxRetryDelay); + Assert.Equal(defaultConfig.MinRetryDelay, config.DefaultHttpRetryConfig.MinRetryDelay); + Assert.Equal(defaultConfig.MaxTotalRetryTime, config.DefaultHttpRetryConfig.MaxTotalRetryTime); + Assert.Equal(defaultConfig.UseExponentialBackoff, config.DefaultHttpRetryConfig.UseExponentialBackoff); + Assert.Equal(defaultConfig.RetryableStatusCodes, config.DefaultHttpRetryConfig.RetryableStatusCodes); + Assert.Equal(defaultConfig.RetryableExceptionTypes, config.DefaultHttpRetryConfig.RetryableExceptionTypes); + } + + [Fact] + public void SetDefaultHttpRetryConfigToDefaultIfNull() + { + // Arrange + var config = new KernelConfig(); + + // Act + config.SetDefaultHttpRetryConfig(null); + + // Assert + var defaultConfig = new KernelConfig.HttpRetryConfig(); + Assert.Equal(defaultConfig.MaxRetryCount, config.DefaultHttpRetryConfig.MaxRetryCount); + Assert.Equal(defaultConfig.MaxRetryDelay, config.DefaultHttpRetryConfig.MaxRetryDelay); + Assert.Equal(defaultConfig.MinRetryDelay, config.DefaultHttpRetryConfig.MinRetryDelay); + Assert.Equal(defaultConfig.MaxTotalRetryTime, config.DefaultHttpRetryConfig.MaxTotalRetryTime); + Assert.Equal(defaultConfig.UseExponentialBackoff, config.DefaultHttpRetryConfig.UseExponentialBackoff); + Assert.Equal(defaultConfig.RetryableStatusCodes, config.DefaultHttpRetryConfig.RetryableStatusCodes); + Assert.Equal(defaultConfig.RetryableExceptionTypes, config.DefaultHttpRetryConfig.RetryableExceptionTypes); } [Fact] diff --git a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs new file mode 100644 index 000000000000..1be47fc70140 --- /dev/null +++ b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs @@ -0,0 +1,634 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Net.Http.Headers; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.SemanticKernel.Reliability; +using Moq; +using Moq.Protected; +using Xunit; +using static Microsoft.SemanticKernel.Configuration.KernelConfig; + +namespace SemanticKernel.UnitTests.Reliability; + +public class DefaultHttpRetryHandlerTests +{ + [Theory] + [InlineData(HttpStatusCode.RequestTimeout)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + [InlineData(HttpStatusCode.TooManyRequests)] + public async Task NoMaxRetryCountCallsOnceForStatusAsync(HttpStatusCode statusCode) + { + // Arrange + using var retry = new DefaultHttpRetryHandler(new HttpRetryConfig() { MaxRetryCount = 0 }, Mock.Of()); + using var mockResponse = new HttpResponseMessage(statusCode); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Once(), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(statusCode, response.StatusCode); + } + + [Theory] + [InlineData(HttpStatusCode.RequestTimeout)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + [InlineData(HttpStatusCode.TooManyRequests)] + public async Task ItRetriesOnceOnRetryableStatusAsync(HttpStatusCode statusCode) + { + // Arrange + using var retry = ConfigureRetryHandler(); + using var mockResponse = new HttpResponseMessage(statusCode); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(statusCode, response.StatusCode); + } + + [Theory] + [InlineData(typeof(HttpRequestException))] + public async Task ItRetriesOnceOnRetryableExceptionAsync(Type exceptionType) + { + // Arrange + using var retry = ConfigureRetryHandler(); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(exceptionType); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await Assert.ThrowsAsync(exceptionType, + async () => await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + } + + [Theory] + [InlineData(typeof(HttpRequestException))] + public async Task NoMaxRetryCountCallsOnceForExceptionAsync(Type exceptionType) + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig() { MaxRetryCount = 0 }); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(exceptionType); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await Assert.ThrowsAsync(exceptionType, + async () => await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Once(), ItExpr.IsAny(), ItExpr.IsAny()); + } + + [Theory] + [InlineData(HttpStatusCode.RequestTimeout)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + [InlineData(HttpStatusCode.TooManyRequests)] + public async Task ItRetriesOnceOnTransientStatusWithExponentialBackoffAsync(HttpStatusCode statusCode) + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig() { UseExponentialBackoff = true }); + using var mockResponse = new HttpResponseMessage(statusCode); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(statusCode, response.StatusCode); + } + + [Theory] + [InlineData(typeof(HttpRequestException))] + public async Task ItRetriesOnceOnRetryableExceptionWithExponentialBackoffAsync(Type exceptionType) + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig() { UseExponentialBackoff = true }); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(exceptionType); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await Assert.ThrowsAsync(exceptionType, + async () => await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + } + + [Theory] + [InlineData(HttpStatusCode.RequestTimeout)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + public async Task ItRetriesExponentiallyWithExponentialBackoffAsync(HttpStatusCode statusCode) + { + // Arrange + var currentTime = DateTimeOffset.UtcNow; + var mockTimeProvider = new Mock(); + var mockDelayProvider = new Mock(); + mockTimeProvider.SetupSequence(x => x.GetCurrentTime()) + .Returns(() => currentTime) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(5)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(510)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(1015)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(1520)); + using var retry = ConfigureRetryHandler(new HttpRetryConfig() + { + UseExponentialBackoff = true, MaxRetryCount = 3, + MinRetryDelay = TimeSpan.FromMilliseconds(500) + }, mockTimeProvider, mockDelayProvider); + using var mockResponse = new HttpResponseMessage(statusCode); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(4), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(statusCode, response.StatusCode); + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(4)); + mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(500), It.IsAny()), Times.Once); + mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(1000), It.IsAny()), Times.Once); + mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(2000), It.IsAny()), Times.Once); + } + + [Theory] + [InlineData(HttpStatusCode.RequestTimeout)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + public async Task ItRetriesOnceOnTransientStatusCodeWithRetryValueAsync(HttpStatusCode statusCode) + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig(), null); + using var mockResponse = new HttpResponseMessage() + { + StatusCode = statusCode, + Headers = { RetryAfter = new RetryConditionHeaderValue(new TimeSpan(0, 0, 0, 1)) }, + }; + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + using var testContent = new StringContent("test"); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(statusCode, response.StatusCode); + Assert.Equal(new TimeSpan(0, 0, 0, 1), response.Headers.RetryAfter?.Delta); + } + + [Theory] + [InlineData(HttpStatusCode.RequestTimeout)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + public async Task ItRetriesStatusCustomCountAsync(HttpStatusCode expectedStatus) + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig() { MaxRetryCount = 3 }, null); + using var mockResponse = new HttpResponseMessage(expectedStatus); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(4), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(expectedStatus, response.StatusCode); + } + + [Theory] + [InlineData(typeof(HttpRequestException))] + public async Task ItRetriesExceptionsCustomCountAsync(Type expectedException) + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig() { MaxRetryCount = 3 }, null); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(expectedException); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await Assert.ThrowsAsync(expectedException, + async () => await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(4), ItExpr.IsAny(), ItExpr.IsAny()); + } + + [Fact] + public async Task NoExceptionNoRetryAsync() + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig() { MaxRetryCount = 3 }, null); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.OK); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(1), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + [Fact] + public async Task ItDoesNotExecuteOnCancellationTokenAsync() + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig() { MaxRetryCount = 3 }, null); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.OK); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + var cancellationToken = new CancellationToken(true); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await Assert.ThrowsAsync(async () => + await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, cancellationToken)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Never(), ItExpr.IsAny(), ItExpr.IsAny()); + } + + [Fact] + public async Task ItDoestExecuteOnFalseCancellationTokenAsync() + { + // Arrange + using var retry = ConfigureRetryHandler(new HttpRetryConfig() { MaxRetryCount = 3 }, null); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.OK); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + var cancellationToken = new CancellationToken(false); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, cancellationToken); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(1), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + [Fact] + public async Task ItRetriesWithMinRetryDelayAsync() + { + var HttpRetryConfig = new HttpRetryConfig + { + MinRetryDelay = TimeSpan.FromMilliseconds(500) + }; + + var mockDelayProvider = new Mock(); + var mockTimeProvider = new Mock(); + + var currentTime = DateTimeOffset.UtcNow; + + mockTimeProvider.SetupSequence(x => x.GetCurrentTime()) + .Returns(() => currentTime) + .Returns(() => currentTime.AddMilliseconds(5)) + .Returns(() => currentTime.AddMilliseconds(510)); + + mockDelayProvider.Setup(x => x.DelayAsync(It.IsAny(), It.IsAny())) + .Returns(() => Task.CompletedTask); + + using var retry = ConfigureRetryHandler(HttpRetryConfig, mockTimeProvider, mockDelayProvider); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.TooManyRequests); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(2)); + mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(500), It.IsAny()), Times.Once); + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.TooManyRequests, response.StatusCode); + } + + [Fact] + public async Task ItRetriesWithMaxRetryDelayAsync() + { + var HttpRetryConfig = new HttpRetryConfig + { + MinRetryDelay = TimeSpan.FromMilliseconds(1), + MaxRetryDelay = TimeSpan.FromMilliseconds(500) + }; + + var mockDelayProvider = new Mock(); + var mockTimeProvider = new Mock(); + + var currentTime = DateTimeOffset.UtcNow; + + mockTimeProvider.SetupSequence(x => x.GetCurrentTime()) + .Returns(() => currentTime) + .Returns(() => currentTime.AddMilliseconds(5)) + .Returns(() => currentTime.AddMilliseconds(505)); + + mockDelayProvider.Setup(x => x.DelayAsync(It.IsAny(), It.IsAny())) + .Returns(() => Task.CompletedTask); + + using var retry = ConfigureRetryHandler(HttpRetryConfig, mockTimeProvider, mockDelayProvider); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.TooManyRequests) + { Headers = { RetryAfter = new RetryConditionHeaderValue(TimeSpan.FromMilliseconds(2000)) } }; + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(2)); + mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(500), It.IsAny()), Times.Once); + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.TooManyRequests, response.StatusCode); + Assert.Equal(TimeSpan.FromMilliseconds(2000), response.Headers.RetryAfter?.Delta); + } + + [Theory] + [InlineData(HttpStatusCode.TooManyRequests)] + [InlineData(HttpStatusCode.ServiceUnavailable)] + [InlineData(HttpStatusCode.GatewayTimeout)] + [InlineData(HttpStatusCode.RequestTimeout)] + public async Task ItRetriesWithMaxTotalDelayAsync(HttpStatusCode statusCode) + { + // Arrange + var HttpRetryConfig = new HttpRetryConfig + { + MaxRetryCount = 5, + MinRetryDelay = TimeSpan.FromMilliseconds(50), + MaxRetryDelay = TimeSpan.FromMilliseconds(50), + MaxTotalRetryTime = TimeSpan.FromMilliseconds(350) + }; + + var mockDelayProvider = new Mock(); + var mockTimeProvider = new Mock(); + + var currentTime = DateTimeOffset.UtcNow; + mockTimeProvider.SetupSequence(x => x.GetCurrentTime()) + .Returns(() => currentTime) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(5)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(55)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(110)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(165)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(220)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(275)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(330)); + + using var retry = ConfigureRetryHandler(HttpRetryConfig, mockTimeProvider, mockDelayProvider); + + using var mockResponse = new HttpResponseMessage(statusCode); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(6)); // one for the initial call, and one for each of 5 attempts + mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(50), It.IsAny()), Times.Exactly(5)); + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(6), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(statusCode, response.StatusCode); + } + + [Fact] + public async Task ItRetriesFewerWithMaxTotalDelayAsync() + { + // Arrange + var HttpRetryConfig = new HttpRetryConfig + { + MaxRetryCount = 5, + MinRetryDelay = TimeSpan.FromMilliseconds(50), + MaxRetryDelay = TimeSpan.FromMilliseconds(50), + MaxTotalRetryTime = TimeSpan.FromMilliseconds(100) + }; + + var mockDelayProvider = new Mock(); + var mockTimeProvider = new Mock(); + + var currentTime = DateTimeOffset.UtcNow; + mockTimeProvider.SetupSequence(x => x.GetCurrentTime()) + .Returns(() => currentTime) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(5)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(55)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(110)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(165)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(220)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(275)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(330)); + + using var retry = ConfigureRetryHandler(HttpRetryConfig, mockTimeProvider, mockDelayProvider); + + using var mockResponse = new HttpResponseMessage(HttpStatusCode.TooManyRequests); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(3)); // one for the initial call, and one for each of 5 attempts + mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(50), It.IsAny()), Times.Exactly(1)); + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.TooManyRequests, response.StatusCode); + } + + [Fact] + public async Task ItRetriesOnRetryableStatusCodesAsync() + { + // Arrange + var config = new HttpRetryConfig() { RetryableStatusCodes = new List { HttpStatusCode.Unauthorized } }; + using var retry = ConfigureRetryHandler(config); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.Unauthorized); + + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + } + + [Fact] + public async Task ItDoesNotRetryOnNonRetryableStatusCodesAsync() + { + // Arrange + var config = new HttpRetryConfig() { RetryableStatusCodes = new List { HttpStatusCode.Unauthorized } }; + using var retry = ConfigureRetryHandler(config); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.TooManyRequests); + + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Once(), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.TooManyRequests, response.StatusCode); + } + + [Fact] + public async Task ItRetriesOnRetryableExceptionsAsync() + { + // Arrange + var config = new HttpRetryConfig() { RetryableExceptionTypes = new List { typeof(InvalidOperationException) } }; + using var retry = ConfigureRetryHandler(config); + + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(typeof(InvalidOperationException)); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + await Assert.ThrowsAsync(async () => + await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + } + + [Fact] + public async Task ItDoesNotRetryOnNonRetryableExceptionsAsync() + { + // Arrange + var config = new HttpRetryConfig() { RetryableExceptionTypes = new List { typeof(InvalidOperationException) } }; + using var retry = ConfigureRetryHandler(config); + + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(typeof(ArgumentException)); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + await Assert.ThrowsAsync(async () => + await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Once(), ItExpr.IsAny(), ItExpr.IsAny()); + } + + private static DefaultHttpRetryHandler ConfigureRetryHandler(HttpRetryConfig? config = null, + Mock? timeProvider = null, Mock? delayProvider = null) + { + delayProvider ??= new Mock(); + timeProvider ??= new Mock(); + var retry = new DefaultHttpRetryHandler(config ?? new HttpRetryConfig(), Mock.Of(), delayProvider.Object, timeProvider.Object); + return retry; + } + + private static Mock GetHttpMessageHandlerMock(HttpResponseMessage mockResponse) + { + var mockHandler = new Mock(); + mockHandler.Protected() + .Setup>("SendAsync", ItExpr.IsAny(), ItExpr.IsAny()) + .ReturnsAsync(mockResponse); + return mockHandler; + } + + private static Mock GetHttpMessageHandlerMock(Type exceptionType) + { + var mockHandler = new Mock(); + mockHandler.Protected() + .Setup>("SendAsync", ItExpr.IsAny(), ItExpr.IsAny()) + .ThrowsAsync(Activator.CreateInstance(exceptionType) as Exception); + return mockHandler; + } +} diff --git a/dotnet/src/SemanticKernel.UnitTests/Reliability/NullHttpRetryHandlerTests.cs b/dotnet/src/SemanticKernel.UnitTests/Reliability/NullHttpRetryHandlerTests.cs new file mode 100644 index 000000000000..c6f6fee99501 --- /dev/null +++ b/dotnet/src/SemanticKernel.UnitTests/Reliability/NullHttpRetryHandlerTests.cs @@ -0,0 +1,107 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Net; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.SemanticKernel.Reliability; +using Moq; +using Moq.Protected; +using Xunit; + +namespace SemanticKernel.UnitTests.Reliability; + +public class NullHttpRetryHandlerTests +{ + [Fact] + public async Task ItDoesNotRetryOnExceptionAsync() + { + // Arrange + using var retry = new NullHttpRetryHandler(); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.TooManyRequests); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Once(), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.TooManyRequests, response.StatusCode); + } + + [Fact] + public async Task NoExceptionNoRetryAsync() + { + // Arrange + using var retry = new NullHttpRetryHandler(); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.OK); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Once(), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + [Fact] + public async Task TaskCanceledExceptionThrownOnCancellationTokenAsync() + { + // Arrange + using var retry = new NullHttpRetryHandler(); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.TooManyRequests); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + using var cancellationTokenSource = new CancellationTokenSource(); + cancellationTokenSource.Cancel(); + + // Act + await Assert.ThrowsAsync(async () => + await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, cancellationTokenSource.Token)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Once(), ItExpr.IsAny(), ItExpr.IsAny()); + } + + [Fact] + public async Task ItDoestExecuteOnFalseCancellationTokenAsync() + { + // Arrange + using var retry = new NullHttpRetryHandler(); + using var mockResponse = new HttpResponseMessage(HttpStatusCode.TooManyRequests); + using var testContent = new StringContent("test"); + var mockHandler = GetHttpMessageHandlerMock(mockResponse); + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, new CancellationToken(false)); + + // Assert + mockHandler.Protected() + .Verify>("SendAsync", Times.Once(), ItExpr.IsAny(), ItExpr.IsAny()); + Assert.Equal(HttpStatusCode.TooManyRequests, response.StatusCode); + } + + private static Mock GetHttpMessageHandlerMock(HttpResponseMessage mockResponse) + { + var mockHandler = new Mock(); + mockHandler.Protected() + .Setup>("SendAsync", ItExpr.IsAny(), ItExpr.IsAny()) + .ReturnsAsync(mockResponse); + return mockHandler; + } +} diff --git a/dotnet/src/SemanticKernel.UnitTests/Reliability/PassThroughWithoutRetryTests.cs b/dotnet/src/SemanticKernel.UnitTests/Reliability/PassThroughWithoutRetryTests.cs deleted file mode 100644 index 098fe632f754..000000000000 --- a/dotnet/src/SemanticKernel.UnitTests/Reliability/PassThroughWithoutRetryTests.cs +++ /dev/null @@ -1,75 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -using System; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.Extensions.Logging; -using Microsoft.SemanticKernel.AI; -using Microsoft.SemanticKernel.Reliability; -using Moq; -using Xunit; - -namespace SemanticKernel.UnitTests.Reliability; - -public class PassThroughWithoutRetryTests -{ - [Fact] - public async Task ItDoesNotRetryOnExceptionAsync() - { - // Arrange - var retry = new PassThroughWithoutRetry(); - var action = new Mock>(); - action.Setup(a => a()).Throws(new AIException(AIException.ErrorCodes.Throttling, "Throttling Test")); - - // Act - await Assert.ThrowsAsync(() => retry.ExecuteWithRetryAsync(action.Object, Mock.Of())); - - // Assert - action.Verify(a => a(), Times.Once); - } - - [Fact] - public async Task NoExceptionNoRetryAsync() - { - // Arrange - var log = new Mock(); - var retry = new PassThroughWithoutRetry(); - var action = new Mock>(); - - // Act - await retry.ExecuteWithRetryAsync(action.Object, log.Object); - - // Assert - action.Verify(a => a(), Times.Once); - } - - [Fact] - public async Task ItDoesNotExecuteOnCancellationTokenAsync() - { - // Arrange - var retry = new PassThroughWithoutRetry(); - var action = new Mock>(); - action.Setup(a => a()).Throws(new AIException(AIException.ErrorCodes.Throttling, "Throttling Test")); - - // Act - await retry.ExecuteWithRetryAsync(action.Object, Mock.Of(), new CancellationToken(true)); - - // Assert - action.Verify(a => a(), Times.Never); - } - - [Fact] - public async Task ItDoestExecuteOnFalseCancellationTokenAsync() - { - // Arrange - var retry = new PassThroughWithoutRetry(); - var action = new Mock>(); - action.Setup(a => a()).Throws(new AIException(AIException.ErrorCodes.Throttling, "Throttling Test")); - - // Act - await Assert.ThrowsAsync(() => retry.ExecuteWithRetryAsync(action.Object, Mock.Of(), new CancellationToken(false))); - - // Assert - action.Verify(a => a(), Times.Once); - } -} diff --git a/dotnet/src/SemanticKernel/SemanticKernel.csproj b/dotnet/src/SemanticKernel/SemanticKernel.csproj index 6d8cc6ba880c..0ae7415b28a3 100644 --- a/dotnet/src/SemanticKernel/SemanticKernel.csproj +++ b/dotnet/src/SemanticKernel/SemanticKernel.csproj @@ -39,5 +39,8 @@ <_Parameter1>SemanticKernel.UnitTests + + <_Parameter1>DynamicProxyGenAssembly2 + \ No newline at end of file From 0edec68eb6c66b709e3ddb7d7d6e6b49019cee3d Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Fri, 10 Mar 2023 13:40:50 -0800 Subject: [PATCH 03/22] Refactor kernel retry mechanism, add new handler, and adjust logging levels Summary: This commit introduces several changes to improve the kernel's performance and usability when using the OpenAI completion backend: - It refactors the retry mechanism for kernel requests by using a delegating handler factory instead of a custom interface. This allows for more flexibility and consistency in the retry logic, and simplifies the configuration of semantic skills. - It adds a new retry handler that uses the RetryAfter value from the response headers to determine the backoff duration, which can improve the efficiency and reliability of the requests. - It adds a new file Example08_RetryHandler.cs that demonstrates how to use different retry policies for semantic skills, and removes some unused or redundant code from other examples. - It changes the logging levels for the RepoUtils project, which provides utilities for working with GitHub repositories. It comments out the line that sets the minimum logging level to Warning, and adds a filter to only log Warning messages from the System namespace, to reduce noise from irrelevant messages. --- samples/dotnet/KernelBuilder/Program.cs | 30 ++- .../Example08_RetryHandler.cs | 175 ++++++++++++++++++ .../Example08_RetryMechanism.cs | 38 ---- .../Example12_Planning.cs | 8 +- .../Example13_ConversationSummarySkill.cs | 4 +- .../dotnet/kernel-syntax-examples/Program.cs | 2 +- .../Reliability/RetryThreeTimesWithBackoff.cs | 43 ++++- .../RetryThreeTimesWithRetryAfterBackoff.cs | 69 +++++++ .../RepoUtils/ConsoleLogger.cs | 6 +- 9 files changed, 311 insertions(+), 64 deletions(-) create mode 100644 samples/dotnet/kernel-syntax-examples/Example08_RetryHandler.cs delete mode 100644 samples/dotnet/kernel-syntax-examples/Example08_RetryMechanism.cs create mode 100644 samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithRetryAfterBackoff.cs diff --git a/samples/dotnet/KernelBuilder/Program.cs b/samples/dotnet/KernelBuilder/Program.cs index c542a2dfe811..2fef62390d24 100644 --- a/samples/dotnet/KernelBuilder/Program.cs +++ b/samples/dotnet/KernelBuilder/Program.cs @@ -104,15 +104,33 @@ // AI requests (when using the kernel). var kernel8 = Kernel.Builder - .Configure(c => c.SetRetryMechanism(new RetryThreeTimes())) + .Configure(c => c.SetHttpRetryHandlerFactory(new RetryThreeTimesFactory())) .Build(); -public class RetryThreeTimes : IRetryMechanism +public class RetryThreeTimesFactory : IDelegatingHandlerFactory { - public Task ExecuteWithRetryAsync(Func action, ILogger log, CancellationToken cancellationToken = default) + public DelegatingHandler Create(ILogger log) { - var policy = GetPolicy(log); - return policy.ExecuteAsync((_) => action(), cancellationToken); + return new RetryThreeTimes(log); + } +} + +public class RetryThreeTimes : DelegatingHandler +{ + private readonly AsyncRetryPolicy _policy; + + public RetryThreeTimes(ILogger log = null) + { + this._policy = GetPolicy(log ?? NullLogger.Instance); + } + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + return await this._policy.ExecuteAsync(async () => + { + var response = await base.SendAsync(request, cancellationToken); + return response; + }); } private static AsyncRetryPolicy GetPolicy(ILogger log) @@ -126,7 +144,7 @@ private static AsyncRetryPolicy GetPolicy(ILogger log) TimeSpan.FromSeconds(8) }, (ex, timespan, retryCount, _) => log.LogWarning(ex, - "Error executing action [attempt {0} of ], pausing {1} msecs", + "Error executing action [attempt {0} of 3], pausing {1}ms", retryCount, timespan.TotalMilliseconds)); } } diff --git a/samples/dotnet/kernel-syntax-examples/Example08_RetryHandler.cs b/samples/dotnet/kernel-syntax-examples/Example08_RetryHandler.cs new file mode 100644 index 000000000000..44902ae62133 --- /dev/null +++ b/samples/dotnet/kernel-syntax-examples/Example08_RetryHandler.cs @@ -0,0 +1,175 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.SemanticKernel; +using Microsoft.SemanticKernel.Configuration; +using Microsoft.SemanticKernel.CoreSkills; +using Microsoft.SemanticKernel.KernelExtensions; +using Microsoft.SemanticKernel.Reliability; +using Reliability; +using RepoUtils; + +// ReSharper disable once InconsistentNaming +public static class Example08_RetryHandler +{ + public static async Task RunAsync() + { + var kernel = InitializeKernel(); + var retryHandlerFactory = new RetryThreeTimesWithBackoffFactory(); + ConsoleLogger.Log.LogInformation("============================== RetryThreeTimesWithBackoff =============================="); + await RunRetryPolicyAsync(kernel, retryHandlerFactory); + + ConsoleLogger.Log.LogInformation("========================= RetryThreeTimesWithRetryAfterBackoff ========================="); + await RunRetryPolicyBuilderAsync(typeof(RetryThreeTimesWithRetryAfterBackoffFactory)); + + ConsoleLogger.Log.LogInformation("==================================== NoRetryPolicy ====================================="); + await RunRetryPolicyBuilderAsync(typeof(NullHttpRetryHandlerFactory)); + + ConsoleLogger.Log.LogInformation("=============================== DefaultHttpRetryHandler ================================"); + await RunRetryHandlerConfigAsync(new KernelConfig.HttpRetryConfig() { MaxRetryCount = 3, UseExponentialBackoff = true }); + + ConsoleLogger.Log.LogInformation("======= DefaultHttpRetryConfig [MaxRetryCount = 3, UseExponentialBackoff = true] ======="); + await RunRetryHandlerConfigAsync(new KernelConfig.HttpRetryConfig() { MaxRetryCount = 3, UseExponentialBackoff = true }); + } + + private static async Task RunRetryHandlerConfigAsync(KernelConfig.HttpRetryConfig? config = null) + { + var kernelBuilder = Kernel.Builder.WithLogger(ConsoleLogger.Log); + if (config != null) + { + kernelBuilder = kernelBuilder.Configure(c => c.SetDefaultHttpRetryConfig(config)); + } + + // Add 401 to the list of retryable status codes + // Typically 401 would not be something we retry but for demonstration + // purposes we are doing so as it's easy to trigger when using an invalid key. + kernelBuilder = kernelBuilder.Configure(c => c.DefaultHttpRetryConfig.RetryableStatusCodes.Add(System.Net.HttpStatusCode.Unauthorized)); + + // OpenAI settings - you can set the OPENAI_API_KEY to an invalid value to see the retry policy in play + kernelBuilder = kernelBuilder.Configure(c => c.AddOpenAICompletionBackend("text-davinci-003", "text-davinci-003", "BAD_KEY")); + + var kernel = kernelBuilder.Build(); + + await ImportAndExecuteSkillAsync(kernel); + } + + private static IKernel InitializeKernel() + { + var kernel = Kernel.Builder.WithLogger(ConsoleLogger.Log).Build(); + // OpenAI settings - you can set the OPENAI_API_KEY to an invalid value to see the retry policy in play + kernel.Config.AddOpenAICompletionBackend("text-davinci-003", "text-davinci-003", "BAD_KEY"); + + return kernel; + } + + private static async Task RunRetryPolicyAsync(IKernel kernel, IDelegatingHandlerFactory retryHandlerFactory) + { + kernel.Config.SetHttpRetryHandlerFactory(retryHandlerFactory); + await ImportAndExecuteSkillAsync(kernel); + } + + private static async Task RunRetryPolicyBuilderAsync(Type retryHandlerFactoryType) + { + var kernelBuilder = Kernel.Builder.WithLogger(ConsoleLogger.Log) + .WithRetryHandlerFactory((Activator.CreateInstance(retryHandlerFactoryType) as IDelegatingHandlerFactory)!); + + // OpenAI settings - you can set the OPENAI_API_KEY to an invalid value to see the retry policy in play + kernelBuilder = kernelBuilder.Configure(c => c.AddOpenAICompletionBackend("text-davinci-003", "text-davinci-003", "BAD_KEY")); + + var kernel = kernelBuilder.Build(); + + await ImportAndExecuteSkillAsync(kernel); + } + + private static async Task ImportAndExecuteSkillAsync(IKernel kernel) + { + // Load semantic skill defined with prompt templates + string folder = RepoFiles.SampleSkillsPath(); + + kernel.ImportSkill(new TimeSkill(), "time"); + + var qaSkill = kernel.ImportSemanticSkillFromDirectory( + folder, + "QASkill"); + + var question = "How popular is Polly library?"; + + ConsoleLogger.Log.LogInformation("Question: {0}", question); + // To see the retry policy in play, you can set the OPENAI_API_KEY to an invalid value + var answer = await kernel.RunAsync(question, qaSkill["Question"]); + ConsoleLogger.Log.LogInformation("Answer: {0}", answer); + } +} + +/* Output: +info: object[0] + ============================== RetryThreeTimesWithBackoff ============================== +info: object[0] + Question: How popular is Polly library? +warn: object[0] + Error executing action [attempt 1 of 3], pausing 2000ms. Outcome: Unauthorized +warn: object[0] + Error executing action [attempt 2 of 3], pausing 4000ms. Outcome: Unauthorized +warn: object[0] + Error executing action [attempt 3 of 3], pausing 8000ms. Outcome: Unauthorized +fail: object[0] + Function call fail during pipeline step 0: QASkill.Question +info: object[0] + Answer: Error: AccessDenied: The request is not authorized, HTTP status: Unauthorized +info: object[0] + ========================= RetryThreeTimesWithRetryAfterBackoff ========================= +info: object[0] + Question: How popular is Polly library? +warn: object[0] + Error executing action [attempt 1 of 3], pausing 2000ms. Outcome: Unauthorized +warn: object[0] + Error executing action [attempt 2 of 3], pausing 2000ms. Outcome: Unauthorized +warn: object[0] + Error executing action [attempt 3 of 3], pausing 2000ms. Outcome: Unauthorized +fail: object[0] + Function call fail during pipeline step 0: QASkill.Question +info: object[0] + Answer: Error: AccessDenied: The request is not authorized, HTTP status: Unauthorized +info: object[0] + ==================================== NoRetryPolicy ===================================== +info: object[0] + Question: How popular is Polly library? +fail: object[0] + Function call fail during pipeline step 0: QASkill.Question +info: object[0] + Answer: Error: AccessDenied: The request is not authorized, HTTP status: Unauthorized +info: object[0] + =============================== DefaultHttpRetryHandler ================================ +info: object[0] + Question: How popular is Polly library? +warn: object[0] + Error executing action [attempt 1 of 3]. Reason: Unauthorized. Will retry after 2000ms +warn: object[0] + Error executing action [attempt 2 of 3]. Reason: Unauthorized. Will retry after 4000ms +warn: object[0] + Error executing action [attempt 3 of 3]. Reason: Unauthorized. Will retry after 8000ms +fail: object[0] + Error executing request, max retry count reached. Reason: Unauthorized +fail: object[0] + Function call fail during pipeline step 0: QASkill.Question +info: object[0] + Answer: Error: AccessDenied: The request is not authorized, HTTP status: Unauthorized +info: object[0] + ======= DefaultHttpRetryConfig [MaxRetryCount = 3, UseExponentialBackoff = true] ======= +info: object[0] + Question: How popular is Polly library? +warn: object[0] + Error executing action [attempt 1 of 3]. Reason: Unauthorized. Will retry after 2000ms +warn: object[0] + Error executing action [attempt 2 of 3]. Reason: Unauthorized. Will retry after 4000ms +warn: object[0] + Error executing action [attempt 3 of 3]. Reason: Unauthorized. Will retry after 8000ms +fail: object[0] + Error executing request, max retry count reached. Reason: Unauthorized +fail: object[0] + Function call fail during pipeline step 0: QASkill.Question +info: object[0] + Answer: Error: AccessDenied: The request is not authorized, HTTP status: Unauthorized +*/ diff --git a/samples/dotnet/kernel-syntax-examples/Example08_RetryMechanism.cs b/samples/dotnet/kernel-syntax-examples/Example08_RetryMechanism.cs deleted file mode 100644 index 115d87b62e6a..000000000000 --- a/samples/dotnet/kernel-syntax-examples/Example08_RetryMechanism.cs +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. - -using System; -using System.Threading.Tasks; -using Microsoft.SemanticKernel; -using Microsoft.SemanticKernel.CoreSkills; -using Microsoft.SemanticKernel.KernelExtensions; -using Reliability; -using RepoUtils; - -// ReSharper disable once InconsistentNaming -public static class Example08_RetryMechanism -{ - public static async Task RunAsync() - { - Console.WriteLine("============================ RetryMechanism ============================"); - - IKernel kernel = Kernel.Builder.WithLogger(ConsoleLogger.Log).Build(); - kernel.Config.SetRetryMechanism(new RetryThreeTimesWithBackoff()); - - // OpenAI settings - kernel.Config.AddOpenAICompletionBackend("text-davinci-003", "text-davinci-003", Env.Var("OPENAI_API_KEY")); - - // Load semantic skill defined with prompt templates - string folder = RepoFiles.SampleSkillsPath(); - - kernel.ImportSkill(new TimeSkill(), "time"); - - var qaSkill = kernel.ImportSemanticSkillFromDirectory( - folder, - "QASkill"); - - var question = "How popular is Polly library?"; - var answer = await kernel.RunAsync(question, qaSkill["Question"]); - - Console.WriteLine($"Question: {question}\n\n" + answer); - } -} diff --git a/samples/dotnet/kernel-syntax-examples/Example12_Planning.cs b/samples/dotnet/kernel-syntax-examples/Example12_Planning.cs index f0038055c222..6bc930db92d3 100644 --- a/samples/dotnet/kernel-syntax-examples/Example12_Planning.cs +++ b/samples/dotnet/kernel-syntax-examples/Example12_Planning.cs @@ -9,7 +9,6 @@ using Microsoft.SemanticKernel.KernelExtensions; using Microsoft.SemanticKernel.Orchestration; using Microsoft.SemanticKernel.Orchestration.Extensions; -using Reliability; using RepoUtils; using Skills; @@ -46,7 +45,7 @@ private static async Task PoetrySamplesAsync() Console.WriteLine("Original plan:"); Console.WriteLine(originalPlan.Variables.ToPlan().PlanString); - _ = await ExecutePlanAsync(kernel, planner, originalPlan, 5); + await ExecutePlanAsync(kernel, planner, originalPlan, 5); } private static async Task EmailSamplesAsync() @@ -87,7 +86,7 @@ private static async Task EmailSamplesAsync() "and the kingdom was at peace once again. The king was so grateful to Mira that he asked her to marry him and she agreed. " + "They ruled the kingdom together, ruling with fairness and compassion, just as Arjun had done before. They lived " + "happily ever after, with the people of the kingdom remembering Mira as the brave young woman who saved them from the dragon."); - _ = await ExecutePlanAsync(kernel, planner, executionResults, 5); + await ExecutePlanAsync(kernel, planner, executionResults, 5); } private static async Task BookSamplesAsync() @@ -118,7 +117,7 @@ private static async Task BookSamplesAsync() Stopwatch sw = new(); sw.Start(); - _ = await ExecutePlanAsync(kernel, planner, originalPlan); + await ExecutePlanAsync(kernel, planner, originalPlan); } private static IKernel InitializeKernelAndPlanner(out IDictionary planner) @@ -129,7 +128,6 @@ private static IKernel InitializeKernelAndPlanner(out IDictionary /// An example of a retry mechanism that retries three times with backoff. /// -public class RetryThreeTimesWithBackoff : IRetryMechanism +public class RetryThreeTimesWithBackoff : DelegatingHandler { - public Task ExecuteWithRetryAsync(Func action, ILogger log, CancellationToken cancellationToken = default) + private readonly AsyncRetryPolicy _policy; + + public RetryThreeTimesWithBackoff(ILogger log) + { + this._policy = GetPolicy(log); + } + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { - var policy = GetPolicy(log); - return policy.ExecuteAsync((_) => action(), cancellationToken); + return await this._policy.ExecuteAsync(async () => + { + var response = await base.SendAsync(request, cancellationToken); + return response; + }); } - private static AsyncRetryPolicy GetPolicy(ILogger log) + private static AsyncRetryPolicy GetPolicy(ILogger log) { + // Handle 429 and 401 errors + // Typically 401 would not be something we retry but for demonstration + // purposes we are doing so as it's easy to trigger when using an invalid key. return Policy - .Handle(ex => ex.ErrorCode == AIException.ErrorCodes.Throttling) + .HandleResult(response => + response.StatusCode is System.Net.HttpStatusCode.TooManyRequests or System.Net.HttpStatusCode.Unauthorized) .WaitAndRetryAsync(new[] { TimeSpan.FromSeconds(2), TimeSpan.FromSeconds(4), TimeSpan.FromSeconds(8) }, - (ex, timespan, retryCount, _) => log.LogWarning(ex, - "Error executing action [attempt {0} of ], pausing {1} msecs", retryCount, timespan.TotalMilliseconds)); + (outcome, timespan, retryCount, _) => log.LogWarning( + "Error executing action [attempt {0} of 3], pausing {1}ms. Outcome: {2}", + retryCount, + timespan.TotalMilliseconds, + outcome.Result.StatusCode)); } } diff --git a/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithRetryAfterBackoff.cs b/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithRetryAfterBackoff.cs new file mode 100644 index 000000000000..2da172becf75 --- /dev/null +++ b/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithRetryAfterBackoff.cs @@ -0,0 +1,69 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.SemanticKernel.Reliability; +using Polly; +using Polly.Retry; + +namespace Reliability; + +public class RetryThreeTimesWithRetryAfterBackoffFactory : IDelegatingHandlerFactory +{ + public DelegatingHandler Create(ILogger log) + { + return new RetryThreeTimesWithRetryAfterBackoff(log); + } +} + +/// +/// An example of a retry mechanism that retries three times with backoff using the RetryAfter value. +/// +public class RetryThreeTimesWithRetryAfterBackoff : DelegatingHandler +{ + private readonly AsyncRetryPolicy _policy; + + public RetryThreeTimesWithRetryAfterBackoff(ILogger log) + { + this._policy = GetPolicy(log); + } + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + return await this._policy.ExecuteAsync(async () => + { + var response = await base.SendAsync(request, cancellationToken); + return response; + }); + } + + private static AsyncRetryPolicy GetPolicy(ILogger log) + { + // Handle 429 and 401 errors + // Typically 401 would not be something we retry but for demonstration + // purposes we are doing so as it's easy to trigger when using an invalid key. + return Policy + .HandleResult(response => + response.StatusCode is System.Net.HttpStatusCode.TooManyRequests or System.Net.HttpStatusCode.Unauthorized) + .WaitAndRetryAsync( + retryCount: 3, + sleepDurationProvider: (_, r, _) => + { + var response = r.Result; + var retryAfter = response.Headers.RetryAfter?.Delta ?? response.Headers.RetryAfter?.Date - DateTimeOffset.Now; + return retryAfter ?? TimeSpan.FromSeconds(2); + }, + (outcome, timespan, retryCount, _) => + { + log.LogWarning( + "Error executing action [attempt {0} of 3], pausing {1}ms. Outcome: {2}", + retryCount, + timespan.TotalMilliseconds, + outcome.Result.StatusCode); + return Task.CompletedTask; + }); + } +} diff --git a/samples/dotnet/kernel-syntax-examples/RepoUtils/ConsoleLogger.cs b/samples/dotnet/kernel-syntax-examples/RepoUtils/ConsoleLogger.cs index 51aa7055668b..a3d7ff63a8f9 100644 --- a/samples/dotnet/kernel-syntax-examples/RepoUtils/ConsoleLogger.cs +++ b/samples/dotnet/kernel-syntax-examples/RepoUtils/ConsoleLogger.cs @@ -13,18 +13,20 @@ internal static class ConsoleLogger internal static ILogger Log => LogFactory.CreateLogger(); private static ILoggerFactory LogFactory => s_loggerFactory.Value; + private static readonly Lazy s_loggerFactory = new(LogBuilder); private static ILoggerFactory LogBuilder() { return LoggerFactory.Create(builder => { - builder.SetMinimumLevel(LogLevel.Warning); + // builder.SetMinimumLevel(LogLevel.Warning); // builder.AddFilter("Microsoft", LogLevel.Trace); // builder.AddFilter("Microsoft", LogLevel.Debug); // builder.AddFilter("Microsoft", LogLevel.Information); - // builder.AddFilter("Microsoft", LogLevel.Warning); + builder.AddFilter("Microsoft", LogLevel.Warning); // builder.AddFilter("Microsoft", LogLevel.Error); + builder.AddFilter("System", LogLevel.Warning); builder.AddConsole(); }); } From dae56cb38a06f105ef957133339ff98542b7be84 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Fri, 10 Mar 2023 16:43:03 -0800 Subject: [PATCH 04/22] Make DefaultHttpRetryHandler and DefaultHttpRetryHandlerFactory public --- .../src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs | 2 +- .../Reliability/DefaultHttpRetryHandlerFactory.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index c0bf8bedb885..dee1cb9d1ec2 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -12,7 +12,7 @@ namespace Microsoft.SemanticKernel.Reliability; -internal sealed class DefaultHttpRetryHandler : DelegatingHandler +public sealed class DefaultHttpRetryHandler : DelegatingHandler { /// /// Initializes a new instance of the class. diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs index 5651bed728cd..06be790f1130 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs @@ -6,7 +6,7 @@ namespace Microsoft.SemanticKernel.Reliability; -internal class DefaultHttpRetryHandlerFactory : IDelegatingHandlerFactory +public class DefaultHttpRetryHandlerFactory : IDelegatingHandlerFactory { internal DefaultHttpRetryHandlerFactory(KernelConfig.HttpRetryConfig? config = null) { From 3d1ccf025a50397d934afb1d08bb62e4b24bfb4a Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Fri, 10 Mar 2023 17:53:24 -0800 Subject: [PATCH 05/22] PR feedback on tests and samples --- .../AI/OpenAICompletionTests.cs | 3 +-- samples/dotnet/KernelBuilder/Program.cs | 22 ++++++++++++++++--- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs b/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs index 58273c9ad92d..706b4494173d 100644 --- a/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs +++ b/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs @@ -105,13 +105,12 @@ public async Task OpenAIHttpRetryPolicyTestAsync(string prompt, string expectedO OpenAIConfiguration? openAIConfiguration = this._configuration.GetSection("OpenAI").Get(); Assert.NotNull(openAIConfiguration); + // Use an invalid API key to force a 401 Unauthorized response target.Config.AddOpenAICompletionBackend( label: openAIConfiguration.Label, modelId: openAIConfiguration.ModelId, apiKey: "INVALID_KEY"); - target.Config.SetDefaultCompletionBackend(openAIConfiguration.Label); - IDictionary skill = GetSkill("SummarizeSkill", target); // Act diff --git a/samples/dotnet/KernelBuilder/Program.cs b/samples/dotnet/KernelBuilder/Program.cs index 2fef62390d24..51f4cca6cd9e 100644 --- a/samples/dotnet/KernelBuilder/Program.cs +++ b/samples/dotnet/KernelBuilder/Program.cs @@ -99,14 +99,30 @@ .SetDefaultEmbeddingsBackend("myName3"); // ========================================================================================================== -// When invoking AI, by default the kernel will not retry on transient errors, such as throttling -// and timeouts. This behavior can be customized injecting a retry strategy that applies to all +// When invoking AI, by default the kernel will retry on transient errors, such as throttling and timeouts. +// The default behavior can be configured or a custom retry handler can be injected that will apply to all // AI requests (when using the kernel). var kernel8 = Kernel.Builder - .Configure(c => c.SetHttpRetryHandlerFactory(new RetryThreeTimesFactory())) + .Configure(c => c.SetDefaultHttpRetryConfig(new KernelConfig.HttpRetryConfig + { + MaxRetryCount = 3, + UseExponentialBackoff = true, + // MinRetryDelay = TimeSpan.FromSeconds(2), + // MaxRetryDelay = TimeSpan.FromSeconds(8), + // MaxTotalRetryTime = TimeSpan.FromSeconds(30), + // RetryableStatusCodes = new[] { HttpStatusCode.TooManyRequests, HttpStatusCode.RequestTimeout }, + // RetryableExceptions = new[] { typeof(HttpRequestException) } + })) + .Build(); + +var kernel9 = Kernel.Builder + .Configure(c => c.SetHttpRetryHandlerFactory(new NullHttpRetryHandlerFactory())) .Build(); +var kernel10 = Kernel.Builder.WithRetryHandlerFactory(new RetryThreeTimesFactory()).Build(); + +// Example of a basic custom retry handler public class RetryThreeTimesFactory : IDelegatingHandlerFactory { public DelegatingHandler Create(ILogger log) From 35347686bdba6458019d89148855b3b7e343f522 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Fri, 10 Mar 2023 17:55:09 -0800 Subject: [PATCH 06/22] kernel syntax examples --- .../Reliability/RetryThreeTimesWithBackoff.cs | 5 ++++- .../Reliability/RetryThreeTimesWithRetryAfterBackoff.cs | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithBackoff.cs b/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithBackoff.cs index bc7e644b696a..ec2176e696d8 100644 --- a/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithBackoff.cs +++ b/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithBackoff.cs @@ -11,6 +11,9 @@ namespace Reliability; +/// +/// A factory for creating a retry handler. +/// public class RetryThreeTimesWithBackoffFactory : IDelegatingHandlerFactory { public DelegatingHandler Create(ILogger log) @@ -20,7 +23,7 @@ public DelegatingHandler Create(ILogger log) } /// -/// An example of a retry mechanism that retries three times with backoff. +/// A basic example of a retry mechanism that retries three times with backoff. /// public class RetryThreeTimesWithBackoff : DelegatingHandler { diff --git a/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithRetryAfterBackoff.cs b/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithRetryAfterBackoff.cs index 2da172becf75..e55591ae88df 100644 --- a/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithRetryAfterBackoff.cs +++ b/samples/dotnet/kernel-syntax-examples/Reliability/RetryThreeTimesWithRetryAfterBackoff.cs @@ -11,6 +11,9 @@ namespace Reliability; +/// +/// A factory for creating a retry handler. +/// public class RetryThreeTimesWithRetryAfterBackoffFactory : IDelegatingHandlerFactory { public DelegatingHandler Create(ILogger log) From 0a6fd7403d04701bf3c0a1beaad82d0218201f6f Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Fri, 10 Mar 2023 18:12:07 -0800 Subject: [PATCH 07/22] Relocate HttpRetryConfig class to Reliability namespace Summary: This commit relocates the HttpRetryConfig class, which defines the retry policy for HTTP requests, from the Configuration namespace to the Reliability namespace. This change improves the code organization and cohesion, as the class is more logically related to the DefaultHttpRetryHandler and its factory, which implement the retry logic. The commit also removes some unused code and simplifies some exception messages. The references and tests for the class are updated accordingly. --- .../AI/OpenAICompletionTests.cs | 3 +- .../Configuration/KernelConfigTests.cs | 64 --------------- .../DefaultHttpRetryHandlerTests.cs | 1 - .../Reliability/HttpRetryConfigTests.cs | 77 +++++++++++++++++++ .../Configuration/KernelConfig.cs | 76 +----------------- .../Reliability/DefaultHttpRetryHandler.cs | 9 +-- .../DefaultHttpRetryHandlerFactory.cs | 5 +- .../Reliability/HttpRetryConfig.cs | 73 ++++++++++++++++++ samples/dotnet/KernelBuilder/Program.cs | 2 +- .../Example08_RetryHandler.cs | 7 +- 10 files changed, 164 insertions(+), 153 deletions(-) create mode 100644 dotnet/src/SemanticKernel.UnitTests/Reliability/HttpRetryConfigTests.cs create mode 100644 dotnet/src/SemanticKernel/Reliability/HttpRetryConfig.cs diff --git a/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs b/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs index 706b4494173d..c9ae6775e300 100644 --- a/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs +++ b/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs @@ -7,7 +7,6 @@ using System.Threading.Tasks; using Microsoft.Extensions.Configuration; using Microsoft.SemanticKernel; -using Microsoft.SemanticKernel.Configuration; using Microsoft.SemanticKernel.KernelExtensions; using Microsoft.SemanticKernel.Orchestration; using SemanticKernel.IntegrationTests.TestSettings; @@ -98,7 +97,7 @@ public async Task AzureOpenAITestAsync(string prompt, string expectedAnswerConta public async Task OpenAIHttpRetryPolicyTestAsync(string prompt, string expectedOutput) { // Arrange - var retryConfig = new KernelConfig.HttpRetryConfig(); + var retryConfig = new HttpRetryConfig(); retryConfig.RetryableStatusCodes.Add(System.Net.HttpStatusCode.Unauthorized); IKernel target = Kernel.Builder.WithLogger(this._testOutputHelper).Configure(c => c.SetDefaultHttpRetryConfig(retryConfig)).Build(); diff --git a/dotnet/src/SemanticKernel.UnitTests/Configuration/KernelConfigTests.cs b/dotnet/src/SemanticKernel.UnitTests/Configuration/KernelConfigTests.cs index c1b45623f980..a8baf80b3675 100644 --- a/dotnet/src/SemanticKernel.UnitTests/Configuration/KernelConfigTests.cs +++ b/dotnet/src/SemanticKernel.UnitTests/Configuration/KernelConfigTests.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. using System.Linq; -using System.Threading.Tasks; using Microsoft.SemanticKernel; using Microsoft.SemanticKernel.AI.OpenAI.Services; using Microsoft.SemanticKernel.Configuration; @@ -68,69 +67,6 @@ public void HttpRetryHandlerFactoryIsSetToDefaultHttpRetryHandlerFactoryIfNotSet Assert.IsType(config.HttpHandlerFactory); } - [Fact] - public async Task NegativeMaxRetryCountThrowsAsync() - { - // Act - await Assert.ThrowsAsync(() => - { - var httpRetryConfig = new KernelConfig.HttpRetryConfig() { MaxRetryCount = -1 }; - return Task.CompletedTask; - }); - } - - [Fact] - public void SetDefaultHttpRetryConfig() - { - // Arrange - var config = new KernelConfig(); - var httpRetryConfig = new KernelConfig.HttpRetryConfig() { MaxRetryCount = 1 }; - - // Act - config.SetDefaultHttpRetryConfig(httpRetryConfig); - - // Assert - Assert.Equal(httpRetryConfig, config.DefaultHttpRetryConfig); - } - - [Fact] - public void SetDefaultHttpRetryConfigToDefaultIfNotSet() - { - // Arrange - var config = new KernelConfig(); - - // Act - // Assert - var defaultConfig = new KernelConfig.HttpRetryConfig(); - Assert.Equal(defaultConfig.MaxRetryCount, config.DefaultHttpRetryConfig.MaxRetryCount); - Assert.Equal(defaultConfig.MaxRetryDelay, config.DefaultHttpRetryConfig.MaxRetryDelay); - Assert.Equal(defaultConfig.MinRetryDelay, config.DefaultHttpRetryConfig.MinRetryDelay); - Assert.Equal(defaultConfig.MaxTotalRetryTime, config.DefaultHttpRetryConfig.MaxTotalRetryTime); - Assert.Equal(defaultConfig.UseExponentialBackoff, config.DefaultHttpRetryConfig.UseExponentialBackoff); - Assert.Equal(defaultConfig.RetryableStatusCodes, config.DefaultHttpRetryConfig.RetryableStatusCodes); - Assert.Equal(defaultConfig.RetryableExceptionTypes, config.DefaultHttpRetryConfig.RetryableExceptionTypes); - } - - [Fact] - public void SetDefaultHttpRetryConfigToDefaultIfNull() - { - // Arrange - var config = new KernelConfig(); - - // Act - config.SetDefaultHttpRetryConfig(null); - - // Assert - var defaultConfig = new KernelConfig.HttpRetryConfig(); - Assert.Equal(defaultConfig.MaxRetryCount, config.DefaultHttpRetryConfig.MaxRetryCount); - Assert.Equal(defaultConfig.MaxRetryDelay, config.DefaultHttpRetryConfig.MaxRetryDelay); - Assert.Equal(defaultConfig.MinRetryDelay, config.DefaultHttpRetryConfig.MinRetryDelay); - Assert.Equal(defaultConfig.MaxTotalRetryTime, config.DefaultHttpRetryConfig.MaxTotalRetryTime); - Assert.Equal(defaultConfig.UseExponentialBackoff, config.DefaultHttpRetryConfig.UseExponentialBackoff); - Assert.Equal(defaultConfig.RetryableStatusCodes, config.DefaultHttpRetryConfig.RetryableStatusCodes); - Assert.Equal(defaultConfig.RetryableExceptionTypes, config.DefaultHttpRetryConfig.RetryableExceptionTypes); - } - [Fact] public void ItFailsWhenAddingCompletionBackendsWithSameLabel() { diff --git a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs index 1be47fc70140..4a0f6210d567 100644 --- a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs +++ b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs @@ -12,7 +12,6 @@ using Moq; using Moq.Protected; using Xunit; -using static Microsoft.SemanticKernel.Configuration.KernelConfig; namespace SemanticKernel.UnitTests.Reliability; diff --git a/dotnet/src/SemanticKernel.UnitTests/Reliability/HttpRetryConfigTests.cs b/dotnet/src/SemanticKernel.UnitTests/Reliability/HttpRetryConfigTests.cs new file mode 100644 index 000000000000..e57c238df8d3 --- /dev/null +++ b/dotnet/src/SemanticKernel.UnitTests/Reliability/HttpRetryConfigTests.cs @@ -0,0 +1,77 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.Threading.Tasks; +using Microsoft.SemanticKernel.Configuration; +using Microsoft.SemanticKernel.Reliability; +using Xunit; + +namespace SemanticKernel.UnitTests.Reliability; + +/// +/// Unit tests of . +/// +public class HttpRetryConfigTests +{ + [Fact] + public async Task NegativeMaxRetryCountThrowsAsync() + { + // Act + await Assert.ThrowsAsync(() => + { + var httpRetryConfig = new HttpRetryConfig() { MaxRetryCount = -1 }; + return Task.CompletedTask; + }); + } + + [Fact] + public void SetDefaultHttpRetryConfig() + { + // Arrange + var config = new KernelConfig(); + var httpRetryConfig = new HttpRetryConfig() { MaxRetryCount = 1 }; + + // Act + config.SetDefaultHttpRetryConfig(httpRetryConfig); + + // Assert + Assert.Equal(httpRetryConfig, config.DefaultHttpRetryConfig); + } + + [Fact] + public void SetDefaultHttpRetryConfigToDefaultIfNotSet() + { + // Arrange + var config = new KernelConfig(); + + // Act + // Assert + var defaultConfig = new HttpRetryConfig(); + Assert.Equal(defaultConfig.MaxRetryCount, config.DefaultHttpRetryConfig.MaxRetryCount); + Assert.Equal(defaultConfig.MaxRetryDelay, config.DefaultHttpRetryConfig.MaxRetryDelay); + Assert.Equal(defaultConfig.MinRetryDelay, config.DefaultHttpRetryConfig.MinRetryDelay); + Assert.Equal(defaultConfig.MaxTotalRetryTime, config.DefaultHttpRetryConfig.MaxTotalRetryTime); + Assert.Equal(defaultConfig.UseExponentialBackoff, config.DefaultHttpRetryConfig.UseExponentialBackoff); + Assert.Equal(defaultConfig.RetryableStatusCodes, config.DefaultHttpRetryConfig.RetryableStatusCodes); + Assert.Equal(defaultConfig.RetryableExceptionTypes, config.DefaultHttpRetryConfig.RetryableExceptionTypes); + } + + [Fact] + public void SetDefaultHttpRetryConfigToDefaultIfNull() + { + // Arrange + var config = new KernelConfig(); + + // Act + config.SetDefaultHttpRetryConfig(null); + + // Assert + var defaultConfig = new HttpRetryConfig(); + Assert.Equal(defaultConfig.MaxRetryCount, config.DefaultHttpRetryConfig.MaxRetryCount); + Assert.Equal(defaultConfig.MaxRetryDelay, config.DefaultHttpRetryConfig.MaxRetryDelay); + Assert.Equal(defaultConfig.MinRetryDelay, config.DefaultHttpRetryConfig.MinRetryDelay); + Assert.Equal(defaultConfig.MaxTotalRetryTime, config.DefaultHttpRetryConfig.MaxTotalRetryTime); + Assert.Equal(defaultConfig.UseExponentialBackoff, config.DefaultHttpRetryConfig.UseExponentialBackoff); + Assert.Equal(defaultConfig.RetryableStatusCodes, config.DefaultHttpRetryConfig.RetryableStatusCodes); + Assert.Equal(defaultConfig.RetryableExceptionTypes, config.DefaultHttpRetryConfig.RetryableExceptionTypes); + } +} diff --git a/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs b/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs index 132b74de66c5..61c2c73ec2b7 100644 --- a/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs +++ b/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs @@ -3,9 +3,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Net; -using System.Net.Http; -using Microsoft.Extensions.Logging.Abstractions; using Microsoft.SemanticKernel.AI.OpenAI.Services; using Microsoft.SemanticKernel.Diagnostics; using Microsoft.SemanticKernel.Reliability; @@ -17,71 +14,6 @@ namespace Microsoft.SemanticKernel.Configuration; /// public sealed class KernelConfig { - /// - /// Retry configuration for IHttpRetryPolicy that uses RetryAfter header when present. - /// - public sealed class HttpRetryConfig - { - /// - /// Maximum number of retries. - /// - /// Thrown when value is negative. - public int MaxRetryCount - { - get { return this._maxRetryCount; } - set - { - if (value < 0) - { - throw new ArgumentOutOfRangeException(nameof(this.MaxRetryCount), "Max retry count cannot be negative."); - } - - this._maxRetryCount = value; - } - } - - /// - /// Minimum delay between retries. - /// - public TimeSpan MinRetryDelay { get; set; } = TimeSpan.FromSeconds(2); - - /// - /// Maximum delay between retries. - /// - public TimeSpan MaxRetryDelay { get; set; } = TimeSpan.FromSeconds(60); - - /// - /// Maximum total time spent retrying. - /// - public TimeSpan MaxTotalRetryTime { get; set; } = TimeSpan.FromMinutes(2); - - /// - /// Whether to use exponential backoff or not. - /// - public bool UseExponentialBackoff { get; set; } - - /// - /// List of status codes that should be retried. - /// - public List RetryableStatusCodes { get; set; } = new() - { - HttpStatusCode.RequestTimeout, - HttpStatusCode.ServiceUnavailable, - HttpStatusCode.GatewayTimeout, - HttpStatusCode.TooManyRequests - }; - - /// - /// List of exception types that should be retried. - /// - public List RetryableExceptionTypes { get; set; } = new() - { - typeof(HttpRequestException) - }; - - private int _maxRetryCount = 1; - } - /// /// Global retry logic used for all the backends http calls /// @@ -336,7 +268,7 @@ public IBackendConfig GetCompletionBackend(string? label = null) { throw new KernelException( KernelException.ErrorCodes.BackendNotFound, - $"A label was not provided and no default completion backend is available."); + "A label was not provided and no default completion backend is available."); } return this.CompletionBackends[this._defaultCompletionBackend]; @@ -371,7 +303,7 @@ public IBackendConfig GetEmbeddingsBackend(string? label = null) { throw new KernelException( KernelException.ErrorCodes.BackendNotFound, - $"A label was not provided and no default embeddings backend is available."); + "A label was not provided and no default embeddings backend is available."); } return this.EmbeddingsBackends[this._defaultEmbeddingsBackend]; @@ -477,8 +409,8 @@ public KernelConfig RemoveAllBackends() #region private - private Dictionary CompletionBackends { get; set; } = new(); - private Dictionary EmbeddingsBackends { get; set; } = new(); + private Dictionary CompletionBackends { get; } = new(); + private Dictionary EmbeddingsBackends { get; } = new(); private string? _defaultCompletionBackend; private string? _defaultEmbeddingsBackend; diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index dee1cb9d1ec2..c077f0ebe191 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -8,7 +8,6 @@ using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; -using Microsoft.SemanticKernel.Configuration; namespace Microsoft.SemanticKernel.Reliability; @@ -19,14 +18,12 @@ public sealed class DefaultHttpRetryHandler : DelegatingHandler /// /// The retry configuration. /// The logger. - public DefaultHttpRetryHandler(KernelConfig.HttpRetryConfig? config = null, ILogger? log = null) : this(config ?? new KernelConfig.HttpRetryConfig(), log, + public DefaultHttpRetryHandler(HttpRetryConfig? config = null, ILogger? log = null) : this(config ?? new HttpRetryConfig(), log, null, null) { } - public readonly Guid Id = Guid.NewGuid(); - - internal DefaultHttpRetryHandler(KernelConfig.HttpRetryConfig config, ILogger? log = null, IDelayProvider? delayProvider = null, + internal DefaultHttpRetryHandler(HttpRetryConfig config, ILogger? log = null, IDelayProvider? delayProvider = null, ITimeProvider? timeProvider = null) { this._config = config; @@ -223,7 +220,7 @@ await originalRequest.Content.ReadAsStreamAsync().ContinueWith(t => return newRequest; } - private readonly KernelConfig.HttpRetryConfig _config; + private readonly HttpRetryConfig _config; private readonly ILogger _log; private readonly IDelayProvider _delayProvider; private readonly ITimeProvider _timeProvider; diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs index 06be790f1130..5b689b8ecd6b 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs @@ -2,13 +2,12 @@ using System.Net.Http; using Microsoft.Extensions.Logging; -using Microsoft.SemanticKernel.Configuration; namespace Microsoft.SemanticKernel.Reliability; public class DefaultHttpRetryHandlerFactory : IDelegatingHandlerFactory { - internal DefaultHttpRetryHandlerFactory(KernelConfig.HttpRetryConfig? config = null) + internal DefaultHttpRetryHandlerFactory(HttpRetryConfig? config = null) { this._config = config; } @@ -18,5 +17,5 @@ public DelegatingHandler Create(ILogger log) return new DefaultHttpRetryHandler(this._config, log); } - private readonly KernelConfig.HttpRetryConfig? _config; + private readonly HttpRetryConfig? _config; } diff --git a/dotnet/src/SemanticKernel/Reliability/HttpRetryConfig.cs b/dotnet/src/SemanticKernel/Reliability/HttpRetryConfig.cs new file mode 100644 index 000000000000..cf8a0e93f9ff --- /dev/null +++ b/dotnet/src/SemanticKernel/Reliability/HttpRetryConfig.cs @@ -0,0 +1,73 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; + +namespace Microsoft.SemanticKernel.Reliability; + +/// +/// Retry configuration for IHttpRetryPolicy that uses RetryAfter header when present. +/// +public sealed class HttpRetryConfig +{ + /// + /// Maximum number of retries. + /// + /// Thrown when value is negative. + public int MaxRetryCount + { + get => this._maxRetryCount; + set + { + if (value < 0) + { + throw new ArgumentOutOfRangeException(nameof(this.MaxRetryCount), "Max retry count cannot be negative."); + } + + this._maxRetryCount = value; + } + } + + /// + /// Minimum delay between retries. + /// + public TimeSpan MinRetryDelay { get; set; } = TimeSpan.FromSeconds(2); + + /// + /// Maximum delay between retries. + /// + public TimeSpan MaxRetryDelay { get; set; } = TimeSpan.FromSeconds(60); + + /// + /// Maximum total time spent retrying. + /// + public TimeSpan MaxTotalRetryTime { get; set; } = TimeSpan.FromMinutes(2); + + /// + /// Whether to use exponential backoff or not. + /// + public bool UseExponentialBackoff { get; set; } + + /// + /// List of status codes that should be retried. + /// + public List RetryableStatusCodes { get; set; } = new() + { + HttpStatusCode.RequestTimeout, + HttpStatusCode.ServiceUnavailable, + HttpStatusCode.GatewayTimeout, + HttpStatusCode.TooManyRequests + }; + + /// + /// List of exception types that should be retried. + /// + public List RetryableExceptionTypes { get; set; } = new() + { + typeof(HttpRequestException) + }; + + private int _maxRetryCount = 1; +} diff --git a/samples/dotnet/KernelBuilder/Program.cs b/samples/dotnet/KernelBuilder/Program.cs index 51f4cca6cd9e..5f0e6c6d775a 100644 --- a/samples/dotnet/KernelBuilder/Program.cs +++ b/samples/dotnet/KernelBuilder/Program.cs @@ -104,7 +104,7 @@ // AI requests (when using the kernel). var kernel8 = Kernel.Builder - .Configure(c => c.SetDefaultHttpRetryConfig(new KernelConfig.HttpRetryConfig + .Configure(c => c.SetDefaultHttpRetryConfig(new HttpRetryConfig { MaxRetryCount = 3, UseExponentialBackoff = true, diff --git a/samples/dotnet/kernel-syntax-examples/Example08_RetryHandler.cs b/samples/dotnet/kernel-syntax-examples/Example08_RetryHandler.cs index 44902ae62133..acbbdc316372 100644 --- a/samples/dotnet/kernel-syntax-examples/Example08_RetryHandler.cs +++ b/samples/dotnet/kernel-syntax-examples/Example08_RetryHandler.cs @@ -4,7 +4,6 @@ using System.Threading.Tasks; using Microsoft.Extensions.Logging; using Microsoft.SemanticKernel; -using Microsoft.SemanticKernel.Configuration; using Microsoft.SemanticKernel.CoreSkills; using Microsoft.SemanticKernel.KernelExtensions; using Microsoft.SemanticKernel.Reliability; @@ -28,13 +27,13 @@ public static async Task RunAsync() await RunRetryPolicyBuilderAsync(typeof(NullHttpRetryHandlerFactory)); ConsoleLogger.Log.LogInformation("=============================== DefaultHttpRetryHandler ================================"); - await RunRetryHandlerConfigAsync(new KernelConfig.HttpRetryConfig() { MaxRetryCount = 3, UseExponentialBackoff = true }); + await RunRetryHandlerConfigAsync(new HttpRetryConfig() { MaxRetryCount = 3, UseExponentialBackoff = true }); ConsoleLogger.Log.LogInformation("======= DefaultHttpRetryConfig [MaxRetryCount = 3, UseExponentialBackoff = true] ======="); - await RunRetryHandlerConfigAsync(new KernelConfig.HttpRetryConfig() { MaxRetryCount = 3, UseExponentialBackoff = true }); + await RunRetryHandlerConfigAsync(new HttpRetryConfig() { MaxRetryCount = 3, UseExponentialBackoff = true }); } - private static async Task RunRetryHandlerConfigAsync(KernelConfig.HttpRetryConfig? config = null) + private static async Task RunRetryHandlerConfigAsync(HttpRetryConfig? config = null) { var kernelBuilder = Kernel.Builder.WithLogger(ConsoleLogger.Log); if (config != null) From 38a55d3d00075133e0bcd2f1267487218b3975f0 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Fri, 10 Mar 2023 18:21:45 -0800 Subject: [PATCH 08/22] r# cleanup first, competes with format --- .vscode/tasks.json | 4 ++-- .../Reliability/HttpRetryConfig.cs | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/.vscode/tasks.json b/.vscode/tasks.json index be95901abec2..d2db660cf8d3 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -74,10 +74,10 @@ "detail": "Runs tasks to validate changes before checking in.", "group": "test", "dependsOn": [ + "R# cleanup", "Build - Semantic-Kernel", "Test - Semantic-Kernel", - "Run - Kernel-Demo", - "R# cleanup" + "Run - Kernel-Demo" ], "dependsOrder": "sequence" }, diff --git a/dotnet/src/SemanticKernel/Reliability/HttpRetryConfig.cs b/dotnet/src/SemanticKernel/Reliability/HttpRetryConfig.cs index cf8a0e93f9ff..3b1a8b53fb21 100644 --- a/dotnet/src/SemanticKernel/Reliability/HttpRetryConfig.cs +++ b/dotnet/src/SemanticKernel/Reliability/HttpRetryConfig.cs @@ -54,20 +54,20 @@ public int MaxRetryCount /// List of status codes that should be retried. /// public List RetryableStatusCodes { get; set; } = new() - { - HttpStatusCode.RequestTimeout, - HttpStatusCode.ServiceUnavailable, - HttpStatusCode.GatewayTimeout, - HttpStatusCode.TooManyRequests - }; + { + HttpStatusCode.RequestTimeout, + HttpStatusCode.ServiceUnavailable, + HttpStatusCode.GatewayTimeout, + HttpStatusCode.TooManyRequests + }; /// /// List of exception types that should be retried. /// public List RetryableExceptionTypes { get; set; } = new() - { - typeof(HttpRequestException) - }; + { + typeof(HttpRequestException) + }; private int _maxRetryCount = 1; } From cbeb26406f701522f54c527580eb45e43bdc4d50 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Mon, 13 Mar 2023 11:30:35 -0700 Subject: [PATCH 09/22] Improve retry logic and documentation in DefaultHttpRetryHandler Summary: This commit makes several improvements to the DefaultHttpRetryHandler class, which handles retrying HTTP requests in case of failures. The main changes are: - Add more comments to explain the logic and parameters of the retry methods. - Refactor the catch block to avoid re-throwing the exception if the max retry count is reached or there is no time left for a retry. - Use the current time from the ITimeProvider interface instead of DateTimeOffset.Now, to enable unit testing. - Make the CloneAsync method private, as it is only used internally by the class. - Add null check for the exception parameter in the ShouldRetry method. --- .../Reliability/DefaultHttpRetryHandler.cs | 54 ++++++++++++++++--- 1 file changed, 47 insertions(+), 7 deletions(-) diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index c077f0ebe191..aba076c59c25 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -68,7 +68,7 @@ protected override async Task SendAsync(HttpRequestMessage } // Drain response content to free connections. Need to perform this - // before retry attempt and before the TooManyRetries ServiceException. + // before retry attempt and before re-throwing. if (response.Content != null) { #if NET5_0_OR_GREATER @@ -80,6 +80,8 @@ protected override async Task SendAsync(HttpRequestMessage reason = response.StatusCode.ToString(); + // If the retry count is greater than the max retry count then we'll + // just return if (retryCount >= this._config.MaxRetryCount) { this._log.LogError( @@ -96,11 +98,21 @@ protected override async Task SendAsync(HttpRequestMessage return response; } } - catch (Exception e) when ((this.ShouldRetry(e) || this.ShouldRetry(e.InnerException)) && - retryCount < this._config.MaxRetryCount && - this.HasTimeForRetry(start, retryCount, response, out waitFor)) + catch (Exception e) when (this.ShouldRetry(e) || this.ShouldRetry(e.InnerException)) { reason = e.GetType().ToString(); + if (retryCount >= this._config.MaxRetryCount) + { + this._log.LogError(e, + "Error executing request, max retry count reached. Reason: {0}", reason); + throw; + } + else if (!this.HasTimeForRetry(start, retryCount, response, out waitFor)) + { + this._log.LogError(e, + "Error executing request, max total retry time reached. Reason: {0}", reason); + throw; + } } // If the request requires a retry then we'll retry @@ -125,19 +137,28 @@ protected override async Task SendAsync(HttpRequestMessage } } + /// + /// Get the wait time for the next retry. + /// + /// Current retry count + /// The response message that potentially contains RetryAfter header. private TimeSpan GetWaitTime(int retryCount, HttpResponseMessage? response) { + // If the response contains a RetryAfter header, use that value + // Otherwise, use the configured min retry delay var retryAfter = response?.Headers.RetryAfter?.Date.HasValue == true - ? response?.Headers.RetryAfter?.Date - DateTimeOffset.Now + ? response?.Headers.RetryAfter?.Date - this._timeProvider.GetCurrentTime() : (response?.Headers.RetryAfter?.Delta) ?? this._config.MinRetryDelay; retryAfter ??= this._config.MinRetryDelay; + // If the retry delay is longer than the max retry delay, use the max retry delay var timeToWait = retryAfter > this._config.MaxRetryDelay ? this._config.MaxRetryDelay : retryAfter < this._config.MinRetryDelay ? this._config.MinRetryDelay : retryAfter ?? default; + // If exponential backoff is enabled, double the delay for each retry if (this._config.UseExponentialBackoff) { for (var backoffRetryCount = 1; backoffRetryCount < retryCount + 1; backoffRetryCount++) @@ -149,6 +170,14 @@ private TimeSpan GetWaitTime(int retryCount, HttpResponseMessage? response) return timeToWait; } + /// + /// Determines if there is time left for a retry. + /// + /// The start time of the original request. + /// The current retry count. + /// The response message that potentially contains RetryAfter header. + /// The wait time for the next retry. + /// True if there is time left for a retry, false otherwise. private bool HasTimeForRetry(DateTimeOffset start, int retryCount, HttpResponseMessage? response, out TimeSpan waitFor) { waitFor = this.GetWaitTime(retryCount, response); @@ -163,8 +192,13 @@ private bool ShouldRetry(HttpStatusCode statusCode) return this._config.RetryableStatusCodes.Contains(statusCode); } - private bool ShouldRetry(Exception exception) + private bool ShouldRetry(Exception? exception) { + if (exception == null) + { + return false; + } + return this._config.RetryableExceptionTypes.Contains(exception.GetType()); } @@ -177,7 +211,7 @@ private bool ShouldRetry(Exception exception) /// /// Re-issue a new HTTP request with the previous request's headers and properties /// - internal static async Task CloneAsync(HttpRequestMessage originalRequest) + private static async Task CloneAsync(HttpRequestMessage originalRequest) { var newRequest = new HttpRequestMessage(originalRequest.Method, originalRequest.RequestUri); @@ -225,6 +259,9 @@ await originalRequest.Content.ReadAsStreamAsync().ContinueWith(t => private readonly IDelayProvider _delayProvider; private readonly ITimeProvider _timeProvider; + /// + /// Interface for a delay provider, primarily to enable unit testing. + /// internal interface IDelayProvider { Task DelayAsync(TimeSpan delay, CancellationToken cancellationToken); @@ -238,6 +275,9 @@ public async Task DelayAsync(TimeSpan delay, CancellationToken cancellationToken } } + /// + /// Interface for a time provider, primarily to enable unit testing. + /// internal interface ITimeProvider { DateTimeOffset GetCurrentTime(); From 2f6a82b566955ea9a79260febce46b1193dd46cd Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Mon, 13 Mar 2023 11:33:08 -0700 Subject: [PATCH 10/22] Add test case for retry handler with max total delay Summary: This commit adds a new unit test for the DefaultHttpRetryHandler class, which verifies that the retry logic respects the max total delay configuration when encountering exceptions. The test uses mock time and delay providers to simulate different scenarios and assert the expected behavior. The commit also removes some redundant comments from other tests. --- .../DefaultHttpRetryHandlerTests.cs | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs index 4a0f6210d567..89a1f069e8ed 100644 --- a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs +++ b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs @@ -461,7 +461,7 @@ public async Task ItRetriesWithMaxTotalDelayAsync(HttpStatusCode statusCode) var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); // Assert - mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(6)); // one for the initial call, and one for each of 5 attempts + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(6)); mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(50), It.IsAny()), Times.Exactly(5)); mockHandler.Protected() .Verify>("SendAsync", Times.Exactly(6), ItExpr.IsAny(), ItExpr.IsAny()); @@ -507,13 +507,55 @@ public async Task ItRetriesFewerWithMaxTotalDelayAsync() var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); // Assert - mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(3)); // one for the initial call, and one for each of 5 attempts + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(3)); mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(50), It.IsAny()), Times.Exactly(1)); mockHandler.Protected() .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); Assert.Equal(HttpStatusCode.TooManyRequests, response.StatusCode); } + [Fact] + public async Task ItRetriesFewerWithMaxTotalDelayOnExceptionAsync() + { + // Arrange + var HttpRetryConfig = new HttpRetryConfig + { + MaxRetryCount = 5, + MinRetryDelay = TimeSpan.FromMilliseconds(50), + MaxRetryDelay = TimeSpan.FromMilliseconds(50), + MaxTotalRetryTime = TimeSpan.FromMilliseconds(100) + }; + + var mockDelayProvider = new Mock(); + var mockTimeProvider = new Mock(); + + var currentTime = DateTimeOffset.UtcNow; + mockTimeProvider.SetupSequence(x => x.GetCurrentTime()) + .Returns(() => currentTime) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(5)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(55)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(110)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(165)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(220)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(275)) + .Returns(() => currentTime + TimeSpan.FromMilliseconds(330)); + + using var retry = ConfigureRetryHandler(HttpRetryConfig, mockTimeProvider, mockDelayProvider); + var mockHandler = GetHttpMessageHandlerMock(typeof(HttpRequestException)); + + retry.InnerHandler = mockHandler.Object; + using var httpClient = new HttpClient(retry); + + // Act + await Assert.ThrowsAsync(() => httpClient.GetAsync(new Uri("https://www.microsoft.com"), CancellationToken.None)); + + // Assert + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(3)); + mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(50), It.IsAny()), Times.Exactly(1)); + mockHandler.Protected() + .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); + } + [Fact] public async Task ItRetriesOnRetryableStatusCodesAsync() { From d1aba656c7d5d0ad837b76cdd8759d5d70915660 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Mon, 13 Mar 2023 19:28:29 -0700 Subject: [PATCH 11/22] only drain response if we aren't returning the response as-is --- .../Reliability/DefaultHttpRetryHandler.cs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index aba076c59c25..b31f3e749609 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -67,17 +67,6 @@ protected override async Task SendAsync(HttpRequestMessage return response; } - // Drain response content to free connections. Need to perform this - // before retry attempt and before re-throwing. - if (response.Content != null) - { -#if NET5_0_OR_GREATER - await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false); -#else - await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false); -#endif - } - reason = response.StatusCode.ToString(); // If the retry count is greater than the max retry count then we'll @@ -97,6 +86,17 @@ protected override async Task SendAsync(HttpRequestMessage "Error executing request, max total retry time reached. Reason: {0}", reason); return response; } + + // Drain response content to free connections. Need to perform this + // before retry attempt and before re-throwing. + if (response.Content != null) + { +#if NET5_0_OR_GREATER + await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false); +#else + await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false); +#endif + } } catch (Exception e) when (this.ShouldRetry(e) || this.ShouldRetry(e.InnerException)) { From eafebedb619e517e12b310befa5496caafe93e22 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Mon, 13 Mar 2023 20:31:22 -0700 Subject: [PATCH 12/22] Remove request cloning and content draining in DefaultHttpRetryHandler --- .../Reliability/DefaultHttpRetryHandler.cs | 76 +------------------ 1 file changed, 4 insertions(+), 72 deletions(-) diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index b31f3e749609..7d49b96f3f59 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. using System; -using System.IO; using System.Net; using System.Net.Http; using System.Threading; @@ -54,8 +53,8 @@ protected override async Task SendAsync(HttpRequestMessage { cancellationToken.ThrowIfCancellationRequested(); - TimeSpan waitFor = default; - string reason = string.Empty; + TimeSpan waitFor; + string reason; HttpResponseMessage? response = null; try { @@ -86,17 +85,6 @@ protected override async Task SendAsync(HttpRequestMessage "Error executing request, max total retry time reached. Reason: {0}", reason); return response; } - - // Drain response content to free connections. Need to perform this - // before retry attempt and before re-throwing. - if (response.Content != null) - { -#if NET5_0_OR_GREATER - await response.Content.ReadAsByteArrayAsync(cancellationToken).ConfigureAwait(false); -#else - await response.Content.ReadAsByteArrayAsync().ConfigureAwait(false); -#endif - } } catch (Exception e) when (this.ShouldRetry(e) || this.ShouldRetry(e.InnerException)) { @@ -123,15 +111,11 @@ protected override async Task SendAsync(HttpRequestMessage reason, waitFor.TotalMilliseconds); - // Clone request with CloneAsync before retrying - // Do not dispose this request as that breaks the request cloning -#pragma warning disable CA2000 - request = await CloneAsync(request); -#pragma warning restore CA2000 - // Increase retryCount retryCount++; + response?.Dispose(); + // Delay await this._delayProvider.DelayAsync(waitFor, cancellationToken).ConfigureAwait(false); } @@ -202,58 +186,6 @@ private bool ShouldRetry(Exception? exception) return this._config.RetryableExceptionTypes.Contains(exception.GetType()); } - /// - /// Create a new HTTP request by copying previous HTTP request's headers and properties from response's request message. - /// Copied from: https://github.com/microsoftgraph/msgraph-sdk-dotnet-core/blob/dev/src/Microsoft.Graph.Core/Extensions/HttpRequestMessageExtensions.cs - /// - /// The previous needs to be copy. - /// The . - /// - /// Re-issue a new HTTP request with the previous request's headers and properties - /// - private static async Task CloneAsync(HttpRequestMessage originalRequest) - { - var newRequest = new HttpRequestMessage(originalRequest.Method, originalRequest.RequestUri); - - // Copy request headers. - foreach (var header in originalRequest.Headers) - { - newRequest.Headers.TryAddWithoutValidation(header.Key, header.Value); - } - - // Copy request properties. - foreach (var property in originalRequest.Properties) - { - newRequest.Properties.Add(property); - } - - // Set Content if previous request had one. - if (originalRequest.Content != null) - { - // HttpClient doesn't rewind streams and we have to explicitly do so. - await originalRequest.Content.ReadAsStreamAsync().ContinueWith(t => - { - if (t.Result.CanSeek) - { - t.Result.Seek(0, SeekOrigin.Begin); - } - - newRequest.Content = new StreamContent(t.Result); - }, TaskScheduler.Current).ConfigureAwait(false); - - // Copy content headers. - if (originalRequest.Content.Headers != null) - { - foreach (var contentHeader in originalRequest.Content.Headers) - { - newRequest.Content.Headers.TryAddWithoutValidation(contentHeader.Key, contentHeader.Value); - } - } - } - - return newRequest; - } - private readonly HttpRetryConfig _config; private readonly ILogger _log; private readonly IDelayProvider _delayProvider; From 7a58ae2358e314d642c00bba5df3bd66eb53c2eb Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:28:26 -0700 Subject: [PATCH 13/22] Fix rebase build error --- .../SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs b/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs index c9ae6775e300..0ed855347140 100644 --- a/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs +++ b/dotnet/src/SemanticKernel.IntegrationTests/AI/OpenAICompletionTests.cs @@ -9,6 +9,7 @@ using Microsoft.SemanticKernel; using Microsoft.SemanticKernel.KernelExtensions; using Microsoft.SemanticKernel.Orchestration; +using Microsoft.SemanticKernel.Reliability; using SemanticKernel.IntegrationTests.TestSettings; using Xunit; using Xunit.Abstractions; From 23c5b397f54674b8df855f2a7ee8395b20dca1be Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:37:37 -0700 Subject: [PATCH 14/22] Refactor OpenAIClientAbstract constructor to accept handler factory --- .../SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dotnet/src/SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs b/dotnet/src/SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs index 022f35ce15f0..5e9df2d923ea 100644 --- a/dotnet/src/SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs +++ b/dotnet/src/SemanticKernel/AI/OpenAI/Clients/OpenAIClientAbstract.cs @@ -35,13 +35,13 @@ public abstract class OpenAIClientAbstract : IDisposable protected HttpClient HTTPClient { get; } private readonly HttpClientHandler _httpClientHandler; - private readonly IDelegatingHandlerFactory _handlerFactory = new DefaultHttpRetryHandlerFactory(); + private readonly IDelegatingHandlerFactory _handlerFactory; private readonly DelegatingHandler _retryHandler; internal OpenAIClientAbstract(ILogger? log = null, IDelegatingHandlerFactory? handlerFactory = null) { this.Log = log ?? this.Log; - this._handlerFactory = handlerFactory ?? this._handlerFactory; + this._handlerFactory = handlerFactory ?? new DefaultHttpRetryHandlerFactory(); this._httpClientHandler = new() { CheckCertificateRevocationList = true }; this._retryHandler = this._handlerFactory.Create(this.Log); From 5c74caa86a6f4d3714c48ef914c9575b31593cc7 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:38:42 -0700 Subject: [PATCH 15/22] format method signature --- .../AI/OpenAI/Services/AzureTextCompletion.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextCompletion.cs b/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextCompletion.cs index aa4cbff440d9..0eb7c96d0896 100644 --- a/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextCompletion.cs +++ b/dotnet/src/SemanticKernel/AI/OpenAI/Services/AzureTextCompletion.cs @@ -25,7 +25,12 @@ public sealed class AzureTextCompletion : AzureOpenAIClientAbstract, ITextComple /// Azure OpenAI API version, see https://learn.microsoft.com/azure/cognitive-services/openai/reference /// Application logger /// Retry handler factory for HTTP requests. - public AzureTextCompletion(string modelId, string endpoint, string apiKey, string apiVersion, ILogger? log = null, + public AzureTextCompletion( + string modelId, + string endpoint, + string apiKey, + string apiVersion, + ILogger? log = null, IDelegatingHandlerFactory? handlerFactory = null) : base(log, handlerFactory) { From 05a24f94d5c4b92666e5689ca79116df8125e8fd Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:46:22 -0700 Subject: [PATCH 16/22] Fix HTTP handler factory configuration in KernelConfig and KernelBuilder Summary: This commit fixes a bug where the HTTP handler factory set in the KernelConfig was not used by the KernelBuilder, and instead a default one was always created. It also makes the DefaultHttpRetryHandlerFactory constructor public, and adds some documentation for the KernelConfig properties. --- dotnet/src/SemanticKernel/Configuration/KernelConfig.cs | 6 +++++- dotnet/src/SemanticKernel/KernelBuilder.cs | 5 ++++- .../Reliability/DefaultHttpRetryHandlerFactory.cs | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs b/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs index 61c2c73ec2b7..1c73bf0821f2 100644 --- a/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs +++ b/dotnet/src/SemanticKernel/Configuration/KernelConfig.cs @@ -15,10 +15,13 @@ namespace Microsoft.SemanticKernel.Configuration; public sealed class KernelConfig { /// - /// Global retry logic used for all the backends http calls + /// Factory for creating HTTP handlers. /// public IDelegatingHandlerFactory HttpHandlerFactory { get; private set; } = new DefaultHttpRetryHandlerFactory(new HttpRetryConfig()); + /// + /// Default HTTP retry configuration for built-in HTTP handler factory. + /// public HttpRetryConfig DefaultHttpRetryConfig { get; private set; } = new(); /// @@ -201,6 +204,7 @@ public KernelConfig SetDefaultHttpRetryConfig(HttpRetryConfig? httpRetryConfig) if (httpRetryConfig != null) { this.DefaultHttpRetryConfig = httpRetryConfig; + this.SetHttpRetryHandlerFactory(new DefaultHttpRetryHandlerFactory(httpRetryConfig)); } return this; diff --git a/dotnet/src/SemanticKernel/KernelBuilder.cs b/dotnet/src/SemanticKernel/KernelBuilder.cs index 6c5a79fd6c33..b84307d09739 100644 --- a/dotnet/src/SemanticKernel/KernelBuilder.cs +++ b/dotnet/src/SemanticKernel/KernelBuilder.cs @@ -43,7 +43,10 @@ public static IKernel Create() /// Kernel instance public IKernel Build() { - this._config.SetHttpRetryHandlerFactory(this._httpHandlerFactory ?? new DefaultHttpRetryHandlerFactory(this._config.DefaultHttpRetryConfig)); + if (this._httpHandlerFactory != null) + { + this._config.SetHttpRetryHandlerFactory(this._httpHandlerFactory); + } var instance = new Kernel( new SkillCollection(this._log), diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs index 5b689b8ecd6b..3c457fda68b7 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandlerFactory.cs @@ -7,7 +7,7 @@ namespace Microsoft.SemanticKernel.Reliability; public class DefaultHttpRetryHandlerFactory : IDelegatingHandlerFactory { - internal DefaultHttpRetryHandlerFactory(HttpRetryConfig? config = null) + public DefaultHttpRetryHandlerFactory(HttpRetryConfig? config = null) { this._config = config; } From cb482161354504a935c84a0252fbf9c2a59a3751 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:47:27 -0700 Subject: [PATCH 17/22] xmldocs --- dotnet/src/SemanticKernel/KernelBuilder.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dotnet/src/SemanticKernel/KernelBuilder.cs b/dotnet/src/SemanticKernel/KernelBuilder.cs index b84307d09739..27aca197704e 100644 --- a/dotnet/src/SemanticKernel/KernelBuilder.cs +++ b/dotnet/src/SemanticKernel/KernelBuilder.cs @@ -116,6 +116,11 @@ public KernelBuilder WithMemoryStorageAndEmbeddingGenerator( return this; } + /// + /// Add a retry handler factory to the kernel to be built. + /// + /// Retry handler factory to add. + /// Updated kernel builder including the retry handler factory. public KernelBuilder WithRetryHandlerFactory(IDelegatingHandlerFactory httpHandlerFactory) { Verify.NotNull(httpHandlerFactory, "The retry handler factory instance provided is NULL"); From 989c7860d03a35121ae43125906da38e24bea8af Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:49:15 -0700 Subject: [PATCH 18/22] wrap signatures --- .../Reliability/DefaultHttpRetryHandler.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index 7d49b96f3f59..716c4bd10cfd 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -17,12 +17,15 @@ public sealed class DefaultHttpRetryHandler : DelegatingHandler /// /// The retry configuration. /// The logger. - public DefaultHttpRetryHandler(HttpRetryConfig? config = null, ILogger? log = null) : this(config ?? new HttpRetryConfig(), log, - null, null) + public DefaultHttpRetryHandler(HttpRetryConfig? config = null, ILogger? log = null) + : this(config ?? new HttpRetryConfig(), log, null, null) { } - internal DefaultHttpRetryHandler(HttpRetryConfig config, ILogger? log = null, IDelayProvider? delayProvider = null, + internal DefaultHttpRetryHandler( + HttpRetryConfig config, + ILogger? log = null, + IDelayProvider? delayProvider = null, ITimeProvider? timeProvider = null) { this._config = config; From ec2e74663fce926a6251a2e996122bd8a6a1654e Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:53:01 -0700 Subject: [PATCH 19/22] Add time spent to error logging in DefaultHttpRetryHandler Summary: This commit adds the time spent on the request to the error logging messages in DefaultHttpRetryHandler, when the maximum total retry time is reached. This helps to diagnose the cause of the error and the performance of the retry policy. The commit also updates the unit tests to verify the expected number of calls to the time provider mock. --- .../Reliability/DefaultHttpRetryHandlerTests.cs | 4 ++-- .../src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs index 89a1f069e8ed..3b3fcddc6bca 100644 --- a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs +++ b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs @@ -507,7 +507,7 @@ public async Task ItRetriesFewerWithMaxTotalDelayAsync() var response = await httpClient.PostAsync(new Uri("https://www.microsoft.com"), testContent, CancellationToken.None); // Assert - mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(3)); + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(4)); // 1 intial, 2 retries, 1 for logging time taken. mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(50), It.IsAny()), Times.Exactly(1)); mockHandler.Protected() .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); @@ -550,7 +550,7 @@ public async Task ItRetriesFewerWithMaxTotalDelayOnExceptionAsync() await Assert.ThrowsAsync(() => httpClient.GetAsync(new Uri("https://www.microsoft.com"), CancellationToken.None)); // Assert - mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(3)); + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(4)); // 1 intial, 2 retries, 1 for logging time taken. mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(50), It.IsAny()), Times.Exactly(1)); mockHandler.Protected() .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index 716c4bd10cfd..6f1e5fb22da0 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -85,7 +85,7 @@ protected override async Task SendAsync(HttpRequestMessage if (!this.HasTimeForRetry(start, retryCount, response, out waitFor)) { this._log.LogError( - "Error executing request, max total retry time reached. Reason: {0}", reason); + "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}", reason, this._timeProvider.GetCurrentTime() - start); return response; } } @@ -101,7 +101,7 @@ protected override async Task SendAsync(HttpRequestMessage else if (!this.HasTimeForRetry(start, retryCount, response, out waitFor)) { this._log.LogError(e, - "Error executing request, max total retry time reached. Reason: {0}", reason); + "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}", reason, this._timeProvider.GetCurrentTime() - start); throw; } } From 881730ba8e244bfab73fb228901de8fa10d51a85 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:53:24 -0700 Subject: [PATCH 20/22] Rename SemanticKernel.Test project to SemanticKernel.UnitTests Summary: This commit renames the SemanticKernel.Test project to SemanticKernel.UnitTests, to follow the naming convention of other test projects in the solution. It also updates the references to the project in the .vscode/tasks.json file, to ensure that the test tasks work correctly. This change does not affect the functionality or behavior of the tests, only the project name. --- .vscode/tasks.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.vscode/tasks.json b/.vscode/tasks.json index d2db660cf8d3..c8b65d365445 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -103,7 +103,7 @@ "label": "Test - Semantic-Kernel", "command": "dotnet", "type": "process", - "args": ["test", "SemanticKernel.Test.csproj"], + "args": ["test", "SemanticKernel.UnitTests.csproj"], "problemMatcher": "$msCompile", "group": "test", "presentation": { @@ -112,7 +112,7 @@ "group": "PR-Validate" }, "options": { - "cwd": "${workspaceFolder}/dotnet/src/SemanticKernel.Test/" + "cwd": "${workspaceFolder}/dotnet/src/SemanticKernel.UnitTests/" } }, { @@ -123,7 +123,7 @@ "test", "--collect", "XPlat Code Coverage;Format=lcov", - "SemanticKernel.Test.csproj" + "SemanticKernel.UnitTests.csproj" ], "problemMatcher": "$msCompile", "group": "test", @@ -132,7 +132,7 @@ "panel": "shared" }, "options": { - "cwd": "${workspaceFolder}/dotnet/src/SemanticKernel.Test/" + "cwd": "${workspaceFolder}/dotnet/src/SemanticKernel.UnitTests/" } }, { From 05476805ab4d6180edc85d9b72901c1c9d59c4ab Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 12:56:38 -0700 Subject: [PATCH 21/22] formatting --- .../Reliability/DefaultHttpRetryHandlerTests.cs | 2 +- .../SemanticKernel/Reliability/DefaultHttpRetryHandler.cs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs index 3b3fcddc6bca..fb5c62120de3 100644 --- a/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs +++ b/dotnet/src/SemanticKernel.UnitTests/Reliability/DefaultHttpRetryHandlerTests.cs @@ -550,7 +550,7 @@ public async Task ItRetriesFewerWithMaxTotalDelayOnExceptionAsync() await Assert.ThrowsAsync(() => httpClient.GetAsync(new Uri("https://www.microsoft.com"), CancellationToken.None)); // Assert - mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(4)); // 1 intial, 2 retries, 1 for logging time taken. + mockTimeProvider.Verify(x => x.GetCurrentTime(), Times.Exactly(4)); // 1 intial, 2 retries, 1 for logging time taken. mockDelayProvider.Verify(x => x.DelayAsync(TimeSpan.FromMilliseconds(50), It.IsAny()), Times.Exactly(1)); mockHandler.Protected() .Verify>("SendAsync", Times.Exactly(2), ItExpr.IsAny(), ItExpr.IsAny()); diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index 6f1e5fb22da0..ca917adf7db6 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -85,7 +85,8 @@ protected override async Task SendAsync(HttpRequestMessage if (!this.HasTimeForRetry(start, retryCount, response, out waitFor)) { this._log.LogError( - "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}", reason, this._timeProvider.GetCurrentTime() - start); + "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}", reason, + this._timeProvider.GetCurrentTime() - start); return response; } } @@ -101,7 +102,8 @@ protected override async Task SendAsync(HttpRequestMessage else if (!this.HasTimeForRetry(start, retryCount, response, out waitFor)) { this._log.LogError(e, - "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}", reason, this._timeProvider.GetCurrentTime() - start); + "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}", reason, + this._timeProvider.GetCurrentTime() - start); throw; } } From cfb83b3e785ee9db09dfa1c698256d5853e1d683 Mon Sep 17 00:00:00 2001 From: "lemiller@microsoft.com" Date: Tue, 14 Mar 2023 13:07:38 -0700 Subject: [PATCH 22/22] fix time formatting in logs --- .../Reliability/DefaultHttpRetryHandler.cs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs index ca917adf7db6..c921d6870b6c 100644 --- a/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs +++ b/dotnet/src/SemanticKernel/Reliability/DefaultHttpRetryHandler.cs @@ -84,9 +84,10 @@ protected override async Task SendAsync(HttpRequestMessage // just return if (!this.HasTimeForRetry(start, retryCount, response, out waitFor)) { + var timeTaken = this._timeProvider.GetCurrentTime() - start; this._log.LogError( - "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}", reason, - this._timeProvider.GetCurrentTime() - start); + "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}ms", reason, + timeTaken.TotalMilliseconds); return response; } } @@ -101,9 +102,10 @@ protected override async Task SendAsync(HttpRequestMessage } else if (!this.HasTimeForRetry(start, retryCount, response, out waitFor)) { - this._log.LogError(e, - "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}", reason, - this._timeProvider.GetCurrentTime() - start); + var timeTaken = this._timeProvider.GetCurrentTime() - start; + this._log.LogError( + "Error executing request, max total retry time reached. Reason: {0}. Time spent: {1}ms", reason, + timeTaken.TotalMilliseconds); throw; } }