fix(instrumentation-aws-sdk): remove un-sanitised db.statement span attribute from DynamoDB spans (#1748)

ramesius · blumamir · web-flow · commit cdbb29f03421 · 2023-12-07T10:12:47.000+02:00
* feat: add configuration to customise dynamodb statement serialization

* fix: format readme

* chore: pass DiagLogger to DynamoDB instrumentation

* chore: omit db statement when serializer is not configured or when it returned undefined
Allow for passing operation to serializer

* chore: run lint:fix

---------

Co-authored-by: Amir Blum &lt;amirgiraffe@gmail.com&gt;
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/README.md b/plugins/node/opentelemetry-instrumentation-aws-sdk/README.md
@@ -20,11 +20,11 @@ npm install --save @opentelemetry/instrumentation-aws-sdk
 For further automatic instrumentation instruction see the [@opentelemetry/instrumentation](https://www.npmjs.com/package/@opentelemetry/instrumentation) package.
 
 ```js
-const { NodeTracerProvider } = require("@opentelemetry/sdk-trace-node");
-const { registerInstrumentations } = require("@opentelemetry/instrumentation");
+const { NodeTracerProvider } = require('@opentelemetry/sdk-trace-node');
+const { registerInstrumentations } = require('@opentelemetry/instrumentation');
 const {
   AwsInstrumentation,
-} = require("@opentelemetry/instrumentation-aws-sdk");
+} = require('@opentelemetry/instrumentation-aws-sdk');
 
 const provider = new NodeTracerProvider();
 provider.register();
@@ -42,13 +42,14 @@ registerInstrumentations({
 
 aws-sdk instrumentation has few options available to choose from. You can set the following:
 
-| Options                           | Type                                      | Description                                                                                                                |
-| --------------------------------- | ----------------------------------------- | -------------------------------------------------------------------------------------------------------------------------- |
-| `preRequestHook`                  | `AwsSdkRequestCustomAttributeFunction`    | Hook called before request send, which allow to add custom attributes to span.                                             |
-| `responseHook`                    | `AwsSdkResponseCustomAttributeFunction`   | Hook for adding custom attributes when response is received from aws.                                                      |
-| `sqsProcessHook`                  | `AwsSdkSqsProcessCustomAttributeFunction` | Hook called after starting sqs `process` span (for each sqs received message), which allow to add custom attributes to it. |
-| `suppressInternalInstrumentation` | `boolean`                                 | Most aws operation use http requests under the hood. Set this to `true` to hide all underlying http spans.                 |
-| `sqsExtractContextPropagationFromPayload` | `boolean` | Will parse and extract context propagation headers from SQS Payload, false by default. [When should it be used?](./doc/sns.md#integration-with-sqs)|
+| Options                                   | Type                                      | Description                                                                                                                                                                     |
+| ----------------------------------------- | ----------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `preRequestHook`                          | `AwsSdkRequestCustomAttributeFunction`    | Hook called before request send, which allow to add custom attributes to span.                                                                                                  |
+| `responseHook`                            | `AwsSdkResponseCustomAttributeFunction`   | Hook for adding custom attributes when response is received from aws.                                                                                                           |
+| `sqsProcessHook`                          | `AwsSdkSqsProcessCustomAttributeFunction` | Hook called after starting sqs `process` span (for each sqs received message), which allow to add custom attributes to it.                                                      |
+| `suppressInternalInstrumentation`         | `boolean`                                 | Most aws operation use http requests under the hood. Set this to `true` to hide all underlying http spans.                                                                      |
+| `sqsExtractContextPropagationFromPayload` | `boolean`                                 | Will parse and extract context propagation headers from SQS Payload, false by default. [When should it be used?](./doc/sns.md#integration-with-sqs)                             |
+| `dynamoDBStatementSerializer`             | `AwsSdkDynamoDBStatementSerializer`       | AWS SDK instrumentation will serialize DynamoDB commands to the `db.statement` attribute using the specified function. Defaults to using a serializer that returns `undefined`. |
 
 ## Span Attributes
 
@@ -82,8 +83,8 @@ Usage example:
 ```js
 awsInstrumentationConfig = {
   preRequestHook: (span, request) => {
-    if (span.serviceName === "s3") {
-      span.setAttribute("s3.bucket.name", request.commandInput["Bucket"]);
+    if (span.serviceName === 's3') {
+      span.setAttribute('s3.bucket.name', request.commandInput['Bucket']);
     }
   },
 };
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/aws-sdk.ts
@@ -475,8 +475,11 @@ export class AwsInstrumentation extends InstrumentationBase<any> {
           command.input,
           undefined
         );
-        const requestMetadata =
-          self.servicesExtensions.requestPreSpanHook(normalizedRequest);
+        const requestMetadata = self.servicesExtensions.requestPreSpanHook(
+          normalizedRequest,
+          self._config,
+          self._diag
+        );
         const span = self._startAwsV3Span(normalizedRequest, requestMetadata);
         const activeContextWithSpan = trace.setSpan(context.active(), span);
 
@@ -602,8 +605,11 @@ export class AwsInstrumentation extends InstrumentationBase<any> {
       }
 
       const normalizedRequest = normalizeV2Request(this);
-      const requestMetadata =
-        self.servicesExtensions.requestPreSpanHook(normalizedRequest);
+      const requestMetadata = self.servicesExtensions.requestPreSpanHook(
+        normalizedRequest,
+        self._config,
+        self._diag
+      );
       const span = self._startAwsV2Span(
         this,
         requestMetadata,
@@ -642,8 +648,11 @@ export class AwsInstrumentation extends InstrumentationBase<any> {
       }
 
       const normalizedRequest = normalizeV2Request(this);
-      const requestMetadata =
-        self.servicesExtensions.requestPreSpanHook(normalizedRequest);
+      const requestMetadata = self.servicesExtensions.requestPreSpanHook(
+        normalizedRequest,
+        self._config,
+        self._diag
+      );
       const span = self._startAwsV2Span(
         this,
         requestMetadata,
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/ServiceExtension.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/ServiceExtension.ts
@@ -13,7 +13,13 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { Span, SpanAttributes, SpanKind, Tracer } from '@opentelemetry/api';
+import {
+  DiagLogger,
+  Span,
+  SpanAttributes,
+  SpanKind,
+  Tracer,
+} from '@opentelemetry/api';
 import {
   AwsSdkInstrumentationConfig,
   NormalizedRequest,
@@ -30,7 +36,11 @@ export interface RequestMetadata {
 
 export interface ServiceExtension {
   // called before request is sent, and before span is started
-  requestPreSpanHook: (request: NormalizedRequest) => RequestMetadata;
+  requestPreSpanHook: (
+    request: NormalizedRequest,
+    config: AwsSdkInstrumentationConfig,
+    diag: DiagLogger
+  ) => RequestMetadata;
 
   // called before request is sent, and after span is started
   requestPostSpanHook?: (request: NormalizedRequest) => void;
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/ServicesExtensions.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/ServicesExtensions.ts
@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { Tracer, Span } from '@opentelemetry/api';
+import { Tracer, Span, DiagLogger } from '@opentelemetry/api';
 import { ServiceExtension, RequestMetadata } from './ServiceExtension';
 import { SqsServiceExtension } from './sqs';
 import {
@@ -35,13 +35,17 @@ export class ServicesExtensions implements ServiceExtension {
     this.services.set('Lambda', new LambdaServiceExtension());
   }
 
-  requestPreSpanHook(request: NormalizedRequest): RequestMetadata {
+  requestPreSpanHook(
+    request: NormalizedRequest,
+    config: AwsSdkInstrumentationConfig,
+    diag: DiagLogger
+  ): RequestMetadata {
     const serviceExtension = this.services.get(request.serviceName);
     if (!serviceExtension)
       return {
         isIncoming: false,
       };
-    return serviceExtension.requestPreSpanHook(request);
+    return serviceExtension.requestPreSpanHook(request, config, diag);
   }
 
   requestPostSpanHook(request: NormalizedRequest) {
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/dynamodb.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/dynamodb.ts
@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { Span, SpanKind, Tracer } from '@opentelemetry/api';
+import { DiagLogger, Span, SpanKind, Tracer } from '@opentelemetry/api';
 import { RequestMetadata, ServiceExtension } from './ServiceExtension';
 import {
   DbSystemValues,
@@ -30,7 +30,11 @@ export class DynamodbServiceExtension implements ServiceExtension {
     return Array.isArray(values) ? values : [values];
   }
 
-  requestPreSpanHook(normalizedRequest: NormalizedRequest): RequestMetadata {
+  requestPreSpanHook(
+    normalizedRequest: NormalizedRequest,
+    config: AwsSdkInstrumentationConfig,
+    diag: DiagLogger
+  ): RequestMetadata {
     const spanKind: SpanKind = SpanKind.CLIENT;
     let spanName: string | undefined;
     const isIncoming = false;
@@ -40,11 +44,23 @@ export class DynamodbServiceExtension implements ServiceExtension {
       [SemanticAttributes.DB_SYSTEM]: DbSystemValues.DYNAMODB,
       [SemanticAttributes.DB_NAME]: normalizedRequest.commandInput?.TableName,
       [SemanticAttributes.DB_OPERATION]: operation,
-      [SemanticAttributes.DB_STATEMENT]: JSON.stringify(
-        normalizedRequest.commandInput
-      ),
     };
 
+    if (config.dynamoDBStatementSerializer) {
+      try {
+        const sanitizedStatement = config.dynamoDBStatementSerializer(
+          operation,
+          normalizedRequest.commandInput
+        );
+
+        if (typeof sanitizedStatement === 'string') {
+          spanAttributes[SemanticAttributes.DB_STATEMENT] = sanitizedStatement;
+        }
+      } catch (err) {
+        diag.error('failed to sanitize DynamoDB statement', err);
+      }
+    }
+
     // normalizedRequest.commandInput.RequestItems) is undefined when no table names are returned
     // keys in this object are the table names
     if (normalizedRequest.commandInput?.TableName) {
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/lambda.ts
@@ -34,7 +34,10 @@ class LambdaCommands {
 }
 
 export class LambdaServiceExtension implements ServiceExtension {
-  requestPreSpanHook(request: NormalizedRequest): RequestMetadata {
+  requestPreSpanHook(
+    request: NormalizedRequest,
+    _config: AwsSdkInstrumentationConfig
+  ): RequestMetadata {
     const functionName = this.extractFunctionName(request.commandInput);
 
     let spanAttributes: SpanAttributes = {};
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/sns.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/sns.ts
@@ -27,7 +27,10 @@ import { injectPropagationContext } from './MessageAttributes';
 import { RequestMetadata, ServiceExtension } from './ServiceExtension';
 
 export class SnsServiceExtension implements ServiceExtension {
-  requestPreSpanHook(request: NormalizedRequest): RequestMetadata {
+  requestPreSpanHook(
+    request: NormalizedRequest,
+    _config: AwsSdkInstrumentationConfig
+  ): RequestMetadata {
     let spanKind: SpanKind = SpanKind.CLIENT;
     let spanName = `SNS ${request.commandName}`;
     const spanAttributes = {
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/sqs.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/services/sqs.ts
@@ -43,7 +43,10 @@ import {
 } from './MessageAttributes';
 
 export class SqsServiceExtension implements ServiceExtension {
-  requestPreSpanHook(request: NormalizedRequest): RequestMetadata {
+  requestPreSpanHook(
+    request: NormalizedRequest,
+    _config: AwsSdkInstrumentationConfig
+  ): RequestMetadata {
     const queueUrl = this.extractQueueUrl(request.commandInput);
     const queueName = this.extractQueueNameFromUrl(queueUrl);
     let spanKind: SpanKind = SpanKind.CLIENT;
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/src/types.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/src/types.ts
@@ -17,6 +17,8 @@ import { Span } from '@opentelemetry/api';
 import { InstrumentationConfig } from '@opentelemetry/instrumentation';
 import { SQS } from './aws-sdk.types';
 
+export type CommandInput = Record<string, any>;
+
 /**
  * These are normalized request and response, which are used by both sdk v2 and v3.
  * They organize the relevant data in one interface which can be processed in a
@@ -25,7 +27,7 @@ import { SQS } from './aws-sdk.types';
 export interface NormalizedRequest {
   serviceName: string;
   commandName: string;
-  commandInput: Record<string, any>;
+  commandInput: CommandInput;
   region?: string;
 }
 export interface NormalizedResponse {
@@ -62,6 +64,11 @@ export interface AwsSdkSqsProcessCustomAttributeFunction {
   (span: Span, sqsProcessInfo: AwsSdkSqsProcessHookInformation): void;
 }
 
+export type AwsSdkDynamoDBStatementSerializer = (
+  operation: string,
+  commandInput: CommandInput
+) => string | undefined;
+
 export interface AwsSdkInstrumentationConfig extends InstrumentationConfig {
   /** hook for adding custom attributes before request is sent to aws */
   preRequestHook?: AwsSdkRequestCustomAttributeFunction;
@@ -72,6 +79,9 @@ export interface AwsSdkInstrumentationConfig extends InstrumentationConfig {
   /** hook for adding custom attribute when an sqs process span is started */
   sqsProcessHook?: AwsSdkSqsProcessCustomAttributeFunction;
 
+  /** custom serializer function for the db.statement attribute in DynamoDB spans */
+  dynamoDBStatementSerializer?: AwsSdkDynamoDBStatementSerializer;
+
   /**
    * Most aws operation use http request under the hood.
    * if http instrumentation is enabled, each aws operation will also create
diff --git a/plugins/node/opentelemetry-instrumentation-aws-sdk/test/dynamodb.test.ts b/plugins/node/opentelemetry-instrumentation-aws-sdk/test/dynamodb.test.ts
