-
Notifications
You must be signed in to change notification settings - Fork 44
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
Provide function execution hooks #522
Comments
I propose adding support for an optional file containing startup code because there's nowhere else to put startup code. There should be a way to specify the name of the startup file. Options include:
Startup file should support both common.js (startup.js) or ES modules (startup.mjs). Example of what this file might look like: module.exports = async function startup(app) {
app.registerPreinvocationHook(async (context) => {
// do stuff with context
// supports both sync and promise-returning functions
});
app.registerPostinvocationHook(async (context) => {
// do stuff with context
// supports both sync and promise-returning functions
});
app.registerStartupHook(async () => {
// do stuff on worker startup
// technically not needed, can just put code in this file
// supports both sync and promise-returning functions
});
app.registerShutdownHook(async () => {
// do stuff on worker shutdown
// supports both sync and promise-returning functions
});
}
// single function
app.registerPreinvocationHook("myfunction", (context) => {});
// multiple functions
app.registerPreinvocationHook(["myfunction1", "myfunction2"], (context) => {});
// complex filtering
app.registerPreinvocationHook((context) => {
// filter logic here
}); For each type of hook, hooks are run in the order in which they were registered. How this might work for App Insights or any other package that want to register hooks: const appInsightsHooks = require('app-insights-functions-hooks');
module.exports = async function startup(app) {
appInsightsHooks.register(app);
}; |
This sounds like a great feature, in terms of where to register the startup file in Just my 2-pence.... The NodeJs function apps I build are all built using a webpack pipeline resulting in a deployable codebase without a For this reason, I'd prefer to see the startup configuration in |
Preface: From a technical perspective, we need the function app directory to read the host.json or package.json. That is not provided by the host today, but is provided with the new worker indexing changes as a part of #480, which is still behind a feature flag. If I ignore that limitation temporarily, here are my suggestions: package.jsonIf we're going to add to package.json (my preference), IMO we should use the "main" field that already exists (docs here) as opposed to a custom field for Azure Functions. In response to @phillipharding, the functions runtime does not run startup.jsI would suggest this file should be more flexible as opposed to just used for startup. First, I would name it "main.js" or "index.js" which I believe are the most common entrypoint names for Node.js apps. Second, I would suggest exporting the "startup" function as part of an object, not as the main export. This allows us to add other functions to the main export in the future (for example "shutdown"): async function startup(app) {}
async function shutdown(app) {}
module.exports = { startup, shutdown } Azure Functions APIMy last suggestion is to provide an official "module" or "api" from the Node.js worker as opposed to just this "app" object at startup. An issue with the app object is that you have to store it and pass it around if you want to use it in other files and it can implicitly only be used at startup. I would recommend we provide a "built-in" module (similar to how Node.js provides "http" or "path") where it's just there by default without having to install it in your project. import * as app from "@azure/function-app"; // Name TBD. A few others I considered were "@azure/functions" (matching the existing types package) and "@azure/functions-worker"
app.addHook('preInvocation', async (context: Context) => {
}); Another benefit of this model is I believe it would be easier to do with or without a startup function. Users could add the hook in any one of their function files in the existing programming model, and then just move that to a startup file in the new programming model. |
Another point to ensure is considered... because what's proposed so far are separate pre and post invocation hooks, the only way for pre and post hooks to communicate is via adding stuff to the context object. I think there should be clear guidance where in the context object this data should be placed. |
As an overall concept - this is something that I think would be really useful and I can see several use cases which it would fit well for, one of this is the OpenAPI extension that I created. I agree with @anthonychu on having four life cycle events to hook into, this offers the most flexibility (and having a filter on the From a design point, 100% vote to Here's another aside though - could you use the |
I think that's out of scope for this issue, but very much should be possible with #480 |
Agreed - I'm just thinking through how a design would look and how it can feed into future planning |
@ejizba thanks for the info and correction 🙂 |
In the interest of enabling distributed tracing faster, I've split the above proposals into several separate issues. We should be able to make progress on them before the new programming model, so I think that'll still satisfy folks.
In terms of this issue, I think the goals are just:
|
since this is closed, does that mean it's possible to use these hooks today? Is there any documentation? |
Yes it is technically possible and we have sample usage at the top of this issue. That being said, we are working on an easier way to use the hooks tracked by Azure/azure-functions-nodejs-library#7 and that is the one we plan to fully document. If you have no urgency, I would suggest waiting until that issue is done |
Assuming this would be a path forward for fixing distributed tracing, this is pretty high priority... Whats ETA on the #7 release? Also, do you happen to know if modifying the function My use case is to fix the fact that service bus registerHook('preInvocation', (context: PreInvocationContext) => {
const sbTraceParent = context.bindingData.applicationProperties['diagnostic-Id'];
if (sbTraceParent) {
context.traceContext.traceparent = sbTraceParent;
}
}); |
You are correct, The point of #7 is to essentially hide all of the confusion about "built-in" modules from users. We have a real npm package I don't have the answers to your app insights questions offhand. I would suggest you file a new issue and we will investigate. Comments on closed issues are somewhat easy for us to lose track of |
Sample Usage
This has been updated to reflect the final design
NOTE: If you want to use hooks with esm, you have to import
registerHook
slightly differently:Background
Basically, users should be able to register a hook that runs a piece of code at various points in time (i.e. pre invocation and post invocation) at both the function level and app level (meaning you register it once and it runs for all functions).
The main scenario is to simplify users integrating with the Application Insights sdk for Node.js. Users can already do it, but it's a bit complicated (see docs here). If we were to add pre and post invocation hooks, most of that logic could be shifted to the sdk itself instead of the user's code. Some work was done in this PR, but the hooks were never actually exposed or usable.
Also, users could leverage these hooks for any reason (other than app insights) so IMO we need to make sure this is an "official" api. The Python team did this by adding worker extensions which we may or may not want to model after. NOTE: It would be much easier for us to support app-level hooks after the new programming model changes.
The text was updated successfully, but these errors were encountered: