feat(tracer): allow disabling result capture for decorators and middleware #1065

misterjoshua · 2022-08-18T18:38:13Z

Description of your changes

This PR adds options to tracer.captureMethod(), tracer.captureLambdaHandler() and the captureLambdaHandler middleware, allowing the user to disable response capture on a per-method basis. This change helps customers control when the tracer should capture information that might be sensitive or larger than 64K, but without disabling these capture mechanisms globally via POWERTOOLS_TRACER_CAPTURE_RESPONSE=false environment variable.

How to verify this change

I have added multiple unit tests that simulate adding captureResponse: false to tracer.captureMethod(), tracer.captureLambdaHandler(), and the Middy captureLambdaHandler middleware. I have also added e2e tests for all three.

Related issues, RFCs

Issue number: #1064

PR status

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

Checklist

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.

…apture

packages/tracer/src/Tracer.ts

dreamorosi · 2022-08-18T19:24:44Z

Hi @misterjoshua thank you for opening this PR.

While we appreciate your bias for action a lot, we would really appreciate if next time you could give yourself and us both, and opportunity to discuss the design of the feature and the scope of the changes. According to the contributing guidelines (point 3 of the Summary) the process for submitting a PR would be to first express interest under the issue and then to mutually agree on the design.

Note: In your last PR I didn't mention this explicitly because we had this agreement in the Slack channel and I also recognise that maybe this is not extra clear from the guidelines nor it was clear in my wording under the issue you opened a couple of hours ago.

Having this discussion beforehand would help both parties with understanding if anyone is already working on the feature but also to avoid you to do unnecessary changes (even though from a first quick review in this specific PR I agree with most of your changes / direction). Additionally, it allow us to budget the time necessary to review & support the PR.

With that said, let's maybe discuss briefly the scope / requirements for this kind of feature before proceeding with the review:

For each new feature / behavior we need to have full unit test coverage as well as integration test cases (same as last time, we don't ask you to run them if you don't want to, just let us know when to run them in a comment & we'll share the result so you don't get billed for these runs)
In this case it would be great to have the behavior implemented for both handler & methods (my comment above was specifically related to the change you made on the error flag - not the whole optional response capture on handler)
Whenever possible we strive to support both decorators & Middy middleware. In this case, this PR would need to include the same option to suppress the response capture to the captureLambdaHandler middleware here (If you're not familiar with Middy please let me know and I'll be happy to give you details of how/where I think the changes should go in that file/logic).

Again, I really appreciate that you opened a PR for an issue that you opened. With the comments above I don't mean to put you off but simply I would like to establish some way of working that will hopefully result in a pleasant and productive collaboration on both parties. Thank you for your understanding!

misterjoshua · 2022-08-18T22:30:32Z

I appreciate your feedback on these procedural matters. It's critical to ensure that we're collaborating effectively.

I take your points about understanding whether anyone is already working on the feature and allocating time for PR support and review. It seemed that because I didn't see any PRs and the issue was new, it was reasonable to conclude that nobody was working on it. My intention here was, as you say, based on a bias for action. I intended to tread a path and leave this PR open until your team had time to review and discuss it further. (i.e., not right away, because I understand that we're all busy and can't simply drop what we're doing.)

I understand you want to discuss the scope/requirements for changes before diving in. Perhaps I assumed prematurely that this project would seek to replicate the AWS Lambda Powertools for Python decorator APIs, which were directly translatable. In the spirit of treading a path, I had taken a crack at translating these APIs to TypeScript before I started removing code in the last two commits. The work was uncomplicated, so I figured the easiest way to demonstrate it was to do it and wait until you were ready to review. I didn't intend to make you feel like you had to jump to my attention. Next time I'll take your comments into consideration.

I'm not sure where that leaves us. Your three scope/requirement points are reasonable, and I can perform that work. But, would it be easier for your team if I were to close this PR in favour of further discussion in #1064? Alternatively, I could do the work and let the PR sit until you're ready. I'll wait until you let me know which way you want to go.

Either way, here are my responses to your scope/requirements comments:

For each new feature / behavior we need to have full unit test coverage as well as integration test cases (same as last time, we don't ask you to run them if you don't want to, just let us know when to run them in a comment & we'll share the result so you don't get billed for these runs)

I had provided unit tests with 100% branch coverage, but I can also add integration test coverage.

In this case it would be great to have the behavior implemented for both handler & methods (my comment above was specifically related to the change you made on the error flag - not the whole optional response capture on handler)

Ok. I undid captureLambdaHandler in a separate commit (ae72d52) so that this change was easily reversible - I can revert that change and start from there.

Whenever possible we strive to support both decorators & Middy middleware. In this case, this PR would need to include the same option to suppress the response capture to the captureLambdaHandler middleware here (If you're not familiar with Middy please let me know and I'll be happy to give you details of how/where I think the changes should go in that file/logic).

Adding Middy support should be fine. The first thing that comes to my mind is to change captureLambdaHandler's signature to (target: Tracer, options?: CaptureLambdaHandlerOptions): middy.MiddlewareObj, then adjust captureLambdaHandlerAfter to make addResponseAsMetadata conditional. Using it would look like this:

const tracer = new Tracer();
export const handler = middy(lambdaHandler)
    .use(captureLambdaHandler(tracer, { captureResponse: false }));

But what do you think?

dreamorosi · 2022-08-18T23:35:45Z

I'm glad that we are on the same page!

Indeed the expectation that no one was working on this PR was reasonable. I just wanted to convey the point in general and used the current issue and PR to do so.

There's no need to close this PR, we're happy with you continuing to work on it if you would like to do so. From my side I'm looking forward to help with the review and iterations and you can move forward whenever it suits you.

Regarding unit tests, indeed I noticed that coverage was already 100%, wanted to point out the integration ones purely for future reference. In this case given it's a new feature that will require adding some new assertions, maybe let's discuss a high level design beforehand - let me know if you're comfortable with extending a proposal or if you'd like me to share ideas.

About the Middy implementation, the proposal looks sensible and aligns with my current thinking, let's move forward and see how it feels once implemented.

Finally, you are also right in wanting to replicate Python's behavior whenever possible and at least for the three core utilities that we have now. At the same time we want to respect and appreciate the differences between languages and make sure this version feels idiomatic to JS/TS devs. I'm mentioning this only as reference, for the purposes of this feature you were already going in the right direction.

Again, I want to thank you for being gracious about this interaction and understanding where I was coming from with my comment.

Looking forward to continue working on this PR with you (or future ones if the opportunity arises), and if you need my input please let me know.

I'll assign the PR to you, and also the issue.

misterjoshua · 2022-08-18T23:49:34Z

Looking forward to continue working on this PR with you (or future ones if the opportunity arises), and if you need my input please let me know.

Sounds good. I'll get to work! :)

This reverts commit ae72d52.

misterjoshua · 2022-08-19T02:37:43Z

I've pushed up some preliminary changes aimed at addressing the initial feedback. It all works according to my testing at this time, but I'll take another look tomorrow with fresh eyes.

On the subject of assertions: I have proposed some new assertions as samples in the new e2e & unit tests. We can change all of that if you like. I tried a few odd things in the e2e tests to set up and test for what I believe the behaviour should be. But, I've separated these out so we can reverse the proposed e2e test changes if you have better ideas for how to test.

Meanwhile, here's the e2e output:

npm run test:e2e output in packages/tracer


> @aws-lambda-powertools/[email protected] test:e2e
> jest --group=e2e

Bundling asset Tracer-E2E-nodejs16x-c7394-AllFeatures-Decorator/Tracer-E2E-nodejs16x-c067f-AllFeatures-Decoratory-AllFlagsEnable/Code/Stage...
Bundling asset Tracer-E2E-nodejs16x-aefab-AllFeatures-Middy/Tracer-E2E-nodejs16x-463f8-AllFeatures-Middy-AllFlagsEnabled/Code/Stage...
Bundling asset Tracer-E2E-nodejs16x-d8349-AllFeatures-Manual/Tracer-E2E-nodejs16x-d8349-AllFeatures-Manual/Code/Stage...
Bundling asset Tracer-E2E-nodejs16x-bec06-AllFeatures-Decorator/Tracer-E2E-nodejs16x-7ece5-AllFeatures-Decoratory-AllFlagsEnable/Code/Stage...

  ...ff84c85b11ddf122a76bc2e28f56cc7d1bde0ffba85d9ab7f6e7fe5/index.js  1.7mb ⚠️

⚡ Done in 122ms

  ...76d62a419e3adf897632fa7c49a1d564d3112366bba86d48fba07e2/index.js  1.7mb ⚠️

⚡ Done in 135ms

  ...6a6369156dbe37f42161998d1201b4f10d8295b83c207a8d390af85/index.js  1.7mb ⚠️

⚡ Done in 137ms

  ...ddb8fa9630aa8f7480ee927ac4ef86fb58f5af38663b518e4fdab79/index.js  1.7mb ⚠️

⚡ Done in 103ms
[0%] start: Publishing c1188543eac34c1e43a821523401c4ac1e55de91b92f474288b6c3f8b61033b3:current_account-current_region
[0%] start: Publishing ffbcdd9cc9ccd352174fce12d36369be07a3016843d911c57feb7e93916fed58:current_account-current_region
[0%] start: Publishing ef402b8fb4bb3684b95615f2fe20da5b1970f86fcd55d227dfe4c95842899dd8:current_account-current_region
[0%] start: Publishing c812bdb83753a544e4737f85d76391976d03260e52cc7bed346bcff1e8b694ec:current_account-current_region
[0%] start: Publishing ae1b8428112636cff548ffba7a5e44ebb71bfa9a7882b7af6dbbe58fda63858d:current_account-current_region
[0%] start: Publishing 0914cfc9b93745260e7c82139cf111011a6e426ff4d258a8bf0b3bca1b3df960:current_account-current_region
[0%] start: Publishing 3e599f445e817e3ae85316bf3a4a4a0ab12ed1e43be210e02204f14601fdf946:current_account-current_region
[0%] start: Publishing 4dc3785cbcd4d52d2855a88cd0b0c6c103b144a48b73cdb487fa27e11f32cc3d:current_account-current_region
[50%] success: Published ef402b8fb4bb3684b95615f2fe20da5b1970f86fcd55d227dfe4c95842899dd8:current_account-current_region
[50%] success: Published 0914cfc9b93745260e7c82139cf111011a6e426ff4d258a8bf0b3bca1b3df960:current_account-current_region
[50%] success: Published c1188543eac34c1e43a821523401c4ac1e55de91b92f474288b6c3f8b61033b3:current_account-current_region
[50%] success: Published ae1b8428112636cff548ffba7a5e44ebb71bfa9a7882b7af6dbbe58fda63858d:current_account-current_region
[100%] success: Published 4dc3785cbcd4d52d2855a88cd0b0c6c103b144a48b73cdb487fa27e11f32cc3d:current_account-current_region
[100%] success: Published c812bdb83753a544e4737f85d76391976d03260e52cc7bed346bcff1e8b694ec:current_account-current_region
[100%] success: Published ffbcdd9cc9ccd352174fce12d36369be07a3016843d911c57feb7e93916fed58:current_account-current_region
[100%] success: Published 3e599f445e817e3ae85316bf3a4a4a0ab12ed1e43be210e02204f14601fdf946:current_account-current_region
Tracer-E2E-nodejs16x-c7394-AllFeatures-Decorator: creating CloudFormation changeset...
Tracer-E2E-nodejs16x-aefab-AllFeatures-Middy: creating CloudFormation changeset...
Tracer-E2E-nodejs16x-bec06-AllFeatures-Decorator: creating CloudFormation changeset...
Tracer-E2E-nodejs16x-d8349-AllFeatures-Manual: creating CloudFormation changeset...
PASS AWS Lambda Powertools utility: TRACER tests/e2e/asyncHandler.decorator.test.ts (119.032 s)
  ● Console

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876318 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-c067f-AllFeatures-Decoratory-AllFlagsEnable"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876324 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-c067f-AllFeatures-Decoratory-AllFlagsEnable"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876324 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-c067f-AllFeatures-Decoratory-AllFlagsEnable"'

      at tests/helpers/tracesUtils.ts:95:13

PASS AWS Lambda Powertools utility: TRACER tests/e2e/allFeatures.manual.test.ts (124.818 s)
  ● Console

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876318 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-d8349-AllFeatures-Manual"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876324 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-d8349-AllFeatures-Manual"'

      at tests/helpers/tracesUtils.ts:95:13

PASS AWS Lambda Powertools utility: TRACER tests/e2e/allFeatures.middy.test.ts (125.742 s)
  ● Console

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876318 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-463f8-AllFeatures-Middy-AllFlagsEnabled"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876324 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-463f8-AllFeatures-Middy-AllFlagsEnabled"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876324 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-463f8-AllFeatures-Middy-AllFlagsEnabled"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876324 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-4d4be-AllFeatures-Middy-NoCaptureErrorOrRes"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876325 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-96996-AllFeatures-Middy-NoCaptureResponse2"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876325 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-7e6d6-AllFeatures-Middy-TracerDisabled"'

      at tests/helpers/tracesUtils.ts:95:13

PASS AWS Lambda Powertools utility: TRACER tests/e2e/allFeatures.decorator.test.ts (130.51 s)
  ● Console

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876322 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-7ece5-AllFeatures-Decoratory-AllFlagsEnable"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876328 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-7ece5-AllFeatures-Decoratory-AllFlagsEnable"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876329 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-7ece5-AllFeatures-Decoratory-AllFlagsEnable"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876329 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-49506-AllFeatures-Decorator-NoCaptureErrorO"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876329 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-d61b3-AllFeatures-Decorator-CaptureResponse"'

      at tests/helpers/tracesUtils.ts:95:13

    console.log
      Manual query: aws xray get-trace-summaries --start-time 1660876237 --end-time 1660876330 --filter-expression 'resource.arn = "arn:aws:lambda:eu-west-1:812696460994:function:Tracer-E2E-nodejs16x-16416-AllFeatures-Decorator-TracerDisabled"'

      at tests/helpers/tracesUtils.ts:95:13


Test Suites: 4 passed, 4 of 9 total
Tests:       14 passed, 14 total
Snapshots:   0 total
Time:        131.013 s, estimated 138 s
Ran all test suites.

dreamorosi

Thank you for all the changes & implementation so far.

I have left some comments - nothing major - on the implementation, docs, and unit tests.

I'm gonna need a bit more time to digest & think through the integration tests, I have also asked a second opinion to other maintainers.

We'll get in touch as soon as we have an opinion

packages/tracer/src/middleware/middy.ts

packages/tracer/src/types/Tracer.ts

docs/core/tracer.md

packages/tracer/src/middleware/middy.ts

dreamorosi

Hi @misterjoshua, back from the weekend and I have reviewed the changes. Thank you for addressing all the comments either with changes or comments. I'm happy with where we are at now.

I have left one minor comment about an import statement for a type, but other than that I'm already marking this as approved.

Will be waiting for a second review by another maintainer before merging.

Co-authored-by: Andrea Amorosi <[email protected]>

misterjoshua · 2022-08-22T15:38:57Z

@dreamorosi Thanks! I applied your suggested change. It looks like by doing that, GitHub automatically dismissed your review - sorry about that. I'll keep an eye out for any more change requests. Thanks for your help so far.

ijemmy

The PR looks perfect to me now, comprehensive test cases, easy to ready, well-documented.

I find it interesting to use inheritance. in e2e decorator tests. I don't usually think about that option when writing tests. But it fits well here.

Thanks for your efforts @misterjoshua , @dreamorosi :)

misterjoshua added 2 commits August 18, 2022 18:27

feat(tracer): use decorator options to disable exception and result c…

cc59160

…apture

docs: small inline docs improvements

6eedbbe

misterjoshua marked this pull request as ready for review August 18, 2022 18:45

dreamorosi reviewed Aug 18, 2022

View reviewed changes

packages/tracer/src/Tracer.ts Outdated Show resolved Hide resolved

misterjoshua added 2 commits August 18, 2022 18:54

chore: remove the captureError option

14ccc2f

chore: remove all captureLambdaHandler decorator changes

ae72d52

github-actions bot added feature labels Aug 18, 2022

misterjoshua requested a review from dreamorosi August 18, 2022 19:17

dreamorosi linked an issue Aug 18, 2022 that may be closed by this pull request

Feature request: Tracer disable response capturing on a per-method basis #1064

Closed

dreamorosi assigned misterjoshua Aug 18, 2022

misterjoshua added 4 commits August 18, 2022 17:50

Revert "chore: remove all captureLambdaHandler decorator changes"

81fd698

This reverts commit ae72d52.

feat: add middy middleware options

e456670

test: add middy options e2e

dd42e52

test: add decorator e2e

cb4de92

misterjoshua changed the title ~~feat(tracer): decorators can disable exception and result capture~~ feat(tracer): decorators can disable result capture Aug 19, 2022

misterjoshua changed the title ~~feat(tracer): decorators can disable result capture~~ feat(tracer): allow disabling result capture for decorators and middleware Aug 19, 2022

dreamorosi reviewed Aug 19, 2022

View reviewed changes

dreamorosi requested a review from ijemmy August 19, 2022 13:58

refactor: initial review changes

2c84e5f

dreamorosi reviewed Aug 22, 2022

View reviewed changes

packages/tracer/src/middleware/middy.ts Outdated Show resolved Hide resolved

dreamorosi previously approved these changes Aug 22, 2022

View reviewed changes

misterjoshua dismissed dreamorosi’s stale review via 9597970 August 22, 2022 15:36

refactor: apply import change from review

9597970

Co-authored-by: Andrea Amorosi <[email protected]>

dreamorosi approved these changes Aug 22, 2022

View reviewed changes

ijemmy approved these changes Aug 23, 2022

View reviewed changes

dreamorosi merged commit c3b9a37 into aws-powertools:main Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracer): allow disabling result capture for decorators and middleware #1065

feat(tracer): allow disabling result capture for decorators and middleware #1065

misterjoshua commented Aug 18, 2022 •

edited

Loading

dreamorosi commented Aug 18, 2022

misterjoshua commented Aug 18, 2022 •

edited

Loading

dreamorosi commented Aug 18, 2022

misterjoshua commented Aug 18, 2022

misterjoshua commented Aug 19, 2022 •

edited

Loading

dreamorosi left a comment

dreamorosi left a comment

misterjoshua commented Aug 22, 2022

ijemmy left a comment

feat(tracer): allow disabling result capture for decorators and middleware #1065

feat(tracer): allow disabling result capture for decorators and middleware #1065

Conversation

misterjoshua commented Aug 18, 2022 • edited Loading

Description of your changes

How to verify this change

Related issues, RFCs

PR status

Checklist

Breaking change checklist

dreamorosi commented Aug 18, 2022

misterjoshua commented Aug 18, 2022 • edited Loading

dreamorosi commented Aug 18, 2022

misterjoshua commented Aug 18, 2022

misterjoshua commented Aug 19, 2022 • edited Loading

dreamorosi left a comment

Choose a reason for hiding this comment

dreamorosi left a comment

Choose a reason for hiding this comment

misterjoshua commented Aug 22, 2022

ijemmy left a comment

Choose a reason for hiding this comment

misterjoshua commented Aug 18, 2022 •

edited

Loading

misterjoshua commented Aug 18, 2022 •

edited

Loading

misterjoshua commented Aug 19, 2022 •

edited

Loading