Skip to content

feature: add before/after InvocationRequest hooks #339

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

Merged
merged 4 commits into from
Oct 26, 2020

Conversation

markwolff
Copy link

Copied from #338

Adds two InvocationRequest hooks to WorkerChannel to enable context correlation and node.js worker APM instrumentation. Sample use case for Azure Monitor node.js SDK.

  • invocationRequestBefore: (context, userFunction) => contextWrappedUserFunction
  • invocationRequestAfter: () => void

note: workerChannel would need to be passed to app insights in some way, i.e. in nodejsWorker.ts:

if (shouldSetupAppInsightsFunctionsIntegration) {
  require("/path/to/applicationinsights/agent/functions")
    .setupFunctionsInvocationHooks(workerChannel);
}

In App Insights SDK, setupFunctionsInvocationHooks would be defined:

workerChannel.invocationRequestBefore = (context, userFunction) => {
    // Map Functions context to AppInsights context
    const correlationContext = appInsights.startOperation(context, "Functions.JavaScript.Trigger");

    // Wrap the Function runtime with correlationContext
    return appInsights.wrapWithCorrelationContext(userFunction, correlationContext);
}

workerChannel.invocationRequestAfter = () => {
    // Do any extra cleanup or flushing here
    appInsights.defaultClient.flush(); // can be async or sync
}

@markwolff markwolff requested a review from mhoeger October 19, 2020 21:55
@mhoeger
Copy link
Contributor

mhoeger commented Oct 20, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@mhoeger
Copy link
Contributor

mhoeger commented Oct 23, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@mhoeger mhoeger left a comment

Choose a reason for hiding this comment

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

✅ LGTM - on registering async functions, since it's not like a one line change, let's just cross that bridge when we get there.

Thanks!!

@mhoeger mhoeger merged commit 0ff6fb4 into dev Oct 26, 2020
mhoeger pushed a commit to mhoeger/azure-functions-nodejs-worker that referenced this pull request Oct 26, 2020
* feature: add before/after InvocationRequest hooks

* rm return workerChannel

* refactor to register cb pattern

* remove double callback on promise resolve
mhoeger pushed a commit to mhoeger/azure-functions-nodejs-worker that referenced this pull request Oct 26, 2020
* feature: add before/after InvocationRequest hooks

* rm return workerChannel

* refactor to register cb pattern

* remove double callback on promise resolve
mhoeger added a commit that referenced this pull request Oct 27, 2020
* feature: add before/after InvocationRequest hooks (#339)

* feature: add before/after InvocationRequest hooks

* rm return workerChannel

* refactor to register cb pattern

* remove double callback on promise resolve

* revert changes

* Re-added before / after invocation tests refactored for this branch

Co-authored-by: Mark Wolff <[email protected]>
@mhoeger mhoeger deleted the feature/add-invocationrequest-hooks branch October 27, 2020 00:20
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