fix(tracer): use instance as this in capture decorators
#1055
Conversation
|
It's unclear from the contribution guide whether, upon acceptance, the reviewer(s) will squash-merge this Pull Request, or if I should rebase from the main branch. Please let me know if you want me to rebase before this PR is acceptable. For now, I've avoided rebasing in favour of a linear feature branch timeline. On another note: Currently, GitHub says that there are 3 workflows awaiting approval. Once these are approved, I can act on any information I gain from their running. |
misterjoshua
changed the title
this in capture decorators
There was a problem hiding this comment.
Thank you for working on this and providing a potential fix. The proposed change seems appropriate and the unit tests are passing.
I will however momentarily put this PR on hold as I need to do more tests and potentially investigate internally at AWS.
The reason why I'm saying this is that the proposed change works locally or on our CI but doesn't work when run in AWS Lambda.
Take this simplified example:
class DecoratorUtilClass {
public decorate() {
return function (
target: any,
propertyKey: string,
descriptor: PropertyDescriptor
) {
const originalMethod = descriptor.value;
console.log(`Evaluation: Target name ${target.constructor.name}`);
console.log(
`Evaluation: ${JSON.stringify(
Object.getOwnPropertyNames(target),
null,
2
)}`
);
descriptor.value = function (...args: any[]) {
console.log(`Call: ${this.constructor.name}`);
console.log(
`Evaluation: ${JSON.stringify(
Object.getOwnPropertyNames(this),
null,
2
)}`
);
// Do something before
const result = originalMethod.call(this, args);
// Do something after
return result;
};
return descriptor;
};
}
}
const decoratorUtil = new DecoratorUtilClass();
class Lambda {
greeting: string;
constructor(message: string) {
this.greeting = message;
}
@decoratorUtil.decorate()
handler() {
console.log(`Current Node version ${process.version}`);
console.log(this.buildGreeting());
}
buildGreeting() {
return "Hello, " + this.greeting;
}
}
const lambda = new Lambda("world");
module.exports = lambda;Transpile it to JS with esbuild (esbuild --bundle index.ts --target=node16 --platform=node --outfile=dist/index.js --sourcemap) and then run it both locally & then in a Lambda function.
Locally the result will be:
Evaluation: Target name Lambda
Evaluation: [
"constructor",
"handler",
"buildGreeting"
]
Call: Lambda
Evaluation: [
"greeting"
]
Current Node version v16.15.0
Hello, world
In Lambda the value of this inside the descriptor.value = function (...args: any[]) { function is not the class but something else, resulting the function to throw error.
dreamorosi
added
on-hold
|
@dreamorosi I believe I've applied all of your review changes. I'll keep an eye out for your findings. I'll investigate in a Lambda environment as well. If I find anything myself, I'll comment here. |
|
Thank you @misterjoshua, I think I have made some progress. Running a version of Tracer from this branch in a Lambda environment (you can generate an installer by running class Lambda implements LambdaInterface {
greeting: string;
constructor(message: string) {
this.greeting = message;
}
@tracer.captureMethod()
public async getGreeting(): string {
/* ... */
return `Hello ${this.greeting}`;
}
public async handler(_event: any, _context: any): Promise<void> {
console.log(await this.getGreeting());
}
}
export const myFunction = new Lambda("World");
export const handler = myFunction.handler.bind(myFunction);
The key is the last line where we re-bind the instance of the class back to the The only other way, would be to transform the handler to arrow functions (& possibly all other class methods in the I'm still trying to wrap my head around it but that seems to be the main discriminant. I'm still not sure why locally in unit tests it works and in Lambda it doesn't, but I'm suspecting it's due to how the Lambda service calls the function and its scope. Let me know if you get to a different conclusion. |
|
If you agree with my assessment, would you mind also adding the
For the last two, I think it would be best to add a side note (see screenshot below) that explains that this is really needed only when the decorated methods need to access to the
To add a note like the above the line would become: export const handlerClass = new Lambda();
export const handler = handlerClass.handler.bind(handlerClass); // (1)And then write the note after the example like here. |
I see what you're saying. Yes, that makes sense, and I can reproduce the problem exactly as you say. In JavaScript, the I'll make the adjustments you suggest. |
dreamorosi
added
tracer
|
I have created a more detailed issue for this #1056, if you could leave a comment under the issue I'd like to assign it to you so that it'll be reflected that you're the one who fixed it. If you're available and can, I would appreciate if you could also make a couple of small changes to the integration/e2e tests that are run in a real AWS environment (you won't have to run them though):
I think the easiest change would be to:
export class MyFunctionWithDecorator {
returnValue: string;
constructor(customResponseValue: string) {
this.returnValue = customResponseValue;
}
....
@tracer.captureMethod()
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
public myMethod(): string {
return this.returnValue;
}
}
export const handlerClass = new MyFunctionWithDecorator(customResponseValue);
export const handler = handlerClass.handler.bind(handlerClass);After the change is done I'll run the e2e tests and share the result here. If successful I'll approve the PR. Finally, to manage expectations, if everything goes well and at least another maintainer is available, we are looking at releasing this change on Tue/Wed next week (Monday is a bank holiday in most of our countries so it's likely we won't be able to review/release). I will also be creating two more issues related to Logger & Metrics since they also are affected from the same issue you're fixing here. |
|
@dreamorosi Alright. I've made the E2E test changes, and as of 8ee7f96 they succeed on my account, but notwithstanding, I suggest independently verifying. Aside: It's pretty cool using the CDK to test like this. :)
|
|
Thank you for the changes & for running them, I have also ran them and was able to see the same result. Thanks, we are using CDK as part of the Jest phases to temporarily provision the functions & other adjacent infrastructure. If this was a branch from one of the maintainers we would be able to run them also in GitHub Actions and see the result here without having to pull the code. We are working on making this possible safely also for code that comes from forks, so that contributors don't have to provision infrastructure in their environments. For the next maintainer, see full logs below:
```sh
npm run test:e2e -w packages/tracer
Bundling asset Tracer-E2E-nodejs16x-c697f-AllFeatures-Manual/Tracer-E2E-nodejs16x-c697f-AllFeatures-Manual/Code/Stage... ...d265293d330dc7fd874e12c32628ec2ddf9e779df177a313de45199/index.js 1.7mb ⚡ Done in 510ms ...9671ee48425e7d7c883c2d5be1ef1fb251d8813dd015d126719311c/index.js 1.7mb ⚡ Done in 512ms ...c82f38f581b0cd2e72f135a501be636966737e480f273cd3f5883be/index.js 1.7mb ⚡ Done in 558ms ...401571eb8514599311322bc71e70bb8de47e817d23685c51cf8613f/index.js 1.7mb ⚡ Done in 607ms PASS AWS Lambda Powertools utility: TRACER tests/e2e/allFeatures.middy.test.ts (121.675 s) PASS AWS Lambda Powertools utility: TRACER tests/e2e/allFeatures.manual.test.ts (124.108 s) PASS AWS Lambda Powertools utility: TRACER tests/e2e/allFeatures.decorator.test.ts (131.776 s) Test Suites: 4 passed, 4 of 9 total |
There was a problem hiding this comment.
Thanks a lot @misterjoshua for taking the time to investigate & make all the changes, I'm sure this will help a lot of other customers.
|
I have requested a review from two other maintainers, we have a two approvals model so as soon as one of them reviews (and any comment they might have is addressed), we'll be able to merge. I have also opened two more issues that document the same exact issue but for Logger & Metrics:
Not sure if you're interested or if you're even an user of those two utilities, but in case you'd like to contribute please feel free to leave a comment under the relevant issue. I'll be happy to work with you to review the PR. If not, thanks a lot for the great work you've done already to uncover & fix the issue with Tracer. |
Thank you. I'll keep an eye on this PR in case there are any changes.
Unfortunately, I can't take on #1057 or #1058 at this time, but I'll check back in a few weeks to see where we're at. |
|
Note to reviewers: don't merge this until #1054 is merged. |
There was a problem hiding this comment.
Approved.
Discussed with @dreamorosi , we'll go with this to push the fix first.
The example will be updated in a separated PR.
Description of your changes
This PR fixes bugs in the
tracer.captureMethod()andtracer.captureLambdaHandler()decorators. These decorators provide the class prototype as thethisscope and not the instance of the class. While the prototype scope allows member functions to be run, it does not allow the customer to access instance member variables, as these instance member variables are not part of the class prototype.Shout out to @dreamorosi for assistance in building an acceptable proof of concept in Slack.
How to verify this change
I have added two new unit tests which reproduce the bug. You may run them to verify the change.
Related issues, RFCs
This is related to #1028. #1026 previously attempted to fix this and got it us part of the way to a fix.
Issue number: #1056
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
Breaking change checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.