[SVLS-4699] Detect exceptions raised outside of the handler #533

TalUsvyatsky · 2024-05-02T17:16:13Z

What does this PR do?

This PR adds telemetry for exceptions that occur outside of the handler function.

When an exception occurs outside of the handler function:

aws.lambda.enhanced.errors is emitted
A trace with an error span is created

Motivation

SVLS-4699
Users should have visibility into this category of errors
We did the same thing in Python in Detect exceptions raised outside of the handler datadog-lambda-python#475

Testing Guidelines

Created a handler file like this for testing "cjs" handler:

  throw new TypeError("im a type error")
  exports.handler = function async (event) {
    const response = {
      statusCode: 200,
      body: JSON.stringify('Hello from Lambda!'),
    };
    return response;
  };

and one like this for testing "mjs" handler:

  throw new TypeError("im a type error")
  export const handler = async (event) => {
    const response = {
      statusCode: 200,
      body: JSON.stringify('Hello from Lambda!'),
    };
    return response;
  };

Deployed my function using each handler and invoked it
Saw traces with an error span from the call (example trace)
Saw aws.lambda.enhanced.errors emitted (graph)
Traces appear in serverless detail page invocation table (link to search for function)
Issues appear in error tracking and are informative

Additional Notes

Types of Changes

Bug fix
New feature
Breaking change
Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

This PR's description is comprehensive
This PR contains breaking changes that are documented in the description
This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
This PR impacts documentation, and it has been updated (or a ticket has been logged)
This PR's changes are covered by the automated tests
This PR collects user input/sensitive content into Datadog
This PR passes the integration tests (ask a Datadog member to run the tests)

codecov-commenter · 2024-05-02T18:22:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.10%. Comparing base (78c1900) to head (249712e).
Report is 8 commits behind head on main.

❗ Current head 249712e differs from pull request most recent head 75ae1c8. Consider uploading reports for the commit 75ae1c8 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
+ Coverage   81.75%   82.10%   +0.35%     
==========================================
  Files          54       56       +2     
  Lines        2209     2286      +77     
  Branches      515      529      +14     
==========================================
+ Hits         1806     1877      +71     
- Misses        337      344       +7     
+ Partials       66       65       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…d outside of the handler function

joeyzhao2018 · 2024-05-07T18:47:35Z

src/handler.cjs

+try {
+  exports.handler = datadog(loadSync(taskRootEnv, handlerEnv), { traceExtractor });
+} catch (error) {
+  emitTelemetryOnErrorOutsideHandler(error, handlerEnv, Date.now());


I might be wrong but is this fire and forget way of invoking the async method? I'm just wondering if we need to add a.catch(error => { // something like logDebug(error) });

thanks! yes I think you're right, added

purple4reina · 2024-05-07T18:50:08Z

src/handler.mjs

@@ -26,4 +33,12 @@ if (extractorEnv) {
  }
 }

-export const handler = datadog(await load(taskRootEnv, handlerEnv), { traceExtractor });
+let wrapped_handler;


nit: use camel case wrappedHandler

purple4reina · 2024-05-07T18:55:48Z

src/metrics/enhanced-metrics.ts

+    if (context.invokedFunctionArn) {
+      arnTags = parseTagsFromARN(context.invokedFunctionArn, context.functionVersion);
+    }
+    tags = [...arnTags, `memorysize:${context.memoryLimitInMB}`];


Does js have an append-like function? so we don't have to do the extra allocation.

duncanista

Left some comments.

duncanista · 2024-05-07T21:37:42Z

src/index.ts

+  const metricsListener = new MetricsListener(new KMSService(), config);
+  await metricsListener.onStartInvocation(undefined);
+  if (config.enhancedMetrics) {
+    incrementErrorsMetric(metricsListener);
+  }
+  await metricsListener.onCompleteInvocation();


Why don't we just lazy load everything inside of the condition?

Good idea! Updated

src/metrics/enhanced-metrics.ts

TalUsvyatsky force-pushed the tal.usvyatsky/capture-exceptions-outside-of-handler branch 4 times, most recently from c96415d to a0c2e24 Compare May 2, 2024 17:59

TalUsvyatsky force-pushed the tal.usvyatsky/capture-exceptions-outside-of-handler branch 7 times, most recently from ef5ba7e to e0259a0 Compare May 6, 2024 14:42

TalUsvyatsky added 2 commits May 7, 2024 11:13

emit enhanced error metric and create span when an exception is raise…

431707b

…d outside of the handler function

add unit tests

02e9ae7

TalUsvyatsky force-pushed the tal.usvyatsky/capture-exceptions-outside-of-handler branch from 375112c to 02e9ae7 Compare May 7, 2024 15:13

add missing await and move metrics block

050662c

TalUsvyatsky force-pushed the tal.usvyatsky/capture-exceptions-outside-of-handler branch from 5a9d85c to 050662c Compare May 7, 2024 17:24

TalUsvyatsky marked this pull request as ready for review May 7, 2024 17:42

TalUsvyatsky requested a review from a team as a code owner May 7, 2024 17:42

test getEnhancedMetricTags with no context

4b441d3

joeyzhao2018 reviewed May 7, 2024

View reviewed changes

purple4reina reviewed May 7, 2024

View reviewed changes

fix casing

56f21e4

purple4reina reviewed May 7, 2024

View reviewed changes

push new tags onto existing array

f14e402

TalUsvyatsky force-pushed the tal.usvyatsky/capture-exceptions-outside-of-handler branch from c04d253 to f14e402 Compare May 7, 2024 19:02

handle promise rejection

5365237

joeyzhao2018 approved these changes May 7, 2024

View reviewed changes

duncanista reviewed May 7, 2024

View reviewed changes

move all listener code inside condition

75ae1c8

TalUsvyatsky requested a review from duncanista May 8, 2024 19:26

duncanista approved these changes May 8, 2024

View reviewed changes

TalUsvyatsky merged commit 6c0447d into main May 9, 2024
25 checks passed

TalUsvyatsky deleted the tal.usvyatsky/capture-exceptions-outside-of-handler branch May 9, 2024 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SVLS-4699] Detect exceptions raised outside of the handler #533

[SVLS-4699] Detect exceptions raised outside of the handler #533

TalUsvyatsky commented May 2, 2024 •

edited

Loading

codecov-commenter commented May 2, 2024 •

edited

Loading

joeyzhao2018 May 7, 2024 •

edited

Loading

TalUsvyatsky May 7, 2024

purple4reina May 7, 2024

purple4reina May 7, 2024

duncanista left a comment

duncanista May 7, 2024

TalUsvyatsky May 8, 2024

[SVLS-4699] Detect exceptions raised outside of the handler #533

[SVLS-4699] Detect exceptions raised outside of the handler #533

Conversation

TalUsvyatsky commented May 2, 2024 • edited Loading

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

Types of Changes

Check all that apply

codecov-commenter commented May 2, 2024 • edited Loading

Codecov Report

joeyzhao2018 May 7, 2024 • edited Loading

Choose a reason for hiding this comment

TalUsvyatsky May 7, 2024

Choose a reason for hiding this comment

purple4reina May 7, 2024

Choose a reason for hiding this comment

purple4reina May 7, 2024

Choose a reason for hiding this comment

duncanista left a comment

Choose a reason for hiding this comment

duncanista May 7, 2024

Choose a reason for hiding this comment

TalUsvyatsky May 8, 2024

Choose a reason for hiding this comment

TalUsvyatsky commented May 2, 2024 •

edited

Loading

codecov-commenter commented May 2, 2024 •

edited

Loading

joeyzhao2018 May 7, 2024 •

edited

Loading