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(tracer): properly return DynamoDB.DocumentClient #528

Merged
merged 4 commits into from
Feb 8, 2022

Conversation

dreamorosi
Copy link
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
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

Copy link
Contributor

@flochaz flochaz left a comment

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 ?

packages/tracing/src/Tracer.ts Outdated Show resolved Hide resolved
@dreamorosi dreamorosi requested a review from ijemmy February 1, 2022 09:34
@dreamorosi
Copy link
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
Copy link
Contributor

@ijemmy ijemmy left a comment

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.

// 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
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
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
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 ijemmy and flochaz 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