-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Singleton Metrics Provider for Listener #285
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a couple more changes.
src/DurableTask.SqlServer.AzureFunctions/SqlDurabilityProvider.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.SqlServer.AzureFunctions/SqlDurabilityProvider.cs
Outdated
Show resolved
Hide resolved
{ | ||
this.sqlMetricsProvider = sqlMetricsProvider; | ||
|
||
// Scalers in Durable Functions are shared for all functions in the same task hub. | ||
// So instead of using a function ID, we use the task hub name as the basis for the descriptor ID. | ||
string id = $"DurableTask-SqlServer:{taskHubName ?? "default"}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
// Scalers in Durable Functions are shared for all functions in the same task hub. | ||
// So instead of using a function ID, we use the task hub name as the basis for the descriptor ID. | ||
// Scalers in Durable Functions is per function ids. And scalers share the same sqlMetricsProvider in the same taskhub. | ||
string id = $"DurableTask-SqlServer:{taskHubName ?? "default"}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string id = $"DurableTask-SqlServer:{taskHubName ?? "default"}"; | |
string id = $"{functionId}-DurableTask-SqlServer:{taskHubName ?? "default"}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! does this string id needs to add ToLower(CultureInfo.InvariantCulture) to be lower case?
{ | ||
var sqlMetricsProvider = new SqlMetricsProvider(this.service); | ||
this.scaleMonitor = new SqlScaleMonitor(hubName, sqlMetricsProvider); | ||
this.singletonSqlMetricsProvider = new SqlMetricsProvider(this.service); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing. I recall the final fix that Alexey made to the Azure Storage metrics provider was to throttle it so that we only fetch metrics once every few seconds or so. This is how we ensured that we don't have too many storage transactions, even if the app has a lot of triggers. I think it will be important to do that for the SqlMetricsProvider
too so that we don't overwhelm the customer's database with expensive SQL queries.
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
public class SqlMetricsProvider | ||
{ | ||
readonly SqlOrchestrationService service; | ||
DateTime metricsTimeStamp = DateTime.MinValue; | ||
SqlScaleMetric? metrics; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this cache. If the target scaler calls GetMetricsAsync
within 5 seconds, return the last result to make sure the target scaler remains the same decision as last time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Similar changes at AzureStorage backend Azure/azure-functions-durable-extension@c5d8e04#diff-09935cd15a9926e82e0d7ff3df8d480813cabd51b8e3a483985ec5a0da3035cd