-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add target based scaling support for Netherite #265
Conversation
I have spent some time to modify the logic so it now computes a target instead of a scale recommendation. It still has the same overall structure but I had to use the data provided by the metrics to determine the current scale. |
Thanks Sebastian! I updated the PR and tested your changes locally. |
@sebastianburckhardt, can you point me to where I should add unit tests? |
I think the best place to add the tests is the DurableTask.Netherite.Tests project. The other tests in there are mostly end-to-end tests that use some special fixture. In your case, just plain normal unit tests are probably enough, so I don't think you need to use any of those. |
test/DurableTask.Netherite.AzureFunctions.Tests/TargetBasedScalingTests.cs
Outdated
Show resolved
Hide resolved
test/DurableTask.Netherite.AzureFunctions.Tests/TargetBasedScalingTests.cs
Outdated
Show resolved
Hide resolved
Is this PR still needed? |
I believe we've parked this for the time being. Right @bachuv? I suspect this may become relevant again once Flex Consumption enter it's GA-prep phase, but we're not there yet. @sebastianburckhardt: is it a problem if this remains open, just perhaps in draft mode? |
@davidmrdavid, yes that's right. |
I've taken the liberty of marking this as draft to help signal that it doesn't need active review, and is simply an idle PR meant for future completion. Hope that's ok! |
Not to be too obvious, but I guess this is why we were often peppered with log entries warning that target-based scaling could not be used. The log entries are quite noisy. |
Hey @ericleigh007 - do you know if your app somehow "opted in" to target based scaling? If you're getting these warnings without opting in, that could be a problem (and possibly a reason to re-prioritize this work) |
I was under the impression that target-based scaling was the default, and you had to opt-out. So we have done nothing specifically for this. |
src/DurableTask.Netherite/OrchestrationService/NetheriteOrchestrationService.cs
Show resolved
Hide resolved
...DurableTask.Netherite.AzureFunctions.Tests/DurableTask.Netherite.AzureFunctions.Tests.csproj
Outdated
Show resolved
Hide resolved
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.
PR looks good to me, though I think we need more unit tests. I will take a look at what to test.
test/DurableTask.Netherite.Tests/DurableTask.Netherite.Tests.csproj
Outdated
Show resolved
Hide resolved
test/DurableTask.Netherite.AzureFunctions.Tests/TargetBasedScalingTests.cs
Outdated
Show resolved
Hide resolved
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.
found one more thing to fix, otherwise looks good.
string connectionName, | ||
out ITargetScaler targetScaler) | ||
{ | ||
ILoadPublisherService loadPublisher = this.Service.GetLoadPublisher(); |
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.
This should be inside the conditional below, I think.
// Target Scaler is created per function id. And they share the same NetheriteMetricsProvider. | ||
if ( this.metricsProvider == null) | ||
{ | ||
this.loadPublisher ??= this.Service.GetLoadPublisher(); |
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.
Make loadPublisher class property too like metricsProvider so that it can be shared among scalers.
This PR adds support for target based scaling. It builds upon Azure/azure-functions-durable-extension#2452. This PR is in draft mode since it hasn't been tested E2E yet.
Changes:
NetheriteMetricsProvider
and moveCollectMetrics
code from ScaleMonitor hereNetheriteTargetScaler
(implementsITargetScaler
)TryGetTargetScaler
in NetheriteProvider