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

feat(tracer): add support for capturing DynamoDB DocumentClient #450

Merged
merged 4 commits into from
Jan 25, 2022

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Jan 10, 2022

Description of your changes

Customer feedback (link) has pointed out a quirk of instrumenting a DynamoDB DocumentClient, in their words:

One thing that isn't solved here (yet?) is the oddness of working with the X-Ray SDK and DynamoDB DocumentClient. When using DocumentClient with X-Ray, we need to do a workaround as the SDK needs access to the DocumentClient service property, but that's not exposed on the type. Here's how I've solved that problem in the past.

The new implementation attempts to instrument a client normally (reference) first and then, if that fails, it tries to do so by accessing the service property. If both fail it throws as expected.

The advantage of this new mechanism (see example below) is that customers can instrument the DocumentClient in the same way as they do with other AWS SDK clients thus providing a more ergonomic API (& hopefully better DX).

How to verify this change

Before:

import DynamoDB, { DocumentClient } from 'aws-sdk/clients/dynamodb';
import { Tracer } from '@aws-lambda-powertools/tracer';

const tracer = new Tracer({ serviceName: 'paymentCollections' });
const client = new DocumentClient();
tracer.captureAWSClient((client as DocumentClient & { service: DynamoDB }).service); // Explicit typing required in userland

After:

import { DocumentClient } from 'aws-sdk/clients/dynamodb';
import { Tracer } from '@aws-lambda-powertools/tracer';

const tracer = new Tracer({ serviceName: 'paymentCollections' });
const client = new DocumentClient();
tracer.captureAWSClient(client); // Like other AWS SDK v2 clients

// OR const client = tracer.captureAWSClient(new DocumentClient);

Related issues, RFCs

N/A

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 enhancement tracer This item relates to the Tracer Utility labels Jan 10, 2022
@dreamorosi dreamorosi added this to the production-ready-release milestone Jan 10, 2022
@dreamorosi dreamorosi self-assigned this Jan 10, 2022
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 we add an e2e test ?

@dreamorosi
Copy link
Contributor Author

Could we add an e2e test ?

Good idea, will do and come back to you

@dreamorosi dreamorosi requested a review from flochaz January 24, 2022 18:25
@dreamorosi
Copy link
Contributor Author

Swapped STS calls for DynamoDB calls in all e2e tests for Tracer. Checks are now failing for unrelated reason, need to check why.

@flochaz
Copy link
Contributor

flochaz commented Jan 24, 2022

you probably need to rebase first

@dreamorosi dreamorosi force-pushed the feat/tracer/accept_dynamodb_documentclient branch from ac09115 to 31969a3 Compare January 24, 2022 19:27
@dreamorosi
Copy link
Contributor Author

you probably need to rebase first

You were right, that worked. Thanks a lot, now checks are passing!

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.

Great DX improvement :)

btw, I can't find this branch run in E2E workflow. Normally this should be triggerred in "Approve" step but we don't have this for core maintainer.

Is there a way for us to (manually?) run this before merging?

@flochaz
Copy link
Contributor

flochaz commented Jan 25, 2022

Great DX improvement :)

btw, I can't find this branch run in E2E workflow. Normally this should be triggerred in "Approve" step but we don't have this for core maintainer.

Is there a way for us to (manually?) run this before merging?

Screenshot 2022-01-25 at 10 47 40

@dreamorosi
Copy link
Contributor Author

dreamorosi commented Jan 25, 2022

@flochaz @ijemmy ran the workflow, passing result here.
In next PR, where applicable, I'll provide it as part of the checklist.

@dreamorosi dreamorosi merged commit 621ae50 into main Jan 25, 2022
@dreamorosi dreamorosi deleted the feat/tracer/accept_dynamodb_documentclient branch January 25, 2022 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracer This item relates to the Tracer Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants