fix(@aws-sdk/client-sns): do not crash if there is no current transac…

…tion (elastic#4168) Before this change, instrumentation of `@aws-sdk/client-sns` would *crash* if there wasn't a current transaction. Fixes: elastic#4138
fpm-git · Aug 20, 2024 · 5de84f1 · 5de84f1
1 parent 2c3d925
commit 5de84f1
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 26 deletions.
diff --git a/lib/instrumentation/modules/@aws-sdk/client-sns.js b/lib/instrumentation/modules/@aws-sdk/client-sns.js
@@ -36,28 +36,30 @@ function snsMiddlewareFactory(client, agent) {
         const targetId =
           input && (input.TopicArn || input.TargetArn || input.PhoneNumber);
 
-        // Though our spec only mentions a 10-message-attribute limit for *SQS*, we'll
-        // do the same limit here, because
-        // https://docs.aws.amazon.com/sns/latest/dg/sns-message-attributes.html
-        // mentions the 10-message-attribute limit for SQS subscriptions.
-        input.MessageAttributes = input.MessageAttributes || {};
-        const attributesCount = Object.keys(input.MessageAttributes).length;
-
-        if (attributesCount + 2 > MAX_SNS_MESSAGE_ATTRIBUTES) {
-          log.warn(
-            'cannot propagate trace-context with SNS message to %s, too many MessageAttributes',
-            targetId,
-          );
-        } else {
-          parentSpan.propagateTraceContextHeaders(
-            input.MessageAttributes,
-            function (msgAttrs, name, value) {
-              if (name.startsWith('elastic-')) {
-                return;
-              }
-              msgAttrs[name] = { DataType: 'String', StringValue: value };
-            },
-          );
+        if (parentSpan) {
+          // Though our spec only mentions a 10-message-attribute limit for *SQS*, we'll
+          // do the same limit here, because
+          // https://docs.aws.amazon.com/sns/latest/dg/sns-message-attributes.html
+          // mentions the 10-message-attribute limit for SQS subscriptions.
+          input.MessageAttributes = input.MessageAttributes || {};
+          const attributesCount = Object.keys(input.MessageAttributes).length;
+
+          if (attributesCount + 2 > MAX_SNS_MESSAGE_ATTRIBUTES) {
+            log.warn(
+              'cannot propagate trace-context with SNS message to %s, too many MessageAttributes',
+              targetId,
+            );
+          } else {
+            parentSpan.propagateTraceContextHeaders(
+              input.MessageAttributes,
+              function (msgAttrs, name, value) {
+                if (name.startsWith('elastic-')) {
+                  return;
+                }
+                msgAttrs[name] = { DataType: 'String', StringValue: value };
+              },
+            );
+          }
         }
 
         // Ensure there is a span from the wrapped `client.send()`.

diff --git a/test/instrumentation/modules/@aws-sdk/client-sns.test.js b/test/instrumentation/modules/@aws-sdk/client-sns.test.js
@@ -235,6 +235,24 @@ const testFixtures = [
       t.equal(spans.length, 0, 'all spans accounted for');
     },
   },
+
+  {
+    name: 'use-client-sns.js without a created transaction to ensure does not crash',
+    script: 'fixtures/use-client-sns.js',
+    cwd: __dirname,
+    env: {
+      AWS_ACCESS_KEY_ID: 'fake',
+      AWS_SECRET_ACCESS_KEY: 'fake',
+      TEST_ENDPOINT: endpoint,
+      TEST_REGION: 'us-east-2',
+      TEST_NO_TRANSACTION: 'true',
+    },
+    versionRanges: {
+      node: '>=14',
+    },
+    verbose: true,
+  },
+
   {
     name: '@aws-sdk/client-sns ESM',
     script: 'fixtures/use-client-sns.mjs',

diff --git a/test/instrumentation/modules/@aws-sdk/fixtures/use-client-sns.js b/test/instrumentation/modules/@aws-sdk/fixtures/use-client-sns.js
@@ -225,6 +225,9 @@ function main() {
   const endpoint = process.env.TEST_ENDPOINT || null;
   const topicName =
     process.env.TEST_TOPIC_NAME || TEST_TOPIC_NAME_PREFIX + getTimestamp();
+  // `TEST_NO_TRANSACTION=true` can be used to disable the creating of a
+  // current transaction for SNS calls; useful only for testing.
+  const noTransaction = process.env.TEST_NO_TRANSACTION === 'true';
 
   // Guard against any topic name being used because we will be publishing
   // messages in it, and potentially *deleting* the topic.
@@ -240,18 +243,25 @@ function main() {
   });
 
   // Ensure an APM transaction so spans can happen.
-  const tx = apm.startTransaction('manual');
+  let tx = null;
+  if (!noTransaction) {
+    tx = apm.startTransaction('manual');
+  }
 
   useClientSNS(snsClient, topicName).then(
     function () {
-      tx.end();
+      if (tx) {
+        tx.end();
+      }
       snsClient.destroy();
       process.exitCode = 0;
     },
     function (err) {
       apm.logger.error(err, 'useClientSNS rejected');
-      tx.setOutcome('failure');
-      tx.end();
+      if (tx) {
+        tx.setOutcome('failure');
+        tx.end();
+      }
       snsClient.destroy();
       process.exitCode = 1;
     },