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

feat: pre-register partial transactions in AWS Lambda #3285

Merged
merged 13 commits into from Apr 28, 2023

Conversation

trentm
Copy link
Member

@trentm trentm commented Apr 25, 2023

When used with the Lambda extension >=v1.4.0, this results in
transactions being reported for Lambda timeout, uncaughtException,
and unhandledRejection.

Closes: #3136
Closes: #2379

checklist

follow-up work

When used with the Lambda extension >=v1.4.0, this results in
transactions being reported for Lambda timeout, uncaughtException,
and unhandledRejection.

Closes: #3136
Closes: #2379
@trentm trentm self-assigned this Apr 25, 2023
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Apr 25, 2023
@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Apr 25, 2023
…nager=patch setting

The use of an async wrapper function and awaiting something breaks the
async context for the `patch` context manager.
…ontextManager=patch setting"

This reverts commit 785732e.
…limitation

The usage of `agent._instrumentation` breaks a lot of test code because
of the AgentMock. A follow-up change will fix this all up (dropping
usage of AgentMock). I'll do this separately to keep this change more
focused
…an 'await Promise' for node v10.0-10.3 inclusive
@trentm trentm marked this pull request as ready for review April 26, 2023 00:23
@trentm
Copy link
Member Author

trentm commented Apr 26, 2023

A note on support for contextManager: patch usage

To support the "synchronously pre-register a transaction with the Lambda extension" behaviour change, were are necessarily using Promises and await ... in the wrappedLambdaHandler instrumentation now. This breaks async context tracking for the contextManager: patch config option usage. The "patch" context manager is deprecated, so this isn't considered a big deal. However, it can be supported again via this diff:

diff --git a/lib/lambda.js b/lib/lambda.js
index 870c0b4f..df27d71a 100644
--- a/lib/lambda.js
+++ b/lib/lambda.js
@@ -438,6 +438,7 @@ function setS3SingleData (trans, event, context, faasId, isColdStart) {

function elasticApmAwsLambda (agent) {
  const log = agent.logger
+  const ins = agent._instrumentation

  /**
    * Register this transaction with the Lambda extension, if possible.  This
@@ -644,13 +645,14 @@ function elasticApmAwsLambda (agent) {
      // Note: Wrapping context needs to happen *before any `await` calls* in
      // this function, otherwise the Lambda Node.js Runtime will call the
      // *unwrapped* `context.{succeed,fail,done}()` methods.
+      const runContext = ins.currRunContext()
      wrapContext(trans, event, context, triggerType)
      const wrappedCallback = wrapLambdaCallback(trans, event, context, triggerType, callback)

      await registerTransaction(trans, context.awsRequestId)

      try {
-        var retval = fn.call(this, event, context, wrappedCallback)
+        const retval = ins.withRunContext(runContext, fn, this, event, context, wrappedCallback)
        if (retval instanceof Promise) {
          return retval
        } else {

or similar.

The issue with applying that diff right now is that it adds usage of agent._instrumentation in this instrumentation, which is good and fine, but breaks the AgentMock being used in a large subset of test/lambda/*.test.js. It'll involve either another fake bandaid to AgentMock to have it start simulating a subset of agent._instrumentation.* behaviour or hundreds of lines of changes to the lambda tests to remove usage of AgentMock. I want to do the latter.

However, I want to do the latter in a separate change to not add noise to this change.

@trentm trentm merged commit d19871b into main Apr 28, 2023
33 checks passed
APM-Agents (OLD) automation moved this from In Progress to Done Apr 28, 2023
@trentm trentm deleted the trentm/lambda-partial-trans branch April 28, 2023 21:53
@trentm
Copy link
Member Author

trentm commented May 2, 2023

However, I want to do the latter in a separate change to not add noise to this change.

#3305 for this

trentm added a commit that referenced this pull request May 3, 2023
…factor Lambda tests (#3305)

Changes in #3285 broke Lambda instrumentation if the deprecated
`contextManager:'patch' config var was used. This restores support for that, FWIW.

Also, refactoring so we can drop AgentMock and TransactionMock under 
test/lambda/... to simplify:
- roll promises.test.js into the main test file
- roll trace-context.test.js into the main lambda test file
- roll transaction.test.js into main lambda test file
- rename main lambda test file to lambda.test.js
- drop now unused MockAgent
trentm added a commit that referenced this pull request May 16, 2023
When used with the Lambda extension >=v1.4.0, this results in
transactions being reported for Lambda timeout, uncaughtException,
unhandledRejection, crashes.

Closes: #3136
Closes: #2379
trentm added a commit that referenced this pull request May 16, 2023
…factor Lambda tests (#3305)

Changes in #3285 broke Lambda instrumentation if the deprecated
`contextManager:'patch' config var was used. This restores support for that, FWIW.

Also, refactoring so we can drop AgentMock and TransactionMock under 
test/lambda/... to simplify:
- roll promises.test.js into the main test file
- roll trace-context.test.js into the main lambda test file
- roll transaction.test.js into main lambda test file
- rename main lambda test file to lambda.test.js
- drop now unused MockAgent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
1 participant