removed lambda and sns tests, addressed PR comments

liustve · liustve · commit 57ed9b47a128 · 2024-11-28T00:32:43.000Z
diff --git a/aws-distro-opentelemetry-node-autoinstrumentation/src/aws-metric-attribute-generator.ts b/aws-distro-opentelemetry-node-autoinstrumentation/src/aws-metric-attribute-generator.ts
@@ -580,13 +580,14 @@ export class AwsMetricAttributeGenerator implements MetricAttributeGenerator {
     return input.split('^').join('^^').split('|').join('^|');
   }
 
-  private static simplifyARNAttribute(attribute: AttributeValue | undefined) {
+  // Extracts the name of the resource from an arn
+  private static simplifyARNAttribute(attribute: AttributeValue | undefined): string | undefined {
     if (typeof attribute == 'string' && attribute.startsWith('arn:aws:')) {
       const split = attribute.split(':');
       return split[split.length - 1];
     }
 
-    return '';
+    return undefined;
   }
 
   /** Span kind is needed for differentiating metrics in the EMF exporter */
diff --git a/aws-distro-opentelemetry-node-autoinstrumentation/src/patches/aws/services/secretsmanager.ts b/aws-distro-opentelemetry-node-autoinstrumentation/src/patches/aws/services/secretsmanager.ts
@@ -19,7 +19,7 @@ export class SecretsManagerServiceExtension implements ServiceExtension {
 
     const spanAttributes: Attributes = {};
 
-    if (secretId && secretId.startsWith('arn:aws:secretsmanager:')) {
+    if (typeof secretId == 'string' && secretId.startsWith('arn:aws:secretsmanager:')) {
       spanAttributes[AWS_ATTRIBUTE_KEYS.AWS_SECRETSMANAGER_SECRET_ARN] = secretId;
     }
 
@@ -34,10 +34,10 @@ export class SecretsManagerServiceExtension implements ServiceExtension {
   }
 
   responseHook(response: NormalizedResponse, span: Span, tracer: Tracer, config: AwsSdkInstrumentationConfig): void {
-    const secret_arn = response.data.ARN;
+    const secretArn = response.data.ARN;
 
-    if (secret_arn) {
-      span.setAttribute(AWS_ATTRIBUTE_KEYS.AWS_SECRETSMANAGER_SECRET_ARN, secret_arn);
+    if (secretArn) {
+      span.setAttribute(AWS_ATTRIBUTE_KEYS.AWS_SECRETSMANAGER_SECRET_ARN, secretArn);
     }
   }
 }
diff --git a/aws-distro-opentelemetry-node-autoinstrumentation/src/patches/instrumentation-patch.ts b/aws-distro-opentelemetry-node-autoinstrumentation/src/patches/instrumentation-patch.ts
@@ -176,6 +176,7 @@ function patchSnsServiceExtension(snsServiceExtension: any): void {
       }
       return requestMetadata;
     };
+
     snsServiceExtension.requestPreSpanHook = patchedRequestPreSpanHook;
   }
 }
diff --git a/aws-distro-opentelemetry-node-autoinstrumentation/test/aws-metric-attribute-generator.test.ts b/aws-distro-opentelemetry-node-autoinstrumentation/test/aws-metric-attribute-generator.test.ts
@@ -782,14 +782,14 @@ describe('AwsMetricAttributeGeneratorTest', () => {
     // Validate behaviour of AWS_STEPFUNCTIONS_STATEMACHINE_ARN and AWS_STEPFUNCTIONS_ACTIVITY_ARN attributes then remove them.
     mockAttribute(
       AWS_ATTRIBUTE_KEYS.AWS_STEPFUNCTIONS_STATEMACHINE_ARN,
-      'arn:aws:states:us-east-1:007003123456789012:stateMachine:testStateMachine'
+      'arn:aws:states:us-east-1:123456789123:stateMachine:testStateMachine'
     );
     validateRemoteResourceAttributes('AWS::StepFunctions::StateMachine', 'testStateMachine');
     mockAttribute(AWS_ATTRIBUTE_KEYS.AWS_STEPFUNCTIONS_STATEMACHINE_ARN, undefined);
 
     mockAttribute(
       AWS_ATTRIBUTE_KEYS.AWS_STEPFUNCTIONS_ACTIVITY_ARN,
-      'arn:aws:states:us-east-1:007003123456789012:activity:testActivity'
+      'arn:aws:states:us-east-1:123456789123:activity:testActivity'
     );
     validateRemoteResourceAttributes('AWS::StepFunctions::Activity', 'testActivity');
     mockAttribute(AWS_ATTRIBUTE_KEYS.AWS_STEPFUNCTIONS_ACTIVITY_ARN, undefined);
@@ -839,12 +839,10 @@ describe('AwsMetricAttributeGeneratorTest', () => {
     validateRemoteResourceAttributes('AWS::Bedrock::DataSource', 'test_datasource_^^id');
     mockAttribute(AWS_ATTRIBUTE_KEYS.AWS_BEDROCK_DATA_SOURCE_ID, undefined);
 
-    // Validate behaviour of AWS_BEDROCK_GUARDRAIL_ID and AWS_BEDROCK_GUARDRAIL_ARN attribute, then remove it.
+    // Validate behaviour of AWS_BEDROCK_GUARDRAIL_ID attribute, then remove it.
     mockAttribute(AWS_ATTRIBUTE_KEYS.AWS_BEDROCK_GUARDRAIL_ID, 'test_guardrail_id');
-    mockAttribute(AWS_ATTRIBUTE_KEYS.AWS_BEDROCK_GUARDRAIL_ARN, 'test_guardrail_arn');
     validateRemoteResourceAttributes('AWS::Bedrock::Guardrail', 'test_guardrail_id');
     mockAttribute(AWS_ATTRIBUTE_KEYS.AWS_BEDROCK_GUARDRAIL_ID, undefined);
-    mockAttribute(AWS_ATTRIBUTE_KEYS.AWS_BEDROCK_GUARDRAIL_ARN, undefined);
 
     // Validate behaviour of AWS_BEDROCK_GUARDRAIL_ID attribute with special chars(^), then remove it.
     mockAttribute(AWS_ATTRIBUTE_KEYS.AWS_BEDROCK_GUARDRAIL_ID, 'test_guardrail_^id');
diff --git a/aws-distro-opentelemetry-node-autoinstrumentation/test/patches/aws/services/lamba.test.ts b/aws-distro-opentelemetry-node-autoinstrumentation/test/patches/aws/services/lamba.test.ts
diff --git a/aws-distro-opentelemetry-node-autoinstrumentation/test/patches/aws/services/sns.test.ts b/aws-distro-opentelemetry-node-autoinstrumentation/test/patches/aws/services/sns.test.ts
diff --git a/aws-distro-opentelemetry-node-autoinstrumentation/test/patches/aws/services/step-functions.test.ts b/aws-distro-opentelemetry-node-autoinstrumentation/test/patches/aws/services/step-functions.test.ts
@@ -33,7 +33,7 @@ describe('SFN', () => {
 
   describe('DescribeStateMachine', () => {
     it('span has stateMachineArn in its attributes', async () => {
-      const stateMachineArn: string = 'arn:aws:states:us-east-1:007003123456789012:stateMachine:testStateMachine';
+      const stateMachineArn: string = 'arn:aws:states:us-east-1:123456789123:stateMachine:testStateMachine';
 
       nock(`https://states.${region}.amazonaws.com/`).post('/').reply(200, 'null');
 
@@ -64,7 +64,7 @@ describe('SFN', () => {
 
   describe('DescribeActivity', () => {
     it('span has activityArn in its attributes', async () => {
-      const activityArn: string = 'arn:aws:states:us-east-1:007003123456789012:activity:testActivity';
+      const activityArn: string = 'arn:aws:states:us-east-1:123456789123:activity:testActivity';
 
       nock(`https://states.${region}.amazonaws.com/`).post('/').reply(200, 'null');
 
diff --git a/aws-distro-opentelemetry-node-autoinstrumentation/test/patches/instrumentation-patch.test.ts b/aws-distro-opentelemetry-node-autoinstrumentation/test/patches/instrumentation-patch.test.ts
@@ -27,8 +27,8 @@ import { SinonStub } from 'sinon';
 const _STREAM_NAME: string = 'streamName';
 const _BUCKET_NAME: string = 'bucketName';
 const _QUEUE_NAME: string = 'queueName';
-const _ACTIVITY_ARN: string = 'arn:aws:states:us-east-1:007003123456789012:activity:testActivity';
-const _STATE_MACHINE_ARN: string = 'arn:aws:states:us-east-1:007003123456789012:stateMachine:testStateMachine';
+const _ACTIVITY_ARN: string = 'arn:aws:states:us-east-1:123456789123:activity:testActivity';
+const _STATE_MACHINE_ARN: string = 'arn:aws:states:us-east-1:123456789123:stateMachine:testStateMachine';
 const _SECRETS_ARN: string = 'arn:aws:secretsmanager:us-east-1:123456789123:secret:testId123456';
 const _UUID: string = 'random-uuid';
 const _TOPIC_ARN: string = 'arn:aws:sns:us-east-1:123456789012:mystack-mytopic-NZJ5JSMVGFIE';
@@ -60,17 +60,17 @@ describe('InstrumentationPatchTest', () => {
     expect(services.has('SQS')).toBeTruthy();
     expect(services.has('SNS')).toBeTruthy();
     expect(services.has('Lambda')).toBeTruthy();
-    expect(services.get('SNS')._requestPreSpanHook).toBeFalsy();
-    expect(services.get('SNS').requestPreSpanHook).toBeTruthy();
-    expect(services.get('Lambda')._requestPreSpanHook).toBeFalsy();
-    expect(services.get('Lambda').requestPreSpanHook).toBeTruthy();
 
     expect(services.has('DynamoDB')).toBeTruthy();
     // From patching but shouldn't be applied
     expect(services.get('SecretsManager')).toBeFalsy();
     expect(services.get('SFN')).toBeFalsy();
     expect(services.has('S3')).toBeFalsy();
     expect(services.has('Kinesis')).toBeFalsy();
+    expect(services.get('SNS')._requestPreSpanHook).toBeFalsy();
+    expect(services.get('SNS').requestPreSpanHook).toBeTruthy();
+    expect(services.get('Lambda')._requestPreSpanHook).toBeFalsy();
+    expect(services.get('Lambda').requestPreSpanHook).toBeTruthy();
     expect(services.get('SQS')._requestPreSpanHook).toBeFalsy();
     expect(services.get('SQS').requestPreSpanHook).toBeTruthy();
     expect(services.has('Bedrock')).toBeFalsy();
@@ -91,15 +91,16 @@ describe('InstrumentationPatchTest', () => {
     expect(services.has('SNS')).toBeTruthy();
     expect(services.has('DynamoDB')).toBeTruthy();
     expect(services.has('Lambda')).toBeTruthy();
-    expect(services.get('SNS')._requestPreSpanHook).toBeTruthy();
-    expect(services.get('SNS').requestPreSpanHook).toBeTruthy();
-    expect(services.get('Lambda')._requestPreSpanHook).toBeTruthy();
-    expect(services.get('Lambda').requestPreSpanHook).toBeTruthy();
+
     // From patching
     expect(services.has('SecretsManager')).toBeTruthy();
     expect(services.has('SFN')).toBeTruthy();
     expect(services.has('S3')).toBeTruthy();
     expect(services.has('Kinesis')).toBeTruthy();
+    expect(services.get('SNS')._requestPreSpanHook).toBeTruthy();
+    expect(services.get('SNS').requestPreSpanHook).toBeTruthy();
+    expect(services.get('Lambda')._requestPreSpanHook).toBeTruthy();
+    expect(services.get('Lambda').requestPreSpanHook).toBeTruthy();
     expect(services.get('SQS')._requestPreSpanHook).toBeTruthy();
     expect(services.get('SQS').requestPreSpanHook).toBeTruthy();
     expect(services.has('Bedrock')).toBeTruthy();

Original file line number	Diff line number	Diff line change
`@@ -580,13 +580,14 @@ export class AwsMetricAttributeGenerator implements MetricAttributeGenerator {`
`580`	`580`	`return input.split('^').join('^^').split('\|').join('^\|');`
`581`	`581`	`}`
`582`	`582`
`583`		`- private static simplifyARNAttribute(attribute: AttributeValue \| undefined) {`
	`583`	`+ // Extracts the name of the resource from an arn`
	`584`	`+ private static simplifyARNAttribute(attribute: AttributeValue \| undefined): string \| undefined {`
`584`	`585`	`if (typeof attribute == 'string' && attribute.startsWith('arn:aws:')) {`
`585`	`586`	`const split = attribute.split(':');`
`586`	`587`	`return split[split.length - 1];`
`587`	`588`	`}`
`588`	`589`
`589`		`- return '';`
	`590`	`+ return undefined;`
`590`	`591`	`}`
`591`	`592`
`592`	`593`	`/** Span kind is needed for differentiating metrics in the EMF exporter */`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ export class SecretsManagerServiceExtension implements ServiceExtension {`
`19`	`19`
`20`	`20`	`const spanAttributes: Attributes = {};`
`21`	`21`
`22`		`- if (secretId && secretId.startsWith('arn:aws:secretsmanager:')) {`
	`22`	`+ if (typeof secretId == 'string' && secretId.startsWith('arn:aws:secretsmanager:')) {`
`23`	`23`	`spanAttributes[AWS_ATTRIBUTE_KEYS.AWS_SECRETSMANAGER_SECRET_ARN] = secretId;`
`24`	`24`	`}`
`25`	`25`
`@@ -34,10 +34,10 @@ export class SecretsManagerServiceExtension implements ServiceExtension {`
`34`	`34`	`}`
`35`	`35`
`36`	`36`	`responseHook(response: NormalizedResponse, span: Span, tracer: Tracer, config: AwsSdkInstrumentationConfig): void {`
`37`		`- const secret_arn = response.data.ARN;`
	`37`	`+ const secretArn = response.data.ARN;`
`38`	`38`
`39`		`- if (secret_arn) {`
`40`		`- span.setAttribute(AWS_ATTRIBUTE_KEYS.AWS_SECRETSMANAGER_SECRET_ARN, secret_arn);`
	`39`	`+ if (secretArn) {`
	`40`	`+ span.setAttribute(AWS_ATTRIBUTE_KEYS.AWS_SECRETSMANAGER_SECRET_ARN, secretArn);`
`41`	`41`	`}`
`42`	`42`	`}`
`43`	`43`	`}`
Original file line number	Diff line number	Diff line change
`@@ -176,6 +176,7 @@ function patchSnsServiceExtension(snsServiceExtension: any): void {`
`176`	`176`	`}`
`177`	`177`	`return requestMetadata;`
`178`	`178`	`};`
	`179`	`+`
`179`	`180`	`snsServiceExtension.requestPreSpanHook = patchedRequestPreSpanHook;`
`180`	`181`	`}`
`181`	`182`	`}`