Introduce a Lightweight Scaling Package for Multi Durable Backends #3240
Conversation
src/WebJobs.Extensions.DurableTask.Scale/DurableTaskJobHostConfigurationExtensions.cs
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask.Scale/AzureStorage/AzureStorageOptions.cs
Outdated
Show resolved
Hide resolved
| int maxConcurrentEntitiesDefault = this.inConsumption ? 10 : 10 * Environment.ProcessorCount; | ||
| int maxEntityOperationBatchSizeDefault = this.inConsumption ? 50 : 5000; | ||
|
|
||
| if (this.inConsumption) |
There was a problem hiding this comment.
I wonder if we should keep the logic from line 72 to line 92. Cause when I checked the TriggerMetadata json payload from SC I didn't find info about runtime. Also I think this basically is not related with scaling?
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new WebJobs.Extensions.DurableTask.Scale package that separates scaling-related functionality from the main Durable Task extension. The package provides autoscaling capabilities for Durable Functions using Azure Storage as the backend provider.
Key Changes:
- Created a new independent Scale package with minimal dependencies (using System.Text.Json instead of Newtonsoft.Json where possible)
- Implemented scale monitoring and target-based scaling for Durable Task triggers
- Added Azure Storage-specific durability provider with metrics collection and scaling logic
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 33 comments.
Show a summary per file
| File | Description |
|---|---|
| WebJobs.Extensions.DurableTask.Scale.csproj | New project file defining the Scale package with net6.0 target and necessary dependencies |
| ScaleUtils.cs | Utility class for creating scale monitors and target scalers with no-op implementations |
| IStorageServiceClientProviderFactory.cs | Interface for retrieving Azure Storage service client providers |
| IDurabilityProviderFactory.cs | Interface for building durability provider instances |
| FunctionName.cs | Value type representing a durable function name with equality semantics |
| DurableTaskTriggersScaleProvider.cs | Main provider implementing scale monitoring and target scaling for triggers |
| DurableTaskScaleExtension.cs | Extension class for registering Durable Task scaling configuration |
| DurableTaskOptions.cs | Minimal options class containing settings for scaling decisions |
| DurableTaskJobHostConfigurationExtensions.cs | Extension methods for registering Durable Task with WebJobs builder |
| DurableClientAttribute.cs | Attribute for binding function parameters to Durable clients |
| DurabilityProvider.cs | Base class for backend storage providers with scaling support |
| NameValidator.cs | Utility for validating Azure Storage resource names |
| DurableTaskTriggerMetrics.cs | Metrics class for scale monitoring |
| DurableTaskTargetScaler.cs | Implementation of target-based scaling logic |
| DurableTaskScaleMonitor.cs | Implementation of scale monitoring |
| DurableTaskMetricsProvider.cs | Provider for collecting metrics from Azure Storage |
| AzureStorageOptions.cs | Configuration options for Azure Storage provider |
| AzureStorageDurabilityProviderFactory.cs | Factory for creating Azure Storage durability providers |
| AzureStorageDurabilityProvider.cs | Azure Storage implementation of durability provider |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private readonly IDurabilityProviderFactory durabilityProviderFactory; | ||
| private readonly DurabilityProvider defaultDurabilityProvider; | ||
| private readonly DurableTaskOptions options; | ||
| private readonly ILogger logger; |
There was a problem hiding this comment.
Inconsistent indentation: This line uses tabs while surrounding lines use tabs with different alignment. The entire file should use consistent indentation (either spaces or tabs, with consistent alignment).
| DurableTaskOptions options, | ||
| ILogger logger, | ||
| IEnumerable<IDurabilityProviderFactory> durabilityProviderFactories) |
There was a problem hiding this comment.
Inconsistent indentation in parameter list: Parameters are indented differently (varying tab/space combinations). All parameters should have consistent indentation.
| DurableTaskOptions options, | |
| ILogger logger, | |
| IEnumerable<IDurabilityProviderFactory> durabilityProviderFactories) | |
| DurableTaskOptions options, | |
| ILogger logger, | |
| IEnumerable<IDurabilityProviderFactory> durabilityProviderFactories) |
| { | ||
| this.options = options ?? throw new ArgumentNullException(nameof(options)); | ||
| this.logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
| this.durabilityProviderFactories = durabilityProviderFactories ?? throw new ArgumentNullException(nameof(durabilityProviderFactories)); | ||
|
|
||
| this.durabilityProviderFactory = GetDurabilityProviderFactory(this.options, this.logger, this.durabilityProviderFactories); | ||
| this.defaultDurabilityProvider = this.durabilityProviderFactory.GetDurabilityProvider(); | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation in constructor body: The method body uses excessive and inconsistent indentation with mixed tabs. Should use consistent indentation matching the rest of the codebase.
| { | |
| this.options = options ?? throw new ArgumentNullException(nameof(options)); | |
| this.logger = logger ?? throw new ArgumentNullException(nameof(logger)); | |
| this.durabilityProviderFactories = durabilityProviderFactories ?? throw new ArgumentNullException(nameof(durabilityProviderFactories)); | |
| this.durabilityProviderFactory = GetDurabilityProviderFactory(this.options, this.logger, this.durabilityProviderFactories); | |
| this.defaultDurabilityProvider = this.durabilityProviderFactory.GetDurabilityProvider(); | |
| } | |
| { | |
| this.options = options ?? throw new ArgumentNullException(nameof(options)); | |
| this.logger = logger ?? throw new ArgumentNullException(nameof(logger)); | |
| this.durabilityProviderFactories = durabilityProviderFactories ?? throw new ArgumentNullException(nameof(durabilityProviderFactories)); | |
| this.durabilityProviderFactory = GetDurabilityProviderFactory(this.options, this.logger, this.durabilityProviderFactories); | |
| this.defaultDurabilityProvider = this.durabilityProviderFactory.GetDurabilityProvider(); | |
| } |
| private static IDurabilityProviderFactory GetDurabilityProviderFactory( | ||
| DurableTaskOptions options, | ||
| ILogger logger, | ||
| IEnumerable<IDurabilityProviderFactory> durabilityProviderFactories) | ||
| { |
There was a problem hiding this comment.
Inconsistent indentation: Method signature and body use mixed tabs with varying alignment. Should use consistent indentation throughout.
| MaxConcurrentTaskActivityWorkItems = this.options.MaxConcurrentActivityFunctions ?? throw new InvalidOperationException($"{nameof(this.options.MaxConcurrentOrchestratorFunctions)} needs a default value"), | ||
| MaxConcurrentTaskEntityWorkItems = this.options.MaxConcurrentEntityFunctions ?? throw new InvalidOperationException($"{nameof(this.options.MaxConcurrentEntityFunctions)} needs a default value"), | ||
| ExtendedSessionsEnabled = this.options.ExtendedSessionsEnabled, | ||
| ExtendedSessionIdleTimeout = extendedSessionTimeout, |
There was a problem hiding this comment.
Undefined variable 'extendedSessionTimeout' is used but never declared. This will cause a compilation error.
| if (metrics[i].ControlQueueLengths == null) | ||
| { | ||
| heartbeats[i].ControlQueueLengths = new List<int>(); | ||
| } | ||
| else | ||
| { | ||
| heartbeats[i].ControlQueueLengths = JsonConvert.DeserializeObject<IReadOnlyList<int>>(metrics[i].ControlQueueLengths); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (metrics[i].ControlQueueLengths == null) | |
| { | |
| heartbeats[i].ControlQueueLengths = new List<int>(); | |
| } | |
| else | |
| { | |
| heartbeats[i].ControlQueueLengths = JsonConvert.DeserializeObject<IReadOnlyList<int>>(metrics[i].ControlQueueLengths); | |
| } | |
| heartbeats[i].ControlQueueLengths = | |
| metrics[i].ControlQueueLengths == null | |
| ? new List<int>() | |
| : JsonConvert.DeserializeObject<IReadOnlyList<int>>(metrics[i].ControlQueueLengths); |
| if (metrics[i].ControlQueueLatencies == null) | ||
| { | ||
| heartbeats[i].ControlQueueLatencies = new List<TimeSpan>(); | ||
| } | ||
| else | ||
| { | ||
| heartbeats[i].ControlQueueLatencies = JsonConvert.DeserializeObject<IReadOnlyList<TimeSpan>>(metrics[i].ControlQueueLatencies); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (metrics[i].ControlQueueLatencies == null) | |
| { | |
| heartbeats[i].ControlQueueLatencies = new List<TimeSpan>(); | |
| } | |
| else | |
| { | |
| heartbeats[i].ControlQueueLatencies = JsonConvert.DeserializeObject<IReadOnlyList<TimeSpan>>(metrics[i].ControlQueueLatencies); | |
| } | |
| heartbeats[i].ControlQueueLatencies = metrics[i].ControlQueueLatencies == null | |
| ? new List<TimeSpan>() | |
| : JsonConvert.DeserializeObject<IReadOnlyList<TimeSpan>>(metrics[i].ControlQueueLatencies); |
| if (string.Equals(durabilityProviderFactory.Name, AzureManagedProviderName, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| defaultDurabilityProvider = durabilityProviderFactory.GetDurabilityProvider(attribute: null, triggerMetadata); | ||
| } | ||
| else | ||
| { | ||
| defaultDurabilityProvider = durabilityProviderFactory.GetDurabilityProvider(); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| if (string.Equals(durabilityProviderFactory.Name, AzureManagedProviderName, StringComparison.OrdinalIgnoreCase)) | |
| { | |
| defaultDurabilityProvider = durabilityProviderFactory.GetDurabilityProvider(attribute: null, triggerMetadata); | |
| } | |
| else | |
| { | |
| defaultDurabilityProvider = durabilityProviderFactory.GetDurabilityProvider(); | |
| } | |
| defaultDurabilityProvider = string.Equals(durabilityProviderFactory.Name, AzureManagedProviderName, StringComparison.OrdinalIgnoreCase) | |
| ? durabilityProviderFactory.GetDurabilityProvider(attribute: null, triggerMetadata) | |
| : durabilityProviderFactory.GetDurabilityProvider(); |
| else | ||
| { | ||
| // the durability provider does not support runtime scaling. | ||
| // Create an empty scale monitor to avoid exceptions (unless runtime scaling is actually turned on). | ||
| return new NoOpScaleMonitor($"{functionId}-DurableTaskTrigger-{hubName}".ToLower(), functionId); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
| else | |
| { | |
| // the durability provider does not support runtime scaling. | |
| // Create an empty scale monitor to avoid exceptions (unless runtime scaling is actually turned on). | |
| return new NoOpScaleMonitor($"{functionId}-DurableTaskTrigger-{hubName}".ToLower(), functionId); | |
| } | |
| // the durability provider does not support runtime scaling. | |
| // Create an empty scale monitor to avoid exceptions (unless runtime scaling is actually turned on). | |
| return new NoOpScaleMonitor($"{functionId}-DurableTaskTrigger-{hubName}".ToLower(), functionId); |
| if (durabilityProvider.TryGetTargetScaler( | ||
| functionId, | ||
| functionName.Name, | ||
| hubName, | ||
| connectionName, | ||
| out ITargetScaler targetScaler)) | ||
| { | ||
| return targetScaler; | ||
| } | ||
| else | ||
| { | ||
| // the durability provider does not support target-based scaling. | ||
| // Create an empty target scaler to avoid exceptions (unless target-based scaling is actually turned on). | ||
| return new NoOpTargetScaler(functionId); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
| if (durabilityProvider.TryGetTargetScaler( | |
| functionId, | |
| functionName.Name, | |
| hubName, | |
| connectionName, | |
| out ITargetScaler targetScaler)) | |
| { | |
| return targetScaler; | |
| } | |
| else | |
| { | |
| // the durability provider does not support target-based scaling. | |
| // Create an empty target scaler to avoid exceptions (unless target-based scaling is actually turned on). | |
| return new NoOpTargetScaler(functionId); | |
| } | |
| // the durability provider does not support target-based scaling. | |
| // Create an empty target scaler to avoid exceptions (unless target-based scaling is actually turned on). | |
| return durabilityProvider.TryGetTargetScaler( | |
| functionId, | |
| functionName.Name, | |
| hubName, | |
| connectionName, | |
| out ITargetScaler targetScaler) | |
| ? targetScaler | |
| : new NoOpTargetScaler(functionId); |
src/WebJobs.Extensions.DurableTask.Scale/IStorageServiceClientProviderFactory.cs
Outdated
Show resolved
Hide resolved
cgillum
left a comment
cgillum
left a comment
There was a problem hiding this comment.
I see that this is all new code and much of it is duplicated, which means that if we need to make a fix to it, we need to make the same fix in the other copy of the code. Is that right? Have you instead considered whether we should refactor the extension code so that we have one scale implementation that's used by both the extension and the scale controller?
cgillum
left a comment
cgillum
left a comment
There was a problem hiding this comment.
Added some comments. I wonder if we should try to make this package even smaller, with fewer dependencies copied over. I think the current approach will be hard to maintain.
src/WebJobs.Extensions.DurableTask.Scale/DurableTaskJobHostConfigurationExtensions.cs
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask.Scale/IStorageServiceClientProviderFactory.cs
Outdated
Show resolved
Hide resolved
cgillum
left a comment
cgillum
left a comment
There was a problem hiding this comment.
A few initial questions to help me better understand the intent of the design.
| string functionId, | ||
| string functionName, | ||
| string hubName, | ||
| string connectionName, |
There was a problem hiding this comment.
@nytian assuming we need both of these connection names, which connectionName takes precendence? We should change the names of both these parameters and the instance member so that we know what their purposes are. For example, one can be defaultConnectionName and the other can be connectionNameOverride.
src/WebJobs.Extensions.DurableTask.Scale/AzureStorage/AzureStorageScalabilityProvider.cs
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask.Scale/AzureStorage/AzureStorageScalabilityProviderFactory.cs
Show resolved
Hide resolved
src/WebJobs.Extensions.DurableTask.Scale/AzureStorage/AzureStorageScalabilityProviderFactory.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (this.defaultStorageProvider == null) | ||
| { | ||
| ILogger logger = this.loggerFactory.CreateLogger(LoggerName); |
There was a problem hiding this comment.
Should we just create the logger once in the constructor and reuse it?
| // Create StorageAccountClientProvider without credential (connection string) | ||
| var storageAccountClientProvider = this.clientProviderFactory.GetClientProvider( | ||
| this.DefaultConnectionName, | ||
| tokenCredential: null); |
There was a problem hiding this comment.
Why can't we use a credential here?
…functions-durable-extension into nytian/sc-light-pkg
Issue describing the changes in this PR
Solve #3193
This PR introduces a new light-weight package for Functions Scaling use specifically as the current size of Webjobs.exntesions.durabletask and the new dependencies like Open Telemetry.
Right now it's a draft version for AzureStorage. MSSQL and DTS support will be added later.
Pull request checklist
pending_docs.mdrelease_notes.md/src/Worker.Extensions.DurableTask/AssemblyInfo.csdevandmainbranches and will not be merged into thev2.xbranch.