Skip to content
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

Add app-level startup hooks #577

Merged
merged 39 commits into from
Jun 28, 2022
Merged

Add app-level startup hooks #577

merged 39 commits into from
Jun 28, 2022

Conversation

hossam-nasr
Copy link
Contributor

@hossam-nasr hossam-nasr commented Apr 25, 2022

Partially implements #539. This PR builds on the work to add invocation hooks (#548) and to support app-level code (#576), to provide the ability to register app startup hooks, which are run in workerInitHandler (or functionEnvironmentReloadHandler in the specialization scenario).

In addition, some additions/decisions in this PR, which may require more discussion:

  • An app teardown hook is not provided for now, since the Host does not yet support the worker_terminate signal for elegant shutdown of the worker. This issue is currently being worker on in the host (Add support to gracefully shutdown language worker azure-functions-host#2308 and Added support to gracefully shutdown language worker azure-functions-host#8385). Once this issue is implemented, a handler for the worker_terminate gRPC message can be added to add app teardown hooks.
  • Added a logger property to HookContext, which allows all hooks, including invocation and app startup hooks, to emit user logs to app insights.
    • Pros:
      • Provides a clear and recommended way to emit logs to app insights from hooks, including app startup ones
      • Allows users to choose the log level, which will be honored in app insights
      • Includes extra metadata in the logs (such as function invocation ID for invocation hooks)
    • Cons:
      • Right now, for invocation hooks, users can already access the same functionality using hookContext.invocationContext.log. Duplicating properties is bad design.
      • If we include this property in the worker core API, it will be harder to change/remove in the future
      • Right now, logs emitted from app startup hooks will be ignored by the host, as they will be treated as "orphaned" logs, without a function invocation ID attached to them. Including this property before the Host can support logging from outside a function invocation, which will essentially do nothing, can cause confusion.
  • The AppStartupContext provided to app startup hooks also includes a hookData property. That hookData property is copied to invocation hooks' contexts' hookData property, but cannot be directly modified by each invocation.
    • Pros:
      • This hookData property can in the future be shared between app-level hooks, such as app teardown hook.
      • Allows sharing of data between app-level and invocation-level hooks
    • Cons:
      • If the data is merely copied to each invocation hook, maybe this isn't necessary, and users can just use global variables instead.

@hossam-nasr hossam-nasr changed the title Add app-level startup and teardown hooks Add app-level startup hooks Jun 3, 2022
@hossam-nasr hossam-nasr requested a review from ejizba June 3, 2022 21:22
@hossam-nasr hossam-nasr marked this pull request as ready for review June 3, 2022 21:25
src/WorkerChannel.ts Outdated Show resolved Hide resolved
src/eventHandlers/FunctionEnvironmentReloadHandler.ts Outdated Show resolved Hide resolved
src/eventHandlers/WorkerInitHandler.ts Outdated Show resolved Hide resolved
types-core/index.d.ts Outdated Show resolved Hide resolved
src/appStartup.ts Outdated Show resolved Hide resolved
src/eventHandlers/InvocationHandler.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@ejizba ejizba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we already discussed all of this offline, but just documenting what I think is left for the PR

src/eventHandlers/WorkerInitHandler.ts Outdated Show resolved Hide resolved
src/eventHandlers/InvocationHandler.ts Outdated Show resolved Hide resolved
types-core/index.d.ts Outdated Show resolved Hide resolved
src/appStartup.ts Outdated Show resolved Hide resolved
test/appStartup.test.ts Outdated Show resolved Hide resolved
test/appStartup.test.ts Outdated Show resolved Hide resolved
@hossam-nasr
Copy link
Contributor Author

Addressed most comments, should be ready for another pass :)

test/startApp.test.ts Outdated Show resolved Hide resolved
src/eventHandlers/InvocationHandler.ts Outdated Show resolved Hide resolved
src/eventHandlers/InvocationHandler.ts Outdated Show resolved Hide resolved
src/eventHandlers/WorkerInitHandler.ts Outdated Show resolved Hide resolved
src/startApp.ts Outdated Show resolved Hide resolved
types-core/index.d.ts Outdated Show resolved Hide resolved
src/eventHandlers/WorkerInitHandler.ts Outdated Show resolved Hide resolved
src/WorkerChannel.ts Outdated Show resolved Hide resolved
src/WorkerChannel.ts Outdated Show resolved Hide resolved
@hossam-nasr hossam-nasr merged commit ad99f5d into v3.x Jun 28, 2022
@hossam-nasr hossam-nasr deleted the hossamnasr/app-hooks branch June 28, 2022 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants