Skip to content

fix(tracer): properly return DynamoDB.DocumentClient - #528

Merged
dreamorosi merged 4 commits into
mainfrom
fix/tracer/return_proper_documentclient
Feb 8, 2022
Merged

fix(tracer): properly return DynamoDB.DocumentClient#528
dreamorosi merged 4 commits into
mainfrom
fix/tracer/return_proper_documentclient

Conversation

@dreamorosi

Copy link
Copy Markdown
Contributor

Description of your changes

This PR aims at fixing the bug identified in #524.

How to verify this change

Check GitHub actions execution results, the unit tests now include test cases to assert on the type of client being returned.

Related issues, RFCs

#524

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
  • 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
  • The PR title follows the conventional commit semantics

Breaking change checklist

N/A


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

@dreamorosi dreamorosi added tracer This item relates to the Tracer Utility fix labels Jan 31, 2022
@dreamorosi dreamorosi self-assigned this Jan 31, 2022
@dreamorosi dreamorosi changed the title fixP: properly return DynamoDB.DocumentClient fix(tracer): properly return DynamoDB.DocumentClient Jan 31, 2022
@dreamorosi dreamorosi added this to the production-ready-release milestone Jan 31, 2022
@github-actions github-actions Bot added the bug Something isn't working label Jan 31, 2022
@dreamorosi

Copy link
Copy Markdown
Contributor Author

Attaching an archive with a packed version of this branch (npm pack) and the same code you find in the linked issue (#524 ) so that you can verify the change easily. Just download, unzip, and run npm i in the folder to run the code (set up a DynamoDB table using the provided createTable.json).

fix_test.zip

@flochaz flochaz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add get or put calls in the e2e as well ?

Comment thread packages/tracing/src/Tracer.ts Outdated
@dreamorosi
dreamorosi requested a review from ijemmy February 1, 2022 09:34
@dreamorosi

Copy link
Copy Markdown
Contributor Author

Here at this link you can find a successful e2e test execution with the new changes.

flochaz
flochaz previously approved these changes Feb 1, 2022
ijemmy
ijemmy previously approved these changes Feb 4, 2022

@ijemmy ijemmy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Thanks for also updating the e2e tests.

Comment thread packages/tracing/src/Tracer.ts Outdated
// This is needed because some aws-sdk clients like AWS.DynamoDB.DocumentDB don't comply with the same
// instrumentation contract like most base clients.
// For detailed explanation see: https://github.com/awslabs/aws-lambda-powertools-typescript/issues/524#issuecomment-1024493662
this.provider.captureAWSClient((service as unknown as T & { service: T }).service);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the detailed investigation!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dreamorosi question just for myself.

Why do we cast to unknown first and as T in the 2nd step?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, it's a leftover from a previous experiment and it's not needed since T is already a generic. Fixed in latest commit.

@dreamorosi
dreamorosi dismissed stale reviews from ijemmy and flochaz via 37c8083 February 4, 2022 16:30
@dreamorosi
dreamorosi requested review from flochaz and ijemmy February 4, 2022 16:30
@dreamorosi
dreamorosi merged commit 3559e7b into main Feb 8, 2022
@dreamorosi
dreamorosi deleted the fix/tracer/return_proper_documentclient branch February 8, 2022 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working tracer This item relates to the Tracer Utility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: tracer.captureAWSClient not working consistently with DynamoDB.DocumentClient

3 participants