Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Conversation

pierre-sk
Copy link
Contributor

@pierre-sk pierre-sk commented Aug 7, 2023

Hello everyone,

The KafkaTargetScaler is using the obsolete ScaleMonitorDescriptor constructor without the functionId parameter.

[Obsolete("This constructor is obsolete. Use the version that takes function id instead.")]
public ScaleMonitorDescriptor(string id)
{
	Id = id;
}

public ScaleMonitorDescriptor(string id, string functionId) : this(id)
{
	FunctionId = functionId;
}

When target scaling is enabled (by default) the ScaleManager compares the FunctionId to evaluate if a ScaleMonitor is used in case a TargetScaler already exists. But for the same function, the FunctionId of the TargetScaler and ScaleMonitor are not equal.

// Check if TBS enabled on app level
if (scaleOptions.Value.IsTargetScalingEnabled)
{
	HashSet<string> targetScalerFunctions = new HashSet<string>();
	foreach (var scaler in targetScalers)
	{
		string scalerUniqueId = GetTargetScalerFunctionUniqueId(scaler);
		if (!_targetScalersInError.Contains(scalerUniqueId))
		{
			string assemblyName = GetAssemblyName(scaler.GetType());
			bool featureDisabled = configuration.GetValue<string>(assemblyName) == "0";
			if (!featureDisabled)
			{
				targetScalersToSample.Add(scaler);
				targetScalerFunctions.Add(scalerUniqueId);
			}
		}
	}

	foreach (var monitor in scaleMonitors)
	{
		string monitorUniqueId = GetScaleMonitorFunctionUniqueId(monitor);
		// Check if target based scaler exists for the function
		if (!targetScalerFunctions.Contains(monitorUniqueId))
		{
			scaleMonitorsToSample.Add(monitor);
		}
	}
}

Both ScaleMonitor and TargetScaler are retrieved for sampling. It leads to a bad behavior as the ScaleManager doesn't use the TargetScaler to override the vote result when there is any ScaleMonitor status.

// Set correct vote if all the triggers are target
if (!scaleStatuses.Any() && aggregateScaleStatus.TargetWorkerCount.HasValue)
{
    aggregateScaleStatus.Vote = (ScaleVote)aggregateScaleStatus.TargetWorkerCount.Value.CompareTo(context.WorkerCount);
}

Example of a bad result on /admin/host/scale/status function runtime endpoint:

{
    "vote": 0,
    "targetWorkerCount": 4,
    "functionScaleStatuses": {
        "my.functions.myfirstfunction.runasync-kafkatrigger-topic.myconsumer": {
            "vote": 0
        },
        "my.functions.mysecondfunction.runasync-kafkatrigger-topic.myconsumer": {
            "vote": 0
        }
    },
    "functionTargetScalerResults": {
        "My.Functions.MyFirstFunction.RunAsync": {
            "targetWorkerCount": 4
        },
        "My.Functions.MySecondFunction.RunAsync": {
            "targetWorkerCount": 1
        }
    }
}

@pierre-sk
Copy link
Contributor Author

@microsoft-github-policy-service agree

@a99cl208
Copy link

Hello @jainharsh98,

Could you please take a look to this PR because as mentionned the target scaler does not work at all for now.

Regards.

@pierre-sk
Copy link
Contributor Author

Hello @jainharsh98,

Any feedback for this simple fix? It prevents us to switch to target scaler, we are blocked for nearly 1 year.

Thank you in advance.

Best regards

Copy link
Contributor

@jainharsh98 jainharsh98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jainharsh98 jainharsh98 merged commit dd44069 into Azure:dev Jul 25, 2024
jainharsh98 added a commit that referenced this pull request Jul 29, 2024
Co-authored-by: jainharsh98 <jainh@microsoft.com>
jainharsh98 added a commit that referenced this pull request Jul 30, 2024
* Changing all references to net8.0 and build errors

* Fixing end to end tests for confluent

* Changes to make Lang E22E tests succeed

* Install .net8 sdk in the build pipeline

* removing confluent sdk dependency on e2e tests

* Upgrading confluent package to 2.4.0(#504)

* Fix scale monitor descriptor function id (#451)

Co-authored-by: jainharsh98 <jainh@microsoft.com>

* SslKeyPassword resolution as secret setting for output binding (#464)

Co-authored-by: Gregory van de Wiele <Gregory.vandeWiele@eneco.com>
Co-authored-by: jainharsh98 <jainh@microsoft.com>

* Added OAuthBearer authentication mode (#437)

Co-authored-by: Hakan Altinbasak <hakan.altinbasak@avanade.com>
Co-authored-by: jainharsh98 <jainh@microsoft.com>

* Fixing Msg commit for Cancellation Scenarios (#505)

* Commiting msg in single mode if cancellation is not requested.

* Removing stray space character

* Removing checks for specific and generic records to allow for other data types for out of proc (#506)

* Fix for schema registry and UTs (#507)

* Updating extension version to 4.0.0

* Resolving Oauthbearer Authentication Attributes from Settings for Output Binding (#511)

* Resolving secure settings from settings for oauthbearer authentication

* Adding Unit Test cases for oauthbearer settings

---------

Co-authored-by: Vivek Jilla <vijilla@microsoft.com>
Co-authored-by: Vivek Jilla <vivekjilla7@gmail.com>
Co-authored-by: Pierre Sk <17861564+pierre-sk@users.noreply.github.com>
Co-authored-by: gvdwiele <22445+gvdwiele@users.noreply.github.com>
Co-authored-by: Gregory van de Wiele <Gregory.vandeWiele@eneco.com>
Co-authored-by: Hakan <hakan.altinbasak@gmail.com>
Co-authored-by: Hakan Altinbasak <hakan.altinbasak@avanade.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.