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

fix: wrong scope in captureMethod #1026

Merged
merged 6 commits into from
Jul 29, 2022
Merged

Conversation

ratscrew
Copy link
Contributor

@ratscrew ratscrew commented Jul 26, 2022

using the captureMethod decorator the "this" is no longer the original obj so changing to the target fixes this

Description of your changes

change the binding of the decorated method back to the target so the below code will work:

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

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

   otherMethod(){}
}

How to verify this change

Related issues, RFCs

#1028

PR status

Is this ready for review?: YES
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding changes to the examples
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

using the captureMethod decorator the "this" is no longer the original obj so changing to the target fixes this
@dreamorosi
Copy link
Contributor

Hi @ratscrew thank you for taking the time to open a PR to this repo.

Before investing time in reviewing the PR, could you please open an issue to report the bug and provide also reproduction steps / example of this bug? We'd like to understand more about what's happening.

Then, after we have all agreed that it's in fact a bug, please review & edit the PR body and make sure to fill/check all the relevant sections.

@ratscrew
Copy link
Contributor Author

@dreamorosi thanks for your quick response i have added in the bug issue.

thanks

@dreamorosi
Copy link
Contributor

Thank you for opening the issue and providing more info, as I mentioned in the issue I was able to reproduce the bug.

The fix that you are proposing seems like it would fix the issue. I was wondering if you could also please add a couple of tests to make sure it's the case and also to avoid future regressions.

For the unit tests you can use this one as a reference: https://github.com/awslabs/aws-lambda-powertools-typescript/blob/main/packages/tracer/tests/unit/Tracer.test.ts#L892-L934 and add one in that same file & test group. Ideally we should have a passing test similar to the one in your issue.

packages/tracer/src/Tracer.ts Outdated Show resolved Hide resolved
@dreamorosi dreamorosi added logger This item relates to the Logger Utility fix labels Jul 28, 2022
@dreamorosi dreamorosi linked an issue Jul 28, 2022 that may be closed by this pull request
@dreamorosi
Copy link
Contributor

Thank you @ratscrew, we are really close, just some minor styling comments in one file - thanks for adding the unit test.

Looking forward to merge this!

@ratscrew ratscrew changed the title Update Tracer.ts fix: wrong scope in captureMethod Jul 28, 2022
@dreamorosi
Copy link
Contributor

It seems that the linting checks are not passing:

Screenshot 2022-07-28 at 18 24 28

According to the style we are using methods inside classes must be ordered alphabetically by name.

@dreamorosi dreamorosi self-requested a review July 29, 2022 09:12
Copy link
Contributor

@dreamorosi dreamorosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ratscrew for the contribution and for taking the time to address the review comments, looks good to me.

I'm going to request the review of a second maintainer as we have a 2-approvals model before merging.

Note to the next maintainer: the checks are failing due to #991 but the tests, lint, etc. are OK

@dreamorosi
Copy link
Contributor

image

image

image

Ran integration tests manually, they are all passing

@dreamorosi dreamorosi merged commit 1a06fed into aws-powertools:main Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logger This item relates to the Logger Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: @tracer.captureMethod() is not bound to the decorated class
3 participants