feat: use trace-context "headers" in SNS and SQS single event message attributes for lambda transaction #2606

trentm · 2022-03-10T02:04:04Z

If a SNS or SQS single event trigger to an instrumented Lambda
function includes message attributes with the name "traceparent" (and
"tracestate"), case-insensitive, then those are used to continue the
trace. This was already being done for API Gateway event headers.
Also, fix a bug in the capturing of SNS and SQS message attributes as
"transaction.context.message.headers". Note that only "String"-type
message attributes are captured. Binary-type attributes are base64
encoded. I'm not sure of the value in capturing them.

Closes: #1831
Fixes: #2605
Refs: elastic/apm#614

Checklist

Implement code
Add tests
Add CHANGELOG.asciidoc entry

… attributes for lambda transaction - If a SNS or SQS single event trigger to an instrumented Lambda function includes message attributes with the name "traceparent" (and "tracestate"), case-insensitive, then those are used to continue the trace. This was already being done for API Gateway event headers. - Also, fix a bug in the capturing of SNS and SQS message attributes as "transaction.context.message.headers". Note that only "String"-type message attributes are captured. Binary-type attributes are base64 encoded. I'm not sure of the value in capturing them. Closes: #1831 Fixes: #2605 Refs: elastic/apm#614

apmmachine · 2022-03-10T02:40:43Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-03-15T16:02:59.589+0000
Duration: 21 min 30 sec

Test stats 🧪

Test	Results
Failed	0
Passed	242849
Skipped	0
Total	242849

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules
run benchmark tests : Run the benchmark test only.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

astorm

Nothing objectionable here. It looks like the thing to do and the approach seems reasonable. Happy to approve once a changelog entry gets added.

I noticed this was still in draft state -- if there's a specific part you wanted feedback on or a part you think needs more extensive testing from a second set of eyes just let me know. Otherwise 👍

astorm · 2022-03-10T19:21:57Z

lib/lambda.js

@@ -9,6 +9,32 @@ const path = require('path')
 let isFirstRun = true
 let gFaasId // Set on first invocation.

+// The trigger types for which we support special handling.
+// https://docs.aws.amazon.com/lambda/latest/dg/lambda-services.html
+const TRIGGER_GENERIC = 1


I see we've replaced the smaller functions that identify an individual payload type (ex. isApiGatewayEvent) with this larger/more-complex function triggerTypeFromEvent function that identifies an individual payload type, and then we call triggerTypeFromEvent once its result value is passed around where it needs to be. The main tradeoffs here seem to be

the perf. of multiple calls to the older small function vs. the higher memory use of keeping the constant-ish TRIGGER_ variables around.

Swapping where the complexity goes :)

No objections, just noting the change out loud.

astorm · 2022-03-10T19:21:57Z

lib/lambda.js

@@ -9,6 +9,32 @@ const path = require('path')
 let isFirstRun = true
 let gFaasId // Set on first invocation.

+// The trigger types for which we support special handling.
+// https://docs.aws.amazon.com/lambda/latest/dg/lambda-services.html
+const TRIGGER_GENERIC = 1


I see we've replaced the smaller functions that identify an individual payload type (ex. isApiGatewayEvent) with this larger/more-complex function triggerTypeFromEvent function that identifies an individual payload type, and then we call triggerTypeFromEvent once its result value is passed around where it needs to be. The main tradeoffs here seem to be

the perf. of multiple calls to the older small function vs. the higher memory use of keeping the constant-ish TRIGGER_ variables around.

Swapping where the complexity goes :)

No objections, just noting the change out loud.

astorm · 2022-03-10T19:24:19Z

lib/lambda.js

@@ -199,7 +221,10 @@ function setSqsData (agent, trans, event, context, faasId, isColdStart) {
  }

  if (agent._conf.captureHeaders) {
-    messageContext.headers = Object.assign({}, record.messageAttributes)
+    const headers = headersFromSqsMessageAttributes(record.messageAttributes)
+    if (Object.keys(headers).length > 0) {


👍 for Object.keys(obj) -- I'm pretty sure that's the fastest way to do this without needing to get into for in and hasOwnProperty territory. Unrelated to this PR/note-to-self -- a custom eslint linting rule that ensures we're using the more efficient Object.keys() for object looping/iteration (vs. one of the other myriad ways you can loop an object in javascript) would be a nice to have.

astorm · 2022-03-10T19:24:19Z

lib/lambda.js

@@ -199,7 +221,10 @@ function setSqsData (agent, trans, event, context, faasId, isColdStart) {
  }

  if (agent._conf.captureHeaders) {
-    messageContext.headers = Object.assign({}, record.messageAttributes)
+    const headers = headersFromSqsMessageAttributes(record.messageAttributes)
+    if (Object.keys(headers).length > 0) {


👍 for Object.keys(obj) -- I'm pretty sure that's the fastest way to do this without needing to get into for in and hasOwnProperty territory. Unrelated to this PR/note-to-self -- a custom eslint linting rule that ensures we're using the more efficient Object.keys() for object looping/iteration (vs. one of the other myriad ways you can loop an object in javascript) would be a nice to have.

astorm · 2022-03-10T19:25:27Z

lib/lambda.js

+//    { <attribute-name>: { dataType: <attr-type>, stringValue: <attr-value>, ... } }
+// For example:
+//    { traceparent: { dataType: 'String', stringValue: 'test-traceparent' } }
+function headersFromSqsMessageAttributes (messageAttributes) {


👍 I see a new headersFromSqsMessageAttributes that simplifies the data from amazon's strong-ishly typed JSON data into something that APM Server can handle. (I also see a separate function for SNS messages)

astorm · 2022-03-10T19:25:27Z

lib/lambda.js

+//    { <attribute-name>: { dataType: <attr-type>, stringValue: <attr-value>, ... } }
+// For example:
+//    { traceparent: { dataType: 'String', stringValue: 'test-traceparent' } }
+function headersFromSqsMessageAttributes (messageAttributes) {


👍 I see a new headersFromSqsMessageAttributes that simplifies the data from amazon's strong-ishly typed JSON data into something that APM Server can handle. (I also see a separate function for SNS messages)

astorm · 2022-03-10T19:27:41Z

lib/lambda.js

@@ -411,7 +410,12 @@ function elasticApmAwsLambda (agent) {
    }

    return function wrappedLambda (event, context, callback) {
-      log.trace({ awsRequestId: context && context.awsRequestId }, 'lambda: fn start')
+      if (!(event && context && typeof callback === 'function')) {


👍 for guarding against unknown datatypes here, per previous discussions on how things can go wrong without this.

astorm · 2022-03-10T19:27:41Z

lib/lambda.js

@@ -411,7 +410,12 @@ function elasticApmAwsLambda (agent) {
    }

    return function wrappedLambda (event, context, callback) {
-      log.trace({ awsRequestId: context && context.awsRequestId }, 'lambda: fn start')
+      if (!(event && context && typeof callback === 'function')) {


👍 for guarding against unknown datatypes here, per previous discussions on how things can go wrong without this.

astorm · 2022-03-10T19:28:42Z

lib/lambda.js

+        if (record.Sns && record.Sns.MessageAttributes) {
+          normedHeaders = lowerCaseObjectKeys(headersFromSnsMessageAttributes(record.Sns.MessageAttributes))
+        }
+      }


Not required for this PR, but I'm genuinely unsure if we should mention the agent's automatic handling of reading the trace context headers in a Lambda environment in our message queue doc, or if that would confusing things more https://github.com/elastic/apm-agent-nodejs/blob/main/docs/message-queues.asciidoc

I think it is worthwhile to mention this behaviour in the lambda.asciidoc. Likely a reference in the message-queues doc would be worthwhile as well, I'd have to re-read that doc.

astorm · 2022-03-10T19:28:42Z

lib/lambda.js

+        if (record.Sns && record.Sns.MessageAttributes) {
+          normedHeaders = lowerCaseObjectKeys(headersFromSnsMessageAttributes(record.Sns.MessageAttributes))
+        }
+      }


Not required for this PR, but I'm genuinely unsure if we should mention the agent's automatic handling of reading the trace context headers in a Lambda environment in our message queue doc, or if that would confusing things more https://github.com/elastic/apm-agent-nodejs/blob/main/docs/message-queues.asciidoc

trentm · 2022-03-10T19:59:12Z

I noticed this was still in draft state

Ah, thanks. I was going to pull out of draft after the changelog entry and merge from main.

trentm · 2022-03-10T20:44:07Z

Also there is still some discussion on the apm spec: elastic/apm#614 (comment)
So I'll wait for that to settle before finalizing this.

trentm · 2022-03-10T21:38:48Z

Trying that failed "Integration Tests" again: https://apm-ci.elastic.co/job/apm-integration-tests-selector-mbp/job/main/2446/

trentm self-assigned this Mar 10, 2022

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Mar 10, 2022

Merge branch 'main' into trentm/issue1831-tracestate

5d12a0f

trentm requested a review from astorm March 10, 2022 15:05

astorm reviewed Mar 10, 2022

View reviewed changes

trentm added 2 commits March 10, 2022 12:44

Merge branch 'main' into trentm/issue1831-tracestate

a7907b9

add a changelog entry

25abc25

trentm mentioned this pull request Mar 10, 2022

fix: if Lambda extension is down, agent eventually causes null function response #2608

Merged

4 tasks

Merge branch 'main' into trentm/issue1831-tracestate

fa29b72

trentm marked this pull request as ready for review March 15, 2022 16:06

trentm requested a review from astorm March 15, 2022 16:06

astorm approved these changes Mar 15, 2022

View reviewed changes

trentm merged commit 9f4b07b into main Mar 15, 2022

trentm deleted the trentm/issue1831-tracestate branch March 15, 2022 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use trace-context "headers" in SNS and SQS single event message attributes for lambda transaction #2606

feat: use trace-context "headers" in SNS and SQS single event message attributes for lambda transaction #2606

trentm commented Mar 10, 2022 •

edited

Loading

apmmachine commented Mar 10, 2022 •

edited

Loading

Build stats

Test stats 🧪

astorm left a comment

astorm Mar 10, 2022

astorm Mar 10, 2022

astorm Mar 10, 2022

astorm Mar 10, 2022

astorm Mar 10, 2022

astorm Mar 10, 2022

astorm Mar 10, 2022

astorm Mar 10, 2022

astorm Mar 10, 2022

trentm Mar 10, 2022

astorm Mar 10, 2022

trentm commented Mar 10, 2022

trentm commented Mar 10, 2022

trentm commented Mar 10, 2022

feat: use trace-context "headers" in SNS and SQS single event message attributes for lambda transaction #2606

feat: use trace-context "headers" in SNS and SQS single event message attributes for lambda transaction #2606

Conversation

trentm commented Mar 10, 2022 • edited Loading

Checklist

apmmachine commented Mar 10, 2022 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

astorm 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

trentm commented Mar 10, 2022

trentm commented Mar 10, 2022

trentm commented Mar 10, 2022

trentm commented Mar 10, 2022 •

edited

Loading

apmmachine commented Mar 10, 2022 •

edited

Loading