Expose current retry count in the context of the function (Issue #2595) #2658

alrod · 2021-01-09T06:50:21Z

Related to #2595
Resolves #2661

src/Microsoft.Azure.WebJobs.Host/Bindings/ExecutionContext/ExecutionContext.cs

pragnagopa · 2021-01-11T18:29:05Z

@alrod - Please open a separate issue in host repo to track updating RPC layer to include RetryContext and link to this PR

mathewc · 2021-01-11T18:43:49Z

src/Microsoft.Azure.WebJobs.Host/Bindings/ExecutionContext/ExecutionContext.cs

+        /// <summary>
+        /// Gets or sets value for retry count.
+        /// </summary>
+        public int RetryCount { get; set; }


Issue #2595 mentions that the use case is to be able to check if RetryCount = MaxRetries. This doesn't enable that, right? Was it decided that that wasn't needed? However, the logic for determining whether this is truly the last retry would also have to also take the message dequeue count into account in the case of SB/Queues, since there are two levels of retries going on. E.g. if we're at the max dequeue count AND the max retry, we know this is the very last attempt.

I did not read #2595 carefully enough. I added IRetryStratagy to RetryPolicy and exposed 'RetryPolicy' in 'ExecutionContext'.

However, the logic for determining whether this is truly the last retry would also have to also take the message dequeue count into account in the case of SB/Queues, since there are two levels of retries going on. E.g. if we're at the max dequeue count AND the max retry, we know this is the very last attempt.

We need to point this out in our docs. By design the retry logic only execute the function code inside one instance and do not know about specific trigger retry mechanism(it's possible that another instance will retry execution for a storage/SB message).

@pragnagopa, is it ok add IRetryStratagy for RPC layer? or we need to use simple types?

It is a little strange to add IRetryStrategy to RetryContext, because that context was designed to pass into IRetryStrategy.GetNextDelay. We're now wanting to pass RetryContext in other contexts and want more info on it. Another option would be to add MaxRetries to RetryContext. That is also a bit of duplication from the standpoint of an IRetryStrategy implementor, but at least leaves us with a single context object with all the info needed for inspection by other consumers (e.g. ExecutionContext). Thoughts @pragnagopa? I don't think IRetryStrategy itself is needed for ExecutionContext, since that interface is for determining the next delay, something that we wouldn't want function code calling since calling it would advance the retry state.

anyway, I added MaxRetriesCount to ExecutionContext instead of IRetryStrategy. I think it fits better.

mathewc · 2021-01-11T18:44:28Z

src/Microsoft.Azure.WebJobs.Host/Executors/IFunctionInstance.cs

@@ -22,6 +22,8 @@ public interface IFunctionInstance
        IFunctionInvoker Invoker { get; }

        FunctionDescriptor FunctionDescriptor { get; }
+
+        int RetryCount { get;  }


This would be a breaking change to a public interface. There's an existing extensibility issue here as evidenced by the IFunctionInstanceEx interface which was added recently to add a single new property. One way we might solve this for now w/o breaking changes:

add internal property RetryCount to FunctionInstance class. Alternatively to address Pragna's suggestion above this could be RetryContext itself

add internal property FunctionInstance to FunctionBindingContext

in the function executor extension where we're invoking (right before call to TryExecuteAsync), try/cast to FunctionInstance and set RetryCount (or RetryContext)

in ExecutionContextBindingProvider, try/cast to FunctionInstance to get RetryCount/RetryContext and FunctionInstance.FunctionDescriptor.RetryStrategy.MaxRetries (from FunctionBindingContext) to flow through via ExecutionContext

This keeps it all internal - we can improve in the future w/o any breaking changes.

mathewc · 2021-01-11T19:03:35Z

src/Microsoft.Azure.WebJobs.Host/Bindings/FunctionBindingContext.cs

+        /// <param name="functionInvocationServices">The user logger.</param>
+        /// <param name="functionDescriptor">Current function being executed. </param>
+        /// /// <param name="functionDescriptor">Current retry count. </param>
+        public FunctionBindingContext(


Let's just add a new ctor taking parameters IFunctionInstanceEx and CancellationToken. All the values needed come from the function instance. FunctionExecutor can then be simplified to call this overload.

…e#2595)

alrod · 2021-01-13T01:14:34Z

@alrod - Please open a separate issue in host repo to track updating RPC layer to include RetryContext and link to this PR

Azure/azure-functions-host#7048

mathewc · 2021-01-15T17:00:02Z

src/Microsoft.Azure.WebJobs.Host/Bindings/FunctionBindingContext.cs

+        /// </summary>
+        /// <param name="functionInstanceId">The instance ID of the function being bound to.</param>
+        /// <param name="functionCancellationToken">The <see cref="CancellationToken"/> to use.</param>
+        /// <param name="functionInvocationServices">The user logger.</param>


These params don't match

mathewc · 2021-01-15T17:05:22Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutorExtensions.cs

+                    retryContext = new RetryContext
+                    {
+                        RetryCount = attempt,
+                        Exception = retryContext == null ? null : retryContext.Exception, // Pass exception from previos attempt


nit: spelling "previos"

mathewc · 2021-01-15T17:08:59Z

src/Microsoft.Azure.WebJobs.Host/Executors/IFunctionInstance.cs

@@ -27,5 +27,7 @@ public interface IFunctionInstance
    public interface IFunctionInstanceEx : IFunctionInstance
    {
        IServiceProvider InstanceServices { get; }
+
+        RetryContext RetryContext { get; }


Again - this is a breaking change. You can't add new properties to public interfaces - it would break any existing implementations. I don't understand why you're adding this here.

I though that IFunctionInstanceEx is internal by some reason. Fixed.

mathewc · 2021-01-15T17:09:42Z

src/Microsoft.Azure.WebJobs.Host/RetryContext.cs

@@ -21,6 +21,11 @@ public class RetryContext
        /// </summary>
        public Exception Exception { get; set; }

+        /// <summary>
+        /// Gets or sets max retry stratagy.


nit: "Gets or sets the retry strategy"

mathewc · 2021-01-19T18:00:26Z

src/Microsoft.Azure.WebJobs.Host/RetryContext.cs

@@ -11,11 +13,18 @@ namespace Microsoft.Azure.WebJobs.Host
    /// </summary>
    public class RetryContext
    {
+        private Dictionary<string, object> _stateDictionary = new Dictionary<string, object>();


Make this Lazy so it's only created when needed. E.g. for FixedDelay we'll never need it, so don't want the perf hit of unused object creation.

mathewc · 2021-01-19T18:06:12Z

src/Microsoft.Azure.WebJobs.Host/RetryContext.cs

+        /// <summary>
+        /// Gets the state dictionary.
+        /// </summary>
+        internal IDictionary<string, object> StateDictionary => _stateDictionary;


Can we just name this "State"? We can also add a doc section here to the effect "This property can be used by retry strategies to store arbitrary state for a retried invocation.".

mathewc · 2021-01-19T18:06:19Z

src/Microsoft.Azure.WebJobs.Host/RetryContext.cs

+        /// <summary>
+        /// Gets the state dictionary.
+        /// </summary>
+        internal IDictionary<string, object> StateDictionary => _stateDictionary;


Can we just name this "State"? We can also add a doc section here to the effect "This property can be used by retry strategies to store arbitrary state for a retried invocation.".

Also this needs to be public - it's part of the programming model now for retry strategy authors. IRetryStrategy implementations used across invocations, so they need a place to store per invocation state.

mathewc · 2021-01-19T18:21:09Z

src/Microsoft.Azure.WebJobs.Host/ExponentialBackoffRetryAttribute.cs

+            else
+            {
+                delayStrategy = new RandomizedExponentialBackoffStrategy(_parsedMinimumInterval, _parsedMaximumInterval);
+                context.StateDictionary.Add(nameof(RandomizedExponentialBackoffStrategy), new RandomizedExponentialBackoffStrategy(_parsedMinimumInterval, _parsedMaximumInterval));


for the dictionary key, can we use something like _msDelayStrategy? In general when we add fx properties to user accessible collecitons we use a MS prefix to avoid collisions.

mathewc · 2021-01-19T18:25:10Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutorExtensions.cs

+                if (functionInstance is FunctionInstance instance)
+                {
+                    // Build retry context
+                    retryContext = new RetryContext


Need to copy State as well here. If you don't have a test that is verifying that custom state is handled properly across retries, please add one. I wonder if to facilitate this, we shouldn't just make State a settable property rather than a Lazy.

mathewc · 2021-01-19T18:30:19Z

src/Microsoft.Azure.WebJobs.Host/ExponentialBackoffRetryAttribute.cs

@@ -49,9 +48,21 @@ public ExponentialBackoffRetryAttribute(int maxRetryCount, string minimumInterva

        public override TimeSpan GetNextDelay(RetryContext context)
        {
+            IDelayStrategy delayStrategy;
+
+            if (context.StateDictionary.TryGetValue(nameof(RandomizedExponentialBackoffStrategy), out object stateObject))


I just want to point out that this new State dictionary now enables our ExponentialBackoffRetryAttribute implementation as it existed. Note that if we weren't reusing the RandomizedBackoffRetryAttribute stateful class, this retry strategy could have been implemented without using State, since all required exponential backoff state can be determined/incremented solely based on the retry count which is maintained on the RetryContext.

So our addition of State is good since it adds flexibility for strategy implementors, but it'll be rare that people actually need to use it.

src/Microsoft.Azure.WebJobs.Host/Bindings/FunctionBindingContext.cs

mathewc · 2021-01-19T18:55:18Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutorExtensions.cs

+                    };
+
+                    instance.RetryContext = retryContext;
+                    functionResult = await executor.TryExecuteAsync(instance, cancellationToken);


Looks like this code can be simplified by removing the else block below and executing these two lines outside of this if.

pragnagopa

Thanks for the PR. Added few comments. Since this is changing public surface, it would be good to prototype changes on the host and Rpc layer to ensure no other changes in SDK are needed.

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutorExtensions.cs

src/Microsoft.Azure.WebJobs.Host/ExponentialBackoffRetryAttribute.cs

pragnagopa · 2021-01-19T21:48:23Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Retry/RetryAttributeTests.cs

@@ -62,7 +62,6 @@ public void ExponentialBackOffDelay_Constructor_Throws()
        {
            Assert.Throws<ArgumentOutOfRangeException>(() => new ExponentialBackoffRetryAttribute(5, "01:020000000000:25", "00:00:10"));
            Assert.Throws<ArgumentOutOfRangeException>(() => new ExponentialBackoffRetryAttribute(5, "01:02:25", "100000000"));
-            Assert.Throws<ArgumentException>(() => new ExponentialBackoffRetryAttribute(5, "05:02:25", "00:00:10"));


intentionally removed?

yes, the ExponentialBackoffRetryAttribute ctr does not throw the exception anymore.

good to have this validation in place to avoid adding additional validation logic in the host. What do you think?

You can retain the validation by refactoring the RandomizedExponentialBackoffStrategy ctor validation into a static Validate method on the RandomizedExponentialBackoffStrategy class that both that class ctor and ExponentialBackoffRetryAttribute ctor can call.

alrod · 2021-01-20T03:09:36Z

Thanks for the PR. Added few comments. Since this is changing public surface, it would be good to prototype changes on the host and Rpc layer to ensure no other changes in SDK are needed.

I checked the change with host/node worker locally:

https://github.com/alrod/azure-webjobs-sdk-script/tree/alrod/rpcretry
https://github.com/Azure/azure-functions-nodejs-worker/tree/alrod/retryCount

pragnagopa

Thanks for the prototyping changes in RPC layer! Looking at https://github.com/alrod/azure-webjobs-sdk-script/tree/alrod/rpcretry , we should define "RetryContext" to ensure we can expose all the properties - including exception. We can discuss these changes in Azure/azure-functions-language-worker-protobuf#52 . Rest looks OK.

src/Microsoft.Azure.WebJobs.Host/RetryContext.cs

mathewc · 2021-01-20T17:44:53Z

test/Microsoft.Azure.WebJobs.Host.UnitTests/Retry/RetryAttributeTests.cs

+        }
+
+        [Fact]
+        public void ExponentialBackOffDelay_GetNextDelay_Throws()


If you do the Validate refactor suggested above, this test can go away in favor of the existing ctor tests.

mathewc · 2021-01-20T17:52:12Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutorExtensions.cs

@@ -22,6 +22,8 @@ public static async Task<IDelayedException> TryExecuteAsync(this IFunctionExecut
            var attempt = 0;
            IDelayedException functionResult = null;
            ILogger logger = null;
+            // Build retry context


nit: remove comment

mathewc · 2021-01-20T23:18:48Z

src/Microsoft.Azure.WebJobs.Host/Executors/FunctionExecutorExtensions.cs

@@ -36,7 +36,6 @@ public static async Task<IDelayedException> TryExecuteAsync(this IFunctionExecut
                // Set retry context only if retry strategy exists and this is not first attempt
                if (functionInstance.FunctionDescriptor.RetryStrategy != null && attempt > 0 && functionInstance is FunctionInstance instance)
                {
-                    retryContext = retryContext ?? new RetryContext();


I see we can also simplify the above if clause (and comment) - we only need if(retryContext != null && functionInstance is FunctionInstance instance) since attempt will always be > 0 at that point, and we short circuit below if RetryStrategy is null.

alrod requested review from pragnagopa and mathewc January 9, 2021 06:50

pragnagopa reviewed Jan 11, 2021

View reviewed changes

src/Microsoft.Azure.WebJobs.Host/Bindings/ExecutionContext/ExecutionContext.cs Show resolved Hide resolved

pragnagopa requested a review from jeffhollan January 11, 2021 18:26

mathewc reviewed Jan 11, 2021

View reviewed changes

Expose current retry count in the context of the function (Issue Azur…

6407f91

…e#2595)

alrod force-pushed the alrod-retrycount branch from cde0db2 to 6407f91 Compare January 13, 2021 00:45

alrod mentioned this pull request Jan 13, 2021

Updating RPC layer to include RetryContext Azure/azure-functions-host#7048

Closed

alrod requested review from mathewc and pragnagopa January 13, 2021 01:14

alrod changed the title ~~Expose current retry count in the context of the function (Issue #2595)~~ [pack] Expose current retry count in the context of the function (Issue #2595) Jan 14, 2021

alrod changed the title ~~[pack] Expose current retry count in the context of the function (Issue #2595)~~ Expose current retry count in the context of the function (Issue #2595) Jan 14, 2021

mathewc reviewed Jan 15, 2021

View reviewed changes

alrod added 2 commits January 15, 2021 14:12

Fix PR

cd50744

Make ExponentialBackoffRetry stateless

c9eed45

alrod requested a review from mathewc January 16, 2021 00:35

Fix tests

4813f1a

mathewc reviewed Jan 19, 2021

View reviewed changes

src/Microsoft.Azure.WebJobs.Host/Bindings/FunctionBindingContext.cs Show resolved Hide resolved

mathewc reviewed Jan 19, 2021

View reviewed changes

Fix PR2

db6ae15

alrod requested a review from mathewc January 19, 2021 21:43

pragnagopa reviewed Jan 19, 2021

View reviewed changes

alrod requested a review from pragnagopa January 20, 2021 03:11

pragnagopa approved these changes Jan 20, 2021

View reviewed changes

src/Microsoft.Azure.WebJobs.Host/RetryContext.cs Show resolved Hide resolved

mathewc reviewed Jan 20, 2021

View reviewed changes

alrod added 2 commits January 20, 2021 11:38

Fix PR3

8d34b56

Fix PR3

ae1fe13

mathewc approved these changes Jan 20, 2021

View reviewed changes

pragnagopa approved these changes Jan 20, 2021

View reviewed changes

Fix test

cafbfff

mathewc reviewed Jan 20, 2021

View reviewed changes

Simplify if clause

def55bd

mathewc approved these changes Jan 20, 2021

View reviewed changes

Set retryContext properties right after creation

9496076

mathewc approved these changes Jan 21, 2021

View reviewed changes

alrod merged commit a3e31ba into Azure:dev Jan 21, 2021

alrod mentioned this pull request Feb 3, 2021

Expose current retry count in the context of the function #2595

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose current retry count in the context of the function (Issue #2595) #2658

Expose current retry count in the context of the function (Issue #2595) #2658

alrod commented Jan 9, 2021 •

edited by fabiocav

Loading

pragnagopa commented Jan 11, 2021 •

edited

Loading

mathewc Jan 11, 2021 •

edited

Loading

alrod Jan 13, 2021 •

edited

Loading

mathewc Jan 15, 2021 •

edited

Loading

alrod Jan 15, 2021

mathewc Jan 11, 2021 •

edited

Loading

alrod Jan 13, 2021

mathewc Jan 11, 2021

alrod Jan 13, 2021

alrod commented Jan 13, 2021

mathewc Jan 15, 2021

alrod Jan 15, 2021

mathewc Jan 15, 2021

mathewc Jan 15, 2021

alrod Jan 15, 2021

mathewc Jan 15, 2021

mathewc Jan 19, 2021

mathewc Jan 19, 2021

mathewc Jan 19, 2021

mathewc Jan 19, 2021 •

edited

Loading

mathewc Jan 19, 2021

mathewc Jan 19, 2021 •

edited

Loading

mathewc Jan 19, 2021

mathewc Jan 19, 2021

pragnagopa left a comment

pragnagopa Jan 19, 2021

alrod Jan 20, 2021

pragnagopa Jan 20, 2021

mathewc Jan 20, 2021

alrod commented Jan 20, 2021

pragnagopa left a comment

mathewc Jan 20, 2021

mathewc Jan 20, 2021

mathewc Jan 20, 2021 •

edited

Loading

Expose current retry count in the context of the function (Issue #2595) #2658

Expose current retry count in the context of the function (Issue #2595) #2658

Conversation

alrod commented Jan 9, 2021 • edited by fabiocav Loading

pragnagopa commented Jan 11, 2021 • edited Loading

mathewc Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

alrod Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

mathewc Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Jan 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrod commented Jan 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pragnagopa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrod commented Jan 20, 2021

pragnagopa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

alrod commented Jan 9, 2021 •

edited by fabiocav

Loading

pragnagopa commented Jan 11, 2021 •

edited

Loading

mathewc Jan 11, 2021 •

edited

Loading

alrod Jan 13, 2021 •

edited

Loading

mathewc Jan 15, 2021 •

edited

Loading

mathewc Jan 11, 2021 •

edited

Loading

mathewc Jan 19, 2021 •

edited

Loading

mathewc Jan 19, 2021 •

edited

Loading

mathewc Jan 20, 2021 •

edited

Loading