-
Notifications
You must be signed in to change notification settings - Fork 357
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
Function Invocation Filters #1207
Conversation
@hhawkins, |
@@ -608,7 +613,8 @@ private static ITaskSeriesTimer StartParameterLogTimer(IRecurrentCommand updateC | |||
} | |||
|
|||
internal static async Task InvokeAsync(IFunctionInvoker invoker, object[] invokeParameters, CancellationTokenSource timeoutTokenSource, | |||
CancellationTokenSource functionCancellationTokenSource, bool throwOnTimeout, TimeSpan timerInterval, IFunctionInstance instance) | |||
CancellationTokenSource functionCancellationTokenSource, bool throwOnTimeout, TimeSpan timerInterval, IFunctionInstance instance, ILogger logger = null, | |||
JobHost jobHost = null, JobHostConfiguration config = null, IReadOnlyList<string> parameterNames = null) |
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.
nit: can you reorder the parameterNames parameter next to the invokeParameters param, before it? Also, I don't think these new parameters should be optional (remove the "= null" on them), unless you have a reason?
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.
The reason behind putting the "= null" was because when I would build it, certain functions in other files would call the function and not provide the new parameters thus causing errors. I can remove the "= null" but then I'd have to pass the new parameters in other places in the project where it does call it. I'd also have to modify other tests.
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 workaround is to have 2 signatures. One with a required parameter (what Mathew wanted), and then one without the parameter that just passes null. The benefit of that is then we know statically which is which.
In reply to: 124422411 [](ancestors = 124422411)
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.
Yes, all those other locations need to be fixed up :) These are required parameters now. I see that all those places are in the tests - they just need to be updated.
} | ||
|
||
/// <summary>Gets or sets the ID of the function.</summary> | ||
public Guid Id { get; set; } |
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.
Id [](start = 20, length = 2)
Is there a more descriptive term than just Id? (InstanceId? FunctionInstanceId?)
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.
It is the FunctionInstanceId. I will change that.
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.
Also update the ctor param so it matches as well
public IReadOnlyDictionary<string, object> Arguments { get; set; } | ||
|
||
/// <summary> | ||
/// User properties |
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.
/// User properties [](start = 8, length = 19)
What's the difference between Properties and Arguments?
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.
Your question is an indication that this summary comment could be better :) This is a property bag that allows users to flow their own custom state through between their executing/executed filters. Standard pattern.
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.
However, you only have a getter, and the value will always be null? Likely you don't have any tests yet using this - please add some :) Just add a setter to this as well, and let the user be responsible for setting it if needed.
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.
But we don't actually carry these over from pre to post.
In reply to: 124420331 [](ancestors = 124420331)
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.
Yes, that's a hole that needs to be fixed, and tests added for it :)
/// <summary>Gets or sets the ID of the function.</summary> | ||
public Guid Id { get; set; } | ||
|
||
/// <summary>Gets or sets the name of the function.</summary> |
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.
or sets [](start = 25, length = 8)
set?
Logger = logger; | ||
} | ||
|
||
/// <summary>Gets or sets the ID of the function.</summary> |
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.
or sets [](start = 26, length = 7)
set?
namespace Microsoft.Azure.WebJobs.Host | ||
{ | ||
/// <summary> | ||
/// The context for an executed function |
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.
The context for an executed function [](start = 7, length = 37)
suggested: "The context describing a function that's about to be executed."
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.
Apply similar wording to your other context classes as well
/// <summary> | ||
/// The context for an executed function | ||
/// </summary> | ||
[CLSCompliant(false)] |
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.
[CLSCompliant(false)] [](start = 4, length = 21)
What are these for?
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.
Without that identifier, the compiler complains about the function not being CLS compliant
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 assembly is CLS compliant, but these types are using non CLS compliant constructs, or at least they were. Hamza, if you remove these, what does the compiler say the issue is? I can't remember. Maybe they're not needed anymore.
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.
The property type ILogger is non CLS compliant according to the compiler in the main FunctionInvocationContext class
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.
Ah, I see, thanks. We might separately investigate just removing our CLS compliance, since this is just a headache that doesn't really add value, and forces us to sprinkle these attributes all over everywhere.
await InvokeJobFunctionAsync(executingContext, cancellationToken); | ||
} | ||
catch (Exception e) | ||
{ |
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.
{ [](start = 16, length = 1)
We swallow the error - should this abort the call?
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.
We shouldn't be swallowing errors. All of the error paths for filter invocations need to be reviewed. I think the behavior should be:
- if any executing filters throw, skip the rest of the executing filters, skip the user method, and invoke executing filters
- if any executed filter throws, skip the rest of the executed filters
We'll need tests for this once we agree.
{ | ||
break; | ||
} | ||
} |
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 is related to our [FunctionName] support in #1170
This will need to be fixed when address that bug.
/// <summary> | ||
/// Executing filter | ||
/// </summary> | ||
public string ExecutingFilter { get; } |
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.
ExecutingFilter [](start = 22, length = 15)
What is the format for this function? We should think about this in context of other function name usages (See #1170 )
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.
Yes, I was thinking the same thing. Right now he's bee using just simple short names, but in cases where there may be multiple job classes with the same name (collisions), we'll need namespacing. I was thinking we should support both short and long names - e.g. if the name is "Foo", we return the first Foo function we find. If the name is "Bar.Foo" we look for type "Bar" in the type locator, and return its "Foo" function.
|
||
IDictionary<string, object> invokeArguments = new Dictionary<string, object>(); | ||
string parameterName = methodInfo.GetParameters().SingleOrDefault(p => p.ParameterType == typeof(TContext)).Name; | ||
invokeArguments.Add(parameterName, context); |
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.
invokeArguments.Add(parameterName, context); [](start = 12, length = 44)
need a null check on parameterNAme. The target function may not have a context.
{ | ||
try | ||
{ | ||
if (filter.GetType() == typeof(InvokeFunctionFilterAttribute)) |
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.
Shouldn't need this special type casing here. Since the JobHost/Config properties are internal, I think it's fine for them to always be set, which would allow you to compress this code.
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 do understand where you're coming from. The reason why it's separated like that is because the InvokeFunctionFiterAttribute is separate from a normal InocationFilterAttribute in terms of behavior and accessing the JobHost and the Config. A normal InvocationFilterAttribute can be set by the user who's putting a custom filter together, the InvokeFunctionFilterAttribute has a specific job to invoke a function and so the user doesn't have control over how the next function is called.
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 still don't understand. It seems to me you're calling these the same way in both cases, so if you just always set host/config on the context, the same code will work for both?
await InvokeJobFunctionAsync(executedContext, cancellationToken); | ||
} | ||
catch (Exception e) | ||
{ |
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.
{ [](start = 16, length = 1)
swallow?
|
||
for (var i = 0; i < invokeParameters.Length; i++) | ||
{ | ||
parameters.Add(parameterNames.ToArray<string>()[i], invokeParameters[i]); |
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.
Don't do the ToArray in the loop like this - it gets executed on every iteration. You can just do parameterNames[i]
|
||
foreach (var type in context.Config.TypeLocator.GetTypes()) | ||
{ | ||
methodInfo = type.GetMethod(ExecutingFilter); |
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.
ExecutingFilter [](start = 44, length = 15)
Does this need to be a parameter passed in?
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.
If you mean ExecutingFilter here, then yes, this is the name of the filter that we would call.
|
||
var parameters = new Dictionary<string, object>(); | ||
|
||
for (var i = 0; i < invokeParameters.Length; i++) |
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.
Move the creation of the parameters dictionary into your below if (invocationFilters != null)
block, so we only do it if needed.
@@ -5,6 +5,7 @@ | |||
using System.Collections.Generic; | |||
using System.Diagnostics.CodeAnalysis; | |||
using System.Globalization; | |||
using System.Linq; |
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.
remove?
{ | ||
Task InvokeAsync(MethodInfo methodInfo, IReadOnlyDictionary<string, object> parameters, CancellationToken cancellationToken); | ||
|
||
MethodInfo GetMethodInfoForInvoke(string methodName); |
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.
MethodInfo GetMethodInfoForInvoke(string methodName); [](start = 8, length = 53)
Is this used?
@@ -238,7 +238,7 @@ public static ServiceProviderWrapper CreateStaticServices(this JobHostConfigurat | |||
|
|||
if (functionExecutor == null) | |||
{ | |||
functionExecutor = new FunctionExecutor(functionInstanceLogger, functionOutputLogger, exceptionHandler, trace, functionEventCollector, loggerFactory); | |||
functionExecutor = new FunctionExecutor(functionInstanceLogger, functionOutputLogger, exceptionHandler, trace, host, config, functionEventCollector, loggerFactory); |
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.
config [](start = 137, length = 6)
Host has the config; so can we just pass host?
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.
We had removed the new public property we added (JobHost.Configuration) to avoid controversy :) If you like the new property we could add it back - I think it makes sense. Just removed it because it was new surface area that wasn't strictly needed.
internal JobHost JobHost { get; set; } | ||
|
||
/// <summary> | ||
/// Gets or sets the method invoker of the JobHost |
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.
the JobHost [](start = 47, length = 11)
typo?
} | ||
} | ||
|
||
internal async Task InvokeJobFunctionAsync<TContext>(TContext context, CancellationToken cancellationToken) where TContext : FunctionInvocationContext |
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.
InvokeJobFunctionAsync [](start = 28, length = 22)
Could we move this to FunctionInvocationContext?
And then FunctionInvocationContext.config, host could be private. (these are dangerous things to be passing around and we want to limit their scope.)
parameters.Add(parameterNames.ToArray<string>()[i], invokeParameters[i]); | ||
} | ||
|
||
if (invokeFilters != null) |
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 don't think this will ever be null - please test it out. You might have test holes. If there are no custom attributes of the right type, an empty collection/array is returned I believe, not null. You can then change this to if (invokeFilters.Any())
{ | ||
MethodInfo functionMethod = instance.FunctionDescriptor.Method; | ||
CancellationToken cancellationToken = functionCancellationTokenSource.Token; | ||
var invokeFilters = functionMethod.GetCustomAttributes().OfType<InvocationFilterAttribute>(); |
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.
Many of the same comments I made above apply in this method as well.
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
||
namespace Microsoft.Azure.WebJobs.Host.Executors |
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 file can be removed now, right?
public class FunctionExecutedContext : FunctionInvocationContext | ||
{ | ||
/// <summary> | ||
/// Constructor to set the context |
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.
Need to fill all this param documentation out, here and througout
…ns/azure-webjobs-sdk into InvocationFilterAttribute # Conflicts: # sample/SampleHost/SampleHost.csproj # src/Microsoft.Azure.WebJobs/WebJobs.csproj # test/Microsoft.Azure.WebJobs.Host.EndToEndTests/WebJobs.Host.EndToEndTests.csproj
There's another pull request open on #1244 due to this branch being out of sync |
For feature tracked here: #980. Sending out another pull request after making updates and changes from feedback from the first pull request #1189