AAD: new AuthenticationTransmissionPolicy for retry scenarios.

[TimothyMothra]

Issue #2190

Changes

• new class: AuthenticationTransmissionPolicy with retry rules for AAD scenarios. Off by default.
• change to TransmissionPolicyCollection to enable AuthenticationTransmissionPolicy
• refactor Unit Test ThrottlingTransmissionPolicyTest. My new tests are based on this.
• new Unit Tests that verify AuthenticationTransmissionPolicy triggers throttling for only specific HTTP StatusCodes.

Note: Exponential backoff is removed from scope of this PR and will be addressed in a following PR.

Checklist

• I ran Unit Tests locally.
• CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

For significant contributions please make sure you have completed the following items:

• Design discussion issue #
• Changes in public surface reviewed

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

• FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

Notes for reviewers:

• We support comment build triggers
  • /AzurePipelines run will queue all builds
  • /AzurePipelines run <pipeline-name> will queue a specific build

[cijothomas]

i think we should do exponential backoffs, similar to how its used in other existing policies.

[rajkumar-rangaraj]

Do we have a recommendation from Azure Identity on the retry timeframe?

[pharring]

See: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/src/RetryOptions.cs

Exponential back-off. Maximum of 3 retries. Initial delay 0.8s; maximum delay 1 minute.

        /// <summary>
        /// The maximum number of retry attempts before giving up.
        /// </summary>
        public int MaxRetries { get; set; } = 3;

        /// <summary>
        /// The delay between retry attempts for a fixed approach or the delay
        /// on which to base calculations for a backoff-based approach.
        /// </summary>
        public TimeSpan Delay { get; set; } = TimeSpan.FromSeconds(0.8);

        /// <summary>
        /// The maximum permissible delay between retry attempts.
        /// </summary>
        public TimeSpan MaxDelay { get; set; } = TimeSpan.FromMinutes(1);

        /// <summary>
        /// The approach to use for calculating retry delays.
        /// </summary>
        public RetryMode Mode { get; set; } = RetryMode.Exponential;

        /// <summary>
        /// The timeout applied to an individual network operations.
        /// </summary>
        public TimeSpan NetworkTimeout { get; set; } = TimeSpan.FromSeconds(100);

Also, the following is relevant (note the randomness applied).

    private TimeSpan CalculateExponentialDelay(int attempted)
    {
        return TimeSpan.FromMilliseconds(
            Math.Min(
                (1 << (attempted - 1)) * _random.Next((int)(_delay.TotalMilliseconds * 0.8), (int)(_delay.TotalMilliseconds * 1.2)),
                _maxDelay.TotalMilliseconds));
    }

[TimothyMothra]

I think the BackoffLogicManager handles the exponential retries.
https://github.com/microsoft/ApplicationInsights-dotnet/blob/develop/BASE/src/ServerTelemetryChannel/Implementation/BackoffLogicManager.cs

Let me spend some time understanding how this works.
In my local testing the retry is a fixed interval, so maybe I've configured something incorrectly.

[TimothyMothra]

The BackoffLogicManager is not documented so I'm still trying to understand how this works.

It seems that the exponential backoff is triggered when it parses the retry value from the response header in GetBackOffTimeInterval().

ApplicationInsights-dotnet/BASE/src/ServerTelemetryChannel/Implementation/BackoffLogicManager.cs

Lines 142 to 175 in ce50b28

	public TimeSpan GetBackOffTimeInterval(string headerValue)
	{
	TimeSpan backOffTime = this.GetBackOffTime(headerValue);
	this.CurrentDelay = backOffTime;
	return backOffTime;
	}
	// Calculates the time to wait before retrying in case of an error based on
	// http://en.wikipedia.org/wiki/Exponential_backoff
	protected virtual TimeSpan GetBackOffTime(string headerValue)
	{
	if (!TryParseRetryAfter(headerValue, out TimeSpan retryAfterTimeSpan))
	{
	double delayInSeconds;
	if (this.ConsecutiveErrors <= 1)
	{
	delayInSeconds = SlotDelayInSeconds;
	}
	else
	{
	double backOffSlot = (Math.Pow(2, this.ConsecutiveErrors) - 1) / 2;
	var backOffDelay = Random.Next(1, (int)Math.Min(backOffSlot * SlotDelayInSeconds, int.MaxValue));
	delayInSeconds = Math.Max(Math.Min(backOffDelay, MaxDelayInSeconds), SlotDelayInSeconds);
	}
	TelemetryChannelEventSource.Log.BackoffTimeSetInSeconds(delayInSeconds);
	retryAfterTimeSpan = TimeSpan.FromSeconds(delayInSeconds);
	}
	TelemetryChannelEventSource.Log.BackoffInterval(retryAfterTimeSpan.TotalSeconds);
	return retryAfterTimeSpan;
	}

THIS won't work for us because in the AAD scenarios, our Ingestion Service does not provide a retry header.

The only example I've found of this is in the PartialSuccessTransmissionPolicy:

ApplicationInsights-dotnet/BASE/src/ServerTelemetryChannel/Implementation/TransmissionPolicy/PartialSuccessTransmissionPolicy.cs

Lines 144 to 155 in ce50b28

	private void DelayFutureProcessing(HttpWebResponseWrapper response, int statusCode)
	{
	// Disable sending and buffer capacity (=EnqueueAsync will enqueue to the Storage)
	this.MaxSenderCapacity = 0;
	this.MaxBufferCapacity = 0;
	this.LogCapacityChanged();
	this.Apply();
	// Back-off for the Delay duration and enable sending capacity
	this.backoffLogicManager.ReportBackoffEnabled(statusCode);
	this.pauseTimer.Delay = this.backoffLogicManager.GetBackOffTimeInterval(response.RetryAfterHeader);

I need more time to find a different way to use the existing code.

---

@cijothomas would it be possible to merge this as-is, without the exponential backoff?
If so, this would unblock Beta3 and give me more time to work on a solution.

[pharring]

You may be overthinking this.

If it's just about AAD auth, really all you need to be concerned with is this:
If you get a response that indicates the token is invalid (401 Unauthorized), then you should go back to the TokenCredential and ask for a new token and immediately retry. No need for any delay. If you get another 401, then fail the call (your credential is bad).

You can probably see why caching inside the TokenCredential is a hard to cope with. If you ask for a new token, but you get back the exact same token (because it was cached and it isn't close enough to expiration), you're hosed. How do you deal with that? I think you just have to fail on the spot.

For 403s (forbidden), the situation is subtle. That indicates that, while the token was good, you just don't have access (Metrics Publisher role is missing). It could be that you just added the role and it hasn't propagated to all regions yet. So, buffering on disk and retrying later might be the right thing -- but that's a long-cycle retry loop. Minutes, not seconds. It would be great if the SDK did that, but I wouldn't grumble if telemetry got dropped in this situation.

[TimothyMothra]

Thanks, @pharring.
In this case, I'm advocating for avoiding the exponential backoff. And I'll change the retry to at least a minute.

I agree, I think data loss is going to be inevitable in most of these situations.

[rajkumar-rangaraj]

If we receive HTTP 400 from the ingestion service, This method will block sender/buffer for the lifetime of an app. Storage will max out in short span of time as not transmissions are not sent to the service. Why are we planning to store transmissions offline in this scenario anyways we will lose data at later point.

[TimothyMothra]

I think data loss is inevitable in each of these scenarios.

Thinking about 400 specifically, I think it is recoverable if the customer disables AAD on their AI Resource.
For that reason, I think storage-only is inappropriate here.

[rajkumar-rangaraj]

Do we have a recommendation from Azure Identity on the retry timeframe?

[TimothyMothra]

Do we have a recommendation from Azure Identity on the retry timeframe?

We do not. In each of these cases, Azure Identity is working as expected.
If a customer reaches one of these cases, this means that the configuration between Azure & SDK are not in sync.

[cijothomas]

could you confirm if BadRequest is not thrown by ingestion for anything else like a invalid json, or json exceeding size etc?

[TimothyMothra]

I confirmed that none of our other Policies are handling 400.
I tested using invalid json and received;
{"itemsReceived":1,"itemsAccepted":0,"errors":[{"index":0,"statusCode":400,"message":"Unknown domain type 'undefined'"}]}

[cijothomas]

Does this mean that, if we sent a bad json, ingestion responds with 400, and AuthenticationTransmissionPolicy would queue this item to be retried...400...retries..400...retries. (and forever, as the bad json will be forever a bad json)?

(The current behavior is - we drop the item if Ingestion responds with 400, and rightly so, as its pointless to retry a invalid json).

[cijothomas]

AuthenticationTransmissionPolicy can ignore 400 as well and drop the data. If user disables AAD in portal, telemetry could be sent, but there is no way to distinguish this vs invalid jsob. So it might be better off not supporting retry for 400

[cijothomas]

you can still use the BackOffLogicManager - its smart enough to deal with the situation where the ingestion service does not provide a 'retry-after" value, and picks a default, and then do the exponential back offs.

[cijothomas]

it doesn't look like the backofflogicmanager is used anywhere but in this line.. which seems incorrect.

[TimothyMothra]

We have multiple threads now about BackoffLogicManager... , so i'm sorry if i've lost the current thread. This may be easier to discuss on a call.

I based my implementation off of ThrottlingTransmissionPolicy

ApplicationInsights-dotnet/BASE/src/ServerTelemetryChannel/Implementation/TransmissionPolicy/ThrottlingTransmissionPolicy.cs

Lines 62 to 72 in ce50b28

	this.backoffLogicManager.ReportBackoffEnabled((int)httpWebResponse.StatusCode);
	this.Transmitter.Enqueue(e.Transmission);
	this.pauseTimer.Delay = this.backoffLogicManager.GetBackOffTimeInterval(httpWebResponse.RetryAfterHeader);
	this.pauseTimer.Start(
	() =>
	{
	this.ResetPolicy();
	return Task.FromResult<object>(null);
	});
	}

At line 65, ThrottlingTransmissionPolicy calls this.backoffLogicManager.GetBackOffTimeInterval and provides the value from the retry header.
This triggers the backofflogicmanager to begin the exponential backoff.
I tried to discribe this above.

I can probably fake this by providing a custom formatted value. But I'll need to do more testing around this specifically.

[TimothyMothra]

What's strange to me is that in the entire SDK, we only have one implementation of TransmissionPolicy that is correctly resetting the backoff:

PartialSuccessTransmissionPolicy is the only class that calls this.backoffLogicManager.ResetConsecutiveErrors();

ApplicationInsights-dotnet/BASE/src/ServerTelemetryChannel/Implementation/TransmissionPolicy/PartialSuccessTransmissionPolicy.cs

Lines 105 to 112 in ce50b28

	private void HandleTransmissionSentEvent(object sender, TransmissionProcessedEventArgs args)
	{
	if (args.Exception == null && (args.Response == null || args.Response.StatusCode == ResponseStatusCodes.Success))
	{
	// We successfully sent transmittion
	this.backoffLogicManager.ResetConsecutiveErrors();
	return;
	}

I don't have an explanation for this and would need more time to fully investigate

[rajkumar-rangaraj]

This PR looks good. We could take exponential backoff on a different issue/conversation.

Currently, AI SDK enables exponential backoff for server(ingestion) side issues. All AAD error codes sent by ingestion service is for client issue. I don't think we should be enabling exponential backoff for client errors, rather trying to reach out the service at a constant time would be a good approach.

[rajkumar-rangaraj]

This policy does not handle BadRequest, let us remove it from the comment.