From faccd8012075d06ed616babe9b284ef8b00291d3 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Fri, 12 Aug 2022 12:37:38 -0600 Subject: [PATCH 1/7] fix(tracer): use instance scope in capture decorators --- packages/tracer/src/Tracer.ts | 47 ++++++++++------ packages/tracer/tests/unit/Tracer.test.ts | 68 +++++++++++++++++++++++ 2 files changed, 97 insertions(+), 18 deletions(-) diff --git a/packages/tracer/src/Tracer.ts b/packages/tracer/src/Tracer.ts index 663b98afc7..f204388a02 100644 --- a/packages/tracer/src/Tracer.ts +++ b/packages/tracer/src/Tracer.ts @@ -340,27 +340,36 @@ class Tracer extends Utility implements TracerInterface { * @decorator Class */ public captureLambdaHandler(): HandlerMethodDecorator { - return (target, _propertyKey, descriptor) => { + return (_target, _propertyKey, descriptor) => { /** * The descriptor.value is the method this decorator decorates, it cannot be undefined. */ // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const originalMethod = descriptor.value!; - descriptor.value = ((event, context, callback) => { - if (!this.isTracingEnabled()) { - return originalMethod.apply(target, [ event, context, callback ]); + // eslint-disable-next-line @typescript-eslint/no-this-alias + const that = this; + descriptor.value = (function (event, context, callback) { + // We know that 'this' is a LambdaHandler because captureLambdaHandler + // can only be applied to a LambdaHandler. + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + // eslint-disable-next-line @typescript-eslint/no-this-alias + const thisLambdaHandler: LambdaHandler = this; + + if (!that.isTracingEnabled()) { + return originalMethod.apply(thisLambdaHandler, [ event, context, callback ]); } - return this.provider.captureAsyncFunc(`## ${process.env._HANDLER}`, async subsegment => { - this.annotateColdStart(); - this.addServiceNameAnnotation(); + return that.provider.captureAsyncFunc(`## ${process.env._HANDLER}`, async subsegment => { + that.annotateColdStart(); + that.addServiceNameAnnotation(); let result: unknown; try { - result = await originalMethod.apply(target, [ event, context, callback ]); - this.addResponseAsMetadata(result, process.env._HANDLER); + result = await originalMethod.apply(thisLambdaHandler, [ event, context, callback ]); + that.addResponseAsMetadata(result, process.env._HANDLER); } catch (error) { - this.addErrorAsMetadata(error as Error); + that.addErrorAsMetadata(error as Error); throw error; } finally { subsegment?.close(); @@ -411,23 +420,25 @@ class Tracer extends Utility implements TracerInterface { * @decorator Class */ public captureMethod(): MethodDecorator { - return (target, _propertyKey, descriptor) => { + return (_target, _propertyKey, descriptor) => { // The descriptor.value is the method this decorator decorates, it cannot be undefined. // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const originalMethod = descriptor.value!; - descriptor.value = (...args: unknown[]) => { - if (!this.isTracingEnabled()) { - return originalMethod.apply(target, [...args]); + // eslint-disable-next-line @typescript-eslint/no-this-alias + const that = this; + descriptor.value = function (...args: unknown[]) { + if (!that.isTracingEnabled()) { + return originalMethod.apply(this, [...args]); } - return this.provider.captureAsyncFunc(`### ${originalMethod.name}`, async subsegment => { + return that.provider.captureAsyncFunc(`### ${originalMethod.name}`, async subsegment => { let result; try { - result = await originalMethod.apply(target, [...args]); - this.addResponseAsMetadata(result, originalMethod.name); + result = await originalMethod.apply(this, [...args]); + that.addResponseAsMetadata(result, originalMethod.name); } catch (error) { - this.addErrorAsMetadata(error as Error); + that.addErrorAsMetadata(error as Error); throw error; } finally { diff --git a/packages/tracer/tests/unit/Tracer.test.ts b/packages/tracer/tests/unit/Tracer.test.ts index 0c6e086508..239f59f14a 100644 --- a/packages/tracer/tests/unit/Tracer.test.ts +++ b/packages/tracer/tests/unit/Tracer.test.ts @@ -854,6 +854,36 @@ describe('Class: Tracer', () => { }); + test('when used as decorator and when calling the handler, it has access to member variables', async () => { + + // Prepare + const tracer: Tracer = new Tracer(); + const newSubsegment: Segment | Subsegment | undefined = new Subsegment('### dummyMethod'); + jest.spyOn(tracer.provider, 'getSegment') + .mockImplementation(() => newSubsegment); + setContextMissingStrategy(() => null); + + class Lambda implements LambdaInterface { + private readonly memberVariable: string; + + public constructor(memberVariable: string) { + this.memberVariable = memberVariable; + } + + @tracer.captureLambdaHandler() + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + public async handler(_event: TEvent, _context: Context, _callback: Callback): void | Promise { + return (`memberVariable:${this.memberVariable}` as unknown); + } + + } + + // Act / Assess + const lambda = new Lambda('someValue'); + expect(await lambda.handler({}, context, () => console.log('Lambda invoked!'))).toEqual('memberVariable:someValue'); + + }); }); describe('Method: captureMethod', () => { @@ -1009,6 +1039,44 @@ describe('Class: Tracer', () => { }); + test('when used as decorator and when calling a method in the class, it has access to member variables', async () => { + + // Prepare + const tracer: Tracer = new Tracer(); + const newSubsegment: Segment | Subsegment | undefined = new Subsegment('### dummyMethod'); + jest.spyOn(tracer.provider, 'getSegment') + .mockImplementation(() => newSubsegment); + setContextMissingStrategy(() => null); + + class Lambda implements LambdaInterface { + private readonly memberVariable: string; + + public constructor(memberVariable: string) { + this.memberVariable = memberVariable; + } + + @tracer.captureMethod() + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + public async dummyMethod(): Promise { + return `memberVariable:${this.memberVariable}`; + } + + @tracer.captureLambdaHandler() + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + public async handler(_event: TEvent, _context: Context, _callback: Callback): void | Promise { + return (await this.dummyMethod() as unknown); + } + + } + + // Act / Assess + const lambda = new Lambda('someValue'); + expect(await lambda.dummyMethod()).toEqual('memberVariable:someValue'); + + }); + }); describe('Method: captureAWS', () => { From 8973d2be0d57e1f708c0fe105e5cb500f8abe5f2 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Fri, 12 Aug 2022 18:02:59 -0600 Subject: [PATCH 2/7] docs: add more inline comments --- packages/tracer/src/Tracer.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/tracer/src/Tracer.ts b/packages/tracer/src/Tracer.ts index f204388a02..fd75820bc2 100644 --- a/packages/tracer/src/Tracer.ts +++ b/packages/tracer/src/Tracer.ts @@ -349,6 +349,8 @@ class Tracer extends Utility implements TracerInterface { // eslint-disable-next-line @typescript-eslint/no-this-alias const that = this; + // Use a function() {} instead of () => {} an arrow function so that we can + // access `myClass` as `this` in a decorated `myClass.myMethod()`. descriptor.value = (function (event, context, callback) { // We know that 'this' is a LambdaHandler because captureLambdaHandler // can only be applied to a LambdaHandler. @@ -427,6 +429,8 @@ class Tracer extends Utility implements TracerInterface { // eslint-disable-next-line @typescript-eslint/no-this-alias const that = this; + // Use a function() {} instead of an () => {} arrow function so that we can + // access `myClass` as `this` in a decorated `myClass.myMethod()`. descriptor.value = function (...args: unknown[]) { if (!that.isTracingEnabled()) { return originalMethod.apply(this, [...args]); From 63df2401c3dc966b01e2a5743209f1093126760f Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Fri, 12 Aug 2022 18:10:27 -0600 Subject: [PATCH 3/7] docs: small grammatical inline docs correction --- packages/tracer/src/Tracer.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracer/src/Tracer.ts b/packages/tracer/src/Tracer.ts index fd75820bc2..af0a5277bc 100644 --- a/packages/tracer/src/Tracer.ts +++ b/packages/tracer/src/Tracer.ts @@ -349,7 +349,7 @@ class Tracer extends Utility implements TracerInterface { // eslint-disable-next-line @typescript-eslint/no-this-alias const that = this; - // Use a function() {} instead of () => {} an arrow function so that we can + // Use a function() {} instead of an () => {} arrow function so that we can // access `myClass` as `this` in a decorated `myClass.myMethod()`. descriptor.value = (function (event, context, callback) { // We know that 'this' is a LambdaHandler because captureLambdaHandler From f9185d60214302d1fd9914dec0305e9aa26c561c Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Sat, 13 Aug 2022 18:47:32 +0000 Subject: [PATCH 4/7] refactor: apply review changes --- packages/tracer/src/Tracer.ts | 40 ++++++++++++++++------------------- 1 file changed, 18 insertions(+), 22 deletions(-) diff --git a/packages/tracer/src/Tracer.ts b/packages/tracer/src/Tracer.ts index af0a5277bc..b11a4f3e7e 100644 --- a/packages/tracer/src/Tracer.ts +++ b/packages/tracer/src/Tracer.ts @@ -1,5 +1,5 @@ import { Handler } from 'aws-lambda'; -import { Utility } from '@aws-lambda-powertools/commons'; +import { AsyncHandler, SyncHandler, Utility } from '@aws-lambda-powertools/commons'; import { TracerInterface } from '.'; import { ConfigServiceInterface, EnvironmentVariablesService } from './config'; import { HandlerMethodDecorator, TracerOptions, MethodDecorator } from './types'; @@ -348,30 +348,26 @@ class Tracer extends Utility implements TracerInterface { const originalMethod = descriptor.value!; // eslint-disable-next-line @typescript-eslint/no-this-alias - const that = this; + const tracerRef = this; // Use a function() {} instead of an () => {} arrow function so that we can // access `myClass` as `this` in a decorated `myClass.myMethod()`. - descriptor.value = (function (event, context, callback) { - // We know that 'this' is a LambdaHandler because captureLambdaHandler - // can only be applied to a LambdaHandler. - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore + descriptor.value = (function (this: Handler, event, context, callback) { // eslint-disable-next-line @typescript-eslint/no-this-alias - const thisLambdaHandler: LambdaHandler = this; + const handlerRef: Handler = this; - if (!that.isTracingEnabled()) { - return originalMethod.apply(thisLambdaHandler, [ event, context, callback ]); + if (!tracerRef.isTracingEnabled()) { + return originalMethod.apply(handlerRef, [ event, context, callback ]); } - return that.provider.captureAsyncFunc(`## ${process.env._HANDLER}`, async subsegment => { - that.annotateColdStart(); - that.addServiceNameAnnotation(); + return tracerRef.provider.captureAsyncFunc(`## ${process.env._HANDLER}`, async subsegment => { + tracerRef.annotateColdStart(); + tracerRef.addServiceNameAnnotation(); let result: unknown; try { - result = await originalMethod.apply(thisLambdaHandler, [ event, context, callback ]); - that.addResponseAsMetadata(result, process.env._HANDLER); + result = await originalMethod.apply(handlerRef, [ event, context, callback ]); + tracerRef.addResponseAsMetadata(result, process.env._HANDLER); } catch (error) { - that.addErrorAsMetadata(error as Error); + tracerRef.addErrorAsMetadata(error as Error); throw error; } finally { subsegment?.close(); @@ -380,7 +376,7 @@ class Tracer extends Utility implements TracerInterface { return result; }); - }) as Handler; + }) as SyncHandler | AsyncHandler; return descriptor; }; @@ -428,21 +424,21 @@ class Tracer extends Utility implements TracerInterface { const originalMethod = descriptor.value!; // eslint-disable-next-line @typescript-eslint/no-this-alias - const that = this; + const tracerRef = this; // Use a function() {} instead of an () => {} arrow function so that we can // access `myClass` as `this` in a decorated `myClass.myMethod()`. descriptor.value = function (...args: unknown[]) { - if (!that.isTracingEnabled()) { + if (!tracerRef.isTracingEnabled()) { return originalMethod.apply(this, [...args]); } - return that.provider.captureAsyncFunc(`### ${originalMethod.name}`, async subsegment => { + return tracerRef.provider.captureAsyncFunc(`### ${originalMethod.name}`, async subsegment => { let result; try { result = await originalMethod.apply(this, [...args]); - that.addResponseAsMetadata(result, originalMethod.name); + tracerRef.addResponseAsMetadata(result, originalMethod.name); } catch (error) { - that.addErrorAsMetadata(error as Error); + tracerRef.addErrorAsMetadata(error as Error); throw error; } finally { From 559ba6fce82784988847fa1c9d2d79bad88ec49f Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Sat, 13 Aug 2022 19:57:42 +0000 Subject: [PATCH 5/7] docs: .bind changes --- docs/core/tracer.md | 10 ++++++++-- packages/tracer/src/Tracer.ts | 4 ++-- packages/tracer/tests/unit/Tracer.test.ts | 3 ++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/core/tracer.md b/docs/core/tracer.md index 8a2b798e18..8b4b49ed39 100644 --- a/docs/core/tracer.md +++ b/docs/core/tracer.md @@ -143,8 +143,11 @@ You can quickly start by importing the `Tracer` class, initialize it outside the } export const handlerClass = new Lambda(); - export const handler = handlerClass.handler; + export const handler = handlerClass.handler.bind(handlerClass); // (1) ``` + + 1. Binding your handler method allows your handler to access `this`. + === "Manual" ```typescript hl_lines="6 8-9 12-13 19 22 26 28" @@ -253,8 +256,11 @@ You can trace other Class methods using the `captureMethod` decorator or any arb } export const myFunction = new Lambda(); - export const handler = myFunction.handler; + export const handler = myFunction.handler.bind(myFunction); // (1) ``` + + 1. Binding your handler method allows your handler to access `this`. + === "Manual" ```typescript hl_lines="6 8-9 15 18 23 25" diff --git a/packages/tracer/src/Tracer.ts b/packages/tracer/src/Tracer.ts index b11a4f3e7e..c7faf1e3e0 100644 --- a/packages/tracer/src/Tracer.ts +++ b/packages/tracer/src/Tracer.ts @@ -67,7 +67,7 @@ import { Segment, Subsegment } from 'aws-xray-sdk-core'; * } * * export const handlerClass = new Lambda(); - * export const handler = handlerClass.handler; + * export const handler = handlerClass.handler.bind(handlerClass); * ``` * * ### Functions usage with manual instrumentation @@ -334,7 +334,7 @@ class Tracer extends Utility implements TracerInterface { * } * * export const handlerClass = new Lambda(); - * export const handler = handlerClass.handler; + * export const handler = handlerClass.handler.bind(handlerClass); * ``` * * @decorator Class diff --git a/packages/tracer/tests/unit/Tracer.test.ts b/packages/tracer/tests/unit/Tracer.test.ts index 239f59f14a..b0100b42d3 100644 --- a/packages/tracer/tests/unit/Tracer.test.ts +++ b/packages/tracer/tests/unit/Tracer.test.ts @@ -881,7 +881,8 @@ describe('Class: Tracer', () => { // Act / Assess const lambda = new Lambda('someValue'); - expect(await lambda.handler({}, context, () => console.log('Lambda invoked!'))).toEqual('memberVariable:someValue'); + const handler = lambda.handler.bind(lambda); + expect(await handler({}, context, () => console.log('Lambda invoked!'))).toEqual('memberVariable:someValue'); }); }); From 87070d3cea81abd5b783b7efe0acf1b59a0f15f4 Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Sat, 13 Aug 2022 20:35:09 +0000 Subject: [PATCH 6/7] test: use member variable in e2e tests --- .../tests/e2e/allFeatures.decorator.test.functionCode.ts | 8 +++++++- .../tests/e2e/asyncHandler.decorator.test.functionCode.ts | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts b/packages/tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts index f86937082f..6eb4a9757e 100644 --- a/packages/tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts +++ b/packages/tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts @@ -36,6 +36,12 @@ const tracer = new Tracer({ serviceName: serviceName }); const dynamoDBv3 = tracer.captureAWSv3Client(new DynamoDBClient({})); export class MyFunctionWithDecorator { + private readonly returnValue: string; + + public constructor() { + this.returnValue = customResponseValue; + } + @tracer.captureLambdaHandler() // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore @@ -77,7 +83,7 @@ export class MyFunctionWithDecorator { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore public myMethod(): string { - return customResponseValue; + return this.returnValue; } } diff --git a/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts b/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts index 2607968a45..41ae77c1a3 100644 --- a/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts +++ b/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts @@ -36,6 +36,12 @@ const tracer = new Tracer({ serviceName: serviceName }); const dynamoDBv3 = tracer.captureAWSv3Client(new DynamoDBClient({})); export class MyFunctionWithDecorator { + private readonly returnValue: string; + + public constructor() { + this.returnValue = customResponseValue; + } + @tracer.captureLambdaHandler() // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore @@ -72,7 +78,7 @@ export class MyFunctionWithDecorator { // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore public myMethod(): string { - return customResponseValue; + return this.returnValue; } } From 8ee7f969e4a9d67744c09e9a291bf9bc4996ce1f Mon Sep 17 00:00:00 2001 From: Josh Kellendonk Date: Sat, 13 Aug 2022 20:48:34 +0000 Subject: [PATCH 7/7] fix: bind the handler in the e2e tests --- .../tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts | 2 +- .../tests/e2e/asyncHandler.decorator.test.functionCode.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts b/packages/tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts index 6eb4a9757e..6edce9e0dd 100644 --- a/packages/tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts +++ b/packages/tracer/tests/e2e/allFeatures.decorator.test.functionCode.ts @@ -88,4 +88,4 @@ export class MyFunctionWithDecorator { } export const handlerClass = new MyFunctionWithDecorator(); -export const handler = handlerClass.handler; \ No newline at end of file +export const handler = handlerClass.handler.bind(handlerClass); \ No newline at end of file diff --git a/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts b/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts index 41ae77c1a3..ea1facd46c 100644 --- a/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts +++ b/packages/tracer/tests/e2e/asyncHandler.decorator.test.functionCode.ts @@ -83,4 +83,4 @@ export class MyFunctionWithDecorator { } export const handlerClass = new MyFunctionWithDecorator(); -export const handler = handlerClass.handler; \ No newline at end of file +export const handler = handlerClass.handler.bind(handlerClass); \ No newline at end of file