Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: @tracer.captureMethod() is not bound to the decorated class #1028

Closed
ratscrew opened this issue Jul 27, 2022 · 6 comments · Fixed by #1026
Closed

Bug: @tracer.captureMethod() is not bound to the decorated class #1028

ratscrew opened this issue Jul 27, 2022 · 6 comments · Fixed by #1026
Assignees
Labels
bug Something isn't working completed This item is complete and has been merged/shipped tracer This item relates to the Tracer Utility

Comments

@ratscrew
Copy link
Contributor

Bug description

The @tracer.captureMethod() decorator is bound to the decorator not the class its decorating.

export class Lambda implements LambdaInterface {
   @tracer.captureMethod()
   myMethod(){

      //other method is undefined as "this" is no longer the Lambda class
      this.otherMethod()
   }

   otherMethod(){}
}

Expected Behavior

this.otherMethod() should call the this.otherMethod() in the same class

Current Behavior

this.otherMethod() is undefined

Possible Solution

see #1026
the @tracer.captureLambdaHandler() decorator already does this

Steps to Reproduce

  1. create a class with two or methods in it
  2. add the decorator @tracer.captureMethod() to one or more of the function
  3. call another method or prop in the decorated method
  4. the method or prop will be undefined

Environment

  • Powertools version used: 1.0.2
  • Packaging format (Layers, npm):
  • AWS Lambda function runtime:
  • Debugging logs:

Related issues, RFCs

#1026

@ratscrew ratscrew added bug Something isn't working triage This item has not been triaged by a maintainer, please wait labels Jul 27, 2022
@dreamorosi dreamorosi changed the title Bug (module name): @tracer.captureMethod() is not bound to the decorated class Bug (tracer): @tracer.captureMethod() is not bound to the decorated class Jul 27, 2022
@dreamorosi dreamorosi added the tracer This item relates to the Tracer Utility label Jul 27, 2022
@dreamorosi
Copy link
Contributor

Thank you for opening the issue, appreciate your time.

I was able to reproduce the bug described using this code:

import { Tracer } from "@aws-lambda-powertools/tracer";
import { LambdaInterface } from "@aws-lambda-powertools/commons";

const tracer = new Tracer();

export class Lambda implements LambdaInterface {
  @tracer.captureMethod()
  myMethod() {
    //other method is undefined as "this" is no longer the Lambda class
    this.otherMethod();
  }

  otherMethod() {
    console.log("other method");
  }

  @tracer.captureLambdaHandler()
  public async handler(_event: any, _context: any): Promise<void> {
    this.myMethod();
  }
}

export const handlerClass = new Lambda();
export const handler = handlerClass.handler;

Given that you have already opened a PR to fix this I'm gonna assign the issue to you.

@dreamorosi dreamorosi removed the triage This item has not been triaged by a maintainer, please wait label Jul 27, 2022
@dreamorosi dreamorosi linked a pull request Jul 28, 2022 that will close this issue
13 tasks
@github-actions
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@misterjoshua
Copy link
Contributor

@dreamorosi Hey. I tested @tracer.captureMethod() in the recently released v1.1.0. Although #1026 was included, I don't believe this issue was successfully fixed. Here is an example of code that does not work:

import { Tracer } from '@aws-lambda-powertools/tracer';

test('bug', async () => {
  const tracer = new Tracer();

  class BugClass {
    constructor(private readonly callback: () => string) {}

    @tracer.captureMethod()
    async getFoo(): Promise<string> {
      return this.callback();
    }
  }

  const myClass = new BugClass(() => 'somevalue');
  await expect(myClass.getFoo()).resolves.toEqual('somevalue');
});

Here is a screenshot demonstrating the unexpected error:

image

And here is a file from a repository that can reproduce the issue:
https://github.com/misterjoshua/lpt-tracer-bug/blob/main/test/bug.test.ts

You can test this for yourself by typing the following:

git clone https://github.com/misterjoshua/lpt-tracer-bug.git
cd lpt-tracer-bug
yarn install
yarn test

@dreamorosi
Copy link
Contributor

After looking into it a bit more it seems that the issue is not anymore related to how this is bound but instead to readonly attributes/methods of the decorated class.

For instance, the example below works as expected:

import { Tracer } from "@aws-lambda-powertools/tracer";
import { LambdaInterface } from "@aws-lambda-powertools/commons";

const tracer = new Tracer({ serviceName: "serverlessAirline" });

class BugClass {
  constructor() {}

  private ok(): void {
    console.log("HELLO");
    return;
  }

  @tracer.captureMethod()
  async getFoo(): Promise<void> {
    return this.ok();
  }
}

class Lambda implements LambdaInterface {
  @tracer.captureMethod()
  // eslint-disable-next-line @typescript-eslint/ban-ts-comment
  // @ts-ignore
  public async dummyMethod(): Promise<string> {
    return `otherMethod:${this.otherMethod()}`;
  }

  @tracer.captureLambdaHandler()
  public async handler(_event: any, _context: any): Promise<string> {
    const bugClass = new BugClass();
    await bugClass.getFoo();

    return await this.dummyMethod();
  }

  public otherMethod(): string {
    return "otherMethod";
  }
}

export const myFunction = new Lambda();
export const handler = myFunction.handler;

Screenshot 2022-08-12 at 18 44 49

To be honest I'm not sure if this is an expected behavior in TS decorators, I'd have to investigate more. But definitely I would consider this a separate issue from the first one that you reported - which was very valid and that was fixed.

@dreamorosi
Copy link
Contributor

Closing this in favor of #1056

@github-actions
Copy link
Contributor

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@dreamorosi dreamorosi changed the title Bug (tracer): @tracer.captureMethod() is not bound to the decorated class Bug: @tracer.captureMethod() is not bound to the decorated class Nov 14, 2022
@dreamorosi dreamorosi added the completed This item is complete and has been merged/shipped label Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working completed This item is complete and has been merged/shipped tracer This item relates to the Tracer Utility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants