feat: add instrumentation of AWS Lambda handlers to generate FaaS spans #399

anuraaga · 2021-03-30T07:44:46Z

Which problem is this PR solving?

Allow AWS Lambda users to have spans generated for function invocations

Short description of the changes

This instrumentation patches the AWS Lambda handler (found via the _HANDLER env variable which is always set by Lambda) to generate spans around invocations. It is the first in a series of PRs implementing the spec defined here

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/instrumentation/aws-lambda.md

This currently only decorates the invocations with spans following FaaS conventions - future PRs will add reading of HTTP for proxy requests and messaging for SQS requests with appropriate parenting.

I actually am not sure if this approach works I will try it out with an actual lambda function, sending out for an initial look at the code and maybe suggestions on how to work around this problem. I noticed that the patching infrastructure, via require-in-middle can only patch core modules or modules in node_modules - it literally checks the path of a required file to see if it is in node_modules and bails if it isn't.

https://github.com/watson/module-details-from-path/blob/master/index.js#L8

I have worked around it in testing here by copying handlers into node_modules - I need to check whether Lambda actually houses a user handler function inside a folder called node_modules. There's a reasonable chance it doesn't - if anyone can provide some tips on whether it's possible to work around this, that would be great.

anuraaga · 2021-03-30T07:45:14Z

@willarmiros @wangzlei

anuraaga · 2021-04-01T06:53:54Z

I found how lambda requires modules (absolute path) and the way to get it to work with the patching infrastructure open-telemetry/opentelemetry-js#2069

anuraaga

@dyladan @willarmiros Updated, now the tests pass even with the HEAD opentelemetry-js code.

anuraaga · 2021-04-06T07:33:48Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+
+    return [
+      new InstrumentationNodeModuleDefinition(
+        // NB: The patching infrastructure seems to match names backwards, this must be the filename, while


I found this strange pattern that did allow patching the lambda handler.

I'm not sure what you meant by matching name backwards ? It matches absolute path (https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts#L107)

Yeah, I mean that we pass the filename to a class called InstrumentationNodeModuleDefinition, but pass a module name to a class called InstrumentationNodeModuleFile. I'd expect file to refer to a filename :)

Yeah thats weird, i'm not sure if it's because of the resolving done by lamda. With other instrumentations we do the opposite (module is a name and file is an relative path to the module): https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-mongodb/src/mongodb.ts#L59

this indeed looks strange as it should be a module name if user requires for example http then the module name is http so in this case it should be aws-lambda not a file name. If you need to patch some additional files then this is what InstrumentationNodeModuleFile is for

Yeah that's what I figured. Well I'd like to leave this as is for now since it seems to be the pattern that works here :)

…trib into awslambda-faas

codecov · 2021-04-06T07:44:10Z

Codecov Report

Merging #399 (14f0f8a) into main (c7fb34b) will not change coverage.
The diff coverage is n/a.

❗ Current head 14f0f8a differs from pull request most recent head c42d413. Consider uploading reports for the commit c42d413 to get more accurate results

@@           Coverage Diff           @@
##             main     #399   +/-   ##
=======================================
  Coverage   94.21%   94.21%           
=======================================
  Files          12       12           
  Lines         432      432           
  Branches       48       48           
=======================================
  Hits          407      407           
  Misses         25       25

plugins/node/opentelemetry-instrumentation-awslambda/package.json

plugins/node/opentelemetry-instrumentation-awslambda/README.md

…e it always is

anuraaga · 2021-04-07T08:16:02Z

By the way I could confirm this instrumentation works to instrumentent lambda invocations by building into a lambda layer in open-telemetry/opentelemetry-lambda#25

vmarchaud

lgtm

obecny

Please change names everywhere from awslambda to aws-lambda including the folder name

obecny · 2021-04-07T13:10:39Z

plugins/node/opentelemetry-instrumentation-awslambda/package.json

@@ -0,0 +1,71 @@
+{
+  "name": "@opentelemetry/instrumentation-awslambda",


I think the name should be

Suggested change

"name": "@opentelemetry/instrumentation-awslambda",

"name": "@opentelemetry/instrumentation-aws-lambda",

you are patching aws-lambda and those are 2 words anyway

obecny · 2021-04-07T13:18:25Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+        () => original.apply(this, [event, context, wrappedCallback]),
+        error => {
+          if (error != null) {
+            span.setStatus({


use also span.recordException to save the whole stack of error

obecny · 2021-04-07T13:18:40Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+              errMessage = err.message;
+            }
+            if (errMessage) {
+              span.setStatus({


span.recordException

recordException I don't think sets status does it? You may need to do both if that is the behavior you want.

obecny · 2021-04-07T13:33:34Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+
+    return [
+      new InstrumentationNodeModuleDefinition(
+        // NB: The patching infrastructure seems to match names backwards, this must be the filename, while


this indeed looks strange as it should be a module name if user requires for example http then the module name is http so in this case it should be aws-lambda not a file name. If you need to patch some additional files then this is what InstrumentationNodeModuleFile is for

obecny · 2021-04-07T13:34:23Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+        attributes: {
+          [FaasAttribute.FAAS_EXECUTION]: context.awsRequestId,
+          [FaasAttribute.FAAS_ID]: context.invokedFunctionArn,
+          // TODO(anuraaga): Add cloud.account.id when there is a JS accessor for it.


can you please create a ticket for that so it is not lost ?

Can this not be extracted from the ARN?

+1 to getting account from ARN

obecny · 2021-04-07T13:38:14Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+    return function wrappedCallback(this: {}, err, res) {
+      diag.debug('executing wrapped lookup callback function');
+
+      let errMessage;


this part of code is the same as in line 135. Please create a private method for that something like
this._addError(span)
please also use recordException if you have an object error.

obecny · 2021-04-07T13:39:55Z

plugins/node/opentelemetry-instrumentation-awslambda/src/types.ts

+
+import { Handler } from 'aws-lambda';
+
+export interface LambdaModule {


Suggested change

export interface LambdaModule {

export type LambdaModule = Record<string, Handler>;

obecny · 2021-04-07T13:41:52Z

plugins/node/opentelemetry-instrumentation-awslambda/test/lambdatest/package.json

@@ -0,0 +1,5 @@
+{
+  "name": "lambdatest",


Suggested change

"name": "lambdatest",

"name": "lambda-test",

obecny · 2021-04-07T13:44:36Z

plugins/node/opentelemetry-instrumentation-awslambda/test/integrations/lambda-handler.test.ts

+    });
+  });
+
+  context('sync success handler', () => {


please add block describe('sync success handler') and similar block to async

dyladan

How are you handling flushing? Are you just hoping that the lambda runtime isn't paused at an inconvenient time?

vmarchaud · 2021-04-07T15:54:48Z

Please change names everywhere from awslambda to aws-lambda including the folder name

Is this a requirement that i missed for vendor components or just a better naming suggestion ?

obecny · 2021-04-07T15:58:23Z

aws-lambda

better naming and consistency, aws-lambda is the official name of the package so I think we should keep the exact name as we do with other things. It is also 2 words so one more reason for having a dash like we do with everything else with regards to naming.

willarmiros

LGTM in general

willarmiros · 2021-04-07T23:37:32Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+        attributes: {
+          [FaasAttribute.FAAS_EXECUTION]: context.awsRequestId,
+          [FaasAttribute.FAAS_ID]: context.invokedFunctionArn,
+          // TODO(anuraaga): Add cloud.account.id when there is a JS accessor for it.


+1 to getting account from ARN

willarmiros · 2021-04-07T23:44:01Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+      });
+
+      // Lambda seems to pass a callback even if handler is of Promise form, so we wrap all the time before calling
+      // the handler and see if the result is a Promise or not. In such a case, the callback is usually ignored.


In such a case, the callback is usually ignored.

What if the callback isn't ignored? Would the span be ended twice/would that matter?

Yeah anything after the first end is ignored as per otel spec

anuraaga

Thanks all applied cleanups

anuraaga · 2021-04-08T05:55:33Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+
+    return [
+      new InstrumentationNodeModuleDefinition(
+        // NB: The patching infrastructure seems to match names backwards, this must be the filename, while


Yeah that's what I figured. Well I'd like to leave this as is for now since it seems to be the pattern that works here :)

anuraaga · 2021-04-08T05:56:37Z

plugins/node/opentelemetry-instrumentation-awslambda/src/awslambda.ts

+      });
+
+      // Lambda seems to pass a callback even if handler is of Promise form, so we wrap all the time before calling
+      // the handler and see if the result is a Promise or not. In such a case, the callback is usually ignored.


Yeah anything after the first end is ignored as per otel spec

…trib into awslambda-faas

anuraaga · 2021-04-08T06:33:05Z

How are you handling flushing? Are you just hoping that the lambda runtime isn't paused at an inconvenient time?

Thanks a lot for the reminder, added a forceFlush at the end of the function invocation.

plugins/node/opentelemetry-instrumentation-aws-lambda/src/aws-lambda.ts

plugins/node/opentelemetry-instrumentation-aws-lambda/package.json

anuraaga

Thanks @Flarna - also, do you happen to know how to ignore a test on nodejs 8 in CI? If it's not easy I'm thinking of just going with then(() => callback(), () => callback())

Flarna · 2021-04-08T08:02:06Z

do you happen to know how to ignore a test on nodejs 8 in CI?

Sorry, don't know. One simple solution would be to call this.skip() in the test suite for old node versions. But maybe there is a better solution.

anuraaga · 2021-04-08T08:12:57Z

@Flarna Thanks! Skipping sounds good

plugins/node/opentelemetry-instrumentation-aws-lambda/README.md

plugins/node/opentelemetry-instrumentation-aws-lambda/package.json

plugins/node/opentelemetry-instrumentation-aws-lambda/src/aws-lambda.ts

plugins/node/opentelemetry-instrumentation-aws-lambda/README.md

Co-authored-by: Bartlomiej Obecny <[email protected]>

anuraaga · 2021-04-08T23:17:26Z

@obecny >< Thanks for the help on cleanup!

…trib into awslambda-faas

anuraaga · 2021-04-12T03:04:39Z

@obecny I ended up switching from finally to .then(repeat, repeat) because skipping the test caused code coverage to not be computed correctly. Hopefully it's not a big deal.

I have a couple of important followups planned, including implementing the parenting mechanism from the spec and HTTP attributes for proxy events. If this looks OK appreciate a merge :)

…ns (open-telemetry#399) Co-authored-by: Bartlomiej Obecny <[email protected]>

feat: add instrumentation of AWS Lambda handlers to generate FaaS spans

e892c90

anuraaga requested a review from a team March 30, 2021 07:44

anuraaga mentioned this pull request Apr 1, 2021

feat: allow patching of absolute path requires open-telemetry/opentelemetry-js#2069

Closed

fix: use absolute path require

d66cc93

anuraaga force-pushed the awslambda-faas branch from 80cff44 to d66cc93 Compare April 6, 2021 07:33

anuraaga commented Apr 6, 2021

View reviewed changes

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

fa04748

…trib into awslambda-faas

vmarchaud reviewed Apr 6, 2021

View reviewed changes

Anuraag Agrawal added 3 commits April 7, 2021 15:13

chore: cleanup

99bc28e

fix: readme

6f7e0d7

fix: check handler result instead of checking callback presence, sinc…

3354fab

…e it always is

vmarchaud approved these changes Apr 7, 2021

View reviewed changes

vmarchaud requested review from willarmiros, obecny and dyladan April 7, 2021 08:24

obecny reviewed Apr 7, 2021

View reviewed changes

dyladan reviewed Apr 7, 2021

View reviewed changes

willarmiros approved these changes Apr 7, 2021

View reviewed changes

fix: cleanups

86b1db9

anuraaga commented Apr 8, 2021

View reviewed changes

Anuraag Agrawal added 2 commits April 8, 2021 15:18

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

0be2fd5

…trib into awslambda-faas

fix: forceFlush

a2438d6

Flarna reviewed Apr 8, 2021

View reviewed changes

fix: cleanups and restrict node version

be096d0

Flarna reviewed Apr 8, 2021

View reviewed changes

plugins/node/opentelemetry-instrumentation-aws-lambda/package.json Outdated Show resolved Hide resolved

plugins/node/opentelemetry-instrumentation-aws-lambda/package.json Outdated Show resolved Hide resolved

anuraaga commented Apr 8, 2021

View reviewed changes

fix: fix version specifiers

4d3d6a2

fix: skip tests on node 8

cdb2079

fix: return?

0c1f151

obecny reviewed Apr 8, 2021

View reviewed changes

Apply suggestions from code review

e5f9244

Co-authored-by: Bartlomiej Obecny <[email protected]>

obecny approved these changes Apr 9, 2021

View reviewed changes

Anuraag Agrawal added 2 commits April 12, 2021 11:35

Merge branch 'main' of github.com:open-telemetry/opentelemetry-js-con…

18a6ad0

…trib into awslambda-faas

fix: remove finally

8f69d9b

fix: process.env restoration

c42d413

vmarchaud merged commit 1381a9a into open-telemetry:main Apr 12, 2021

obecny added a commit to obecny/opentelemetry-js-contrib that referenced this pull request Apr 15, 2021

feat: add instrumentation of AWS Lambda handlers to generate FaaS spa…

a97bd46

…ns (open-telemetry#399) Co-authored-by: Bartlomiej Obecny <[email protected]>

wangzlei mentioned this pull request Apr 18, 2021

NodeJS 12.x environment support for auto instrumentation aws-observability/aws-otel-lambda#18

Closed

willarmiros mentioned this pull request May 18, 2021

crash when using aws-lambda instrumentation open-telemetry/opentelemetry-js#2193

Closed

		@@ -0,0 +1,71 @@
		{
		"name": "@opentelemetry/instrumentation-awslambda",


		import { Handler } from 'aws-lambda';

		export interface LambdaModule {

	export interface LambdaModule {
	export type LambdaModule = Record<string, Handler>;

feat: add instrumentation of AWS Lambda handlers to generate FaaS spans #399

feat: add instrumentation of AWS Lambda handlers to generate FaaS spans #399

Conversation

anuraaga commented Mar 30, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

anuraaga commented Mar 30, 2021

anuraaga commented Apr 1, 2021

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmarchaud Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 6, 2021 • edited Loading

Codecov Report

anuraaga commented Apr 7, 2021

vmarchaud left a comment

Choose a reason for hiding this comment

obecny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan left a comment

Choose a reason for hiding this comment

vmarchaud commented Apr 7, 2021

obecny commented Apr 7, 2021

willarmiros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga commented Apr 8, 2021

anuraaga left a comment

Choose a reason for hiding this comment

Flarna commented Apr 8, 2021

anuraaga commented Apr 8, 2021

anuraaga commented Apr 8, 2021

anuraaga commented Apr 12, 2021

anuraaga commented Mar 30, 2021 •

edited

Loading

vmarchaud Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 6, 2021 •

edited

Loading