-
Notifications
You must be signed in to change notification settings - Fork 66
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
Include all bindings when generating function metadata #542
Conversation
common.props
Outdated
@@ -4,7 +4,7 @@ | |||
|
|||
<MajorProductVersion>4</MajorProductVersion> | |||
<MinorProductVersion>0</MinorProductVersion> | |||
<PatchProductVersion>1</PatchProductVersion> | |||
<PatchProductVersion>2</PatchProductVersion> |
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 published the nuget package locally and consumed it in an in-proc app and verified that it works fine (and all bindings are used in the binding metric logging code path)
// Get binding if a return attribute is used. | ||
// Ex: [return: Queue("myqueue-items-a", Connection = "MyStorageConnStr")] | ||
var returnBindings = GetOutputBindingsFromReturnAttribute(method); | ||
var allBindings = bindingsFromParameters.Concat(returnBindings); |
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.
Let's retain the ToArray so the result is materialized. I don't know how downstream callers are using this.
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.
Ok. Kept the ToArray
call as suggested. But as you already have noticed, the Binding property (to which this collection was assigned) is IEnumerable.
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.
That's fine - we just don't want a deferred enumerable that gets reevaluated each time
src/Microsoft.NET.Sdk.Functions.Generator/MethodInfoExtensions.cs
Outdated
Show resolved
Hide resolved
/// [return: Queue("myqueue-items-a", Connection = "MyStorageConnStra")] | ||
/// public static string Run([HttpTrigger] HttpRequestMessage request) => "foo"; | ||
/// </summary> | ||
private static JObject[] GetOutputBindingsFromReturnAttribute(MethodDefinition method) |
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 was going to say that a return should/can only be bound once, but it looks like in this generator, we're handling 1->N (e.g. here). I'd expect at runtime that this would fail however, since parameter/return bindings must be 1:1. Not sure if we actually have explicit validation for this in the host or SDK. Ultimately the binding provider model for parameters is first one wins https://github.com/Azure/azure-webjobs-sdk/blob/88ed7806c63d9c589ecf1a44ec7575f1e72384e5/src/Microsoft.Azure.WebJobs.Host/Indexers/FunctionIndexer.cs#L172.
test/Microsoft.NET.Sdk.Functions.Generator.Tests/FunctionJsonConverterTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.NET.Sdk.Functions.Generator.Tests/FunctionJsonConverterTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.NET.Sdk.Functions.Generator.Tests/FunctionJsonConverterTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.NET.Sdk.Functions.Generator.Tests/FunctionJsonConverterTests.cs
Outdated
Show resolved
Hide resolved
Looks like you have some build failures, but code looks good to me |
Build failure is caused by an unrelated issue. I have another PR to fix it here. #543 Will rebase this branch after that one is merged. |
common.props
Outdated
@@ -4,7 +4,7 @@ | |||
|
|||
<MajorProductVersion>4</MajorProductVersion> | |||
<MinorProductVersion>0</MinorProductVersion> |
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: We should bump the minor here.
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.
Bumped minor version number. New version with this fix will be 4.1.0
d542aa0
to
fe16f24
Compare
fe16f24
to
d021e90
Compare
Sdk.Functions package 4.1.0 which has this fix, is now available in nuget gallery. https://www.nuget.org/packages/Microsoft.NET.Sdk.Functions/4.1.0 |
* Adding all bindings to function.json * Fixed casing of direction attribute * Update assembly version * Removed Array allocation * sFixed to address PR comments * Bumped minor version of package.
* Adding all bindings to function.json * Fixed casing of direction attribute * Update assembly version * Removed Array allocation * sFixed to address PR comments * Bumped minor version of package.
Currently, when a function invocation happens, we log binding usage to our function metrics logs table. We recently found out that not all the bindings were logged for in-proc precompiled apps.
For in-proc apps, the function.json generated is not including all the bindings. It is including only the trigger bindings. In this PR, we are making a change to include all bindings.
We have also decided to remove the binding direction from the event name when logging the binding metric in host.