-
Notifications
You must be signed in to change notification settings - Fork 156
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
Using captureAWSClient
over directly referenced dynamodb client fails
#23
Comments
Hi nolde, I haven't been able to reproduce your issue.
And the resultant trace:
Can you post a code snippet to reproduce the issue? Are there any errors in the logs? Thanks, |
After reviewing the issue, I found out that it was actually my mistake. Thanks for the quick reply, though. |
And should the usage below work? var xray = require('aws-xray-sdk-core')
const AWS = require('aws-sdk')
const docclient = xray.captureAWSClient(new AWS.DynamoDB.DocumentClient()) |
The forums post here covers this: https://forums.aws.amazon.com/thread.jspa?messageID=821049󈜹 The DocumentClient constructor is not captured in the same way the base level clients are - this is a special case of the AWS SDK not conforming to the same initialization code path that the other AWS Clients utilize and the hook they provide to us does not properly set up tracing. We are currently investigating a work around for this issue. Hope this helps, |
Ok! |
Is this issue resolved now? |
Any progress? |
Re-open due to popular requests. We need to work with AWS SDK team so there is a proper public interface to plugin the X-Ray intercepter on the Document client. This is on our backlog and we already brought this issue to AWS SDK team's attention. We will post an update as soon as we have further information. Please stay tuned and thank you for your patience. |
Here is a workaround (from aws/aws-sdk-js#1846) const AWSXray = require('aws-xray-sdk');
const AWS = require('aws-sdk');
const client = new AWS.DynamoDB.DocumentClient({
service: new AWS.DynamoDB({...})
});
AWSXray.captureAWSClient(client.service); |
I'm using Typescript and Webpack - here's how I'm instrumenting with the thin dynamoDB client, if that's helpful: const AWSXRay = require('aws-xray-sdk-core')
import { DocumentClient } from 'aws-sdk/clients/dynamodb';
let client: DocumentClient;
client = new DocumentClient();
AWSXRay.captureAWSClient((client as any).service); |
Hi @heitorlessa Thanks for sharing how you are instrumenting DDB. It worked with
const AWSXRay = require("aws-xray-sdk");
const lambda = new Lambda();
AWSXRay.captureAWSClient((lambda as any).service); Have you tried to use |
Hi @phstc
That's because the class Lambda extends an aws Service. |
Hi @phstc, |
I'm not sure why this has been closed. It's still not possible to patch a DynamoDB Client class directly. @willarmiros can you reopen, please? |
Sure. This issue was just getting some clutter but the bug does still exist. Unfortunately this and #184 are reliant on the AWS SDK team to unify their client classes. Reopening to continue to track that progress. |
FYI, I've created npm package which encapsulates the workaround described in the comments: https://github.com/shelfio/aws-ddb-with-xray |
this is getting closed for no good reason, the problem still exists and no proper solutions has been presented here yet. |
@rpstreef have you tried the workaround suggested on this thread? As we've mentioned this is a problem with how the Document Client is modeled in the AWS SDK itself. Given that the AWS SDK for JS team is soon to release V3 of their SDK, it's unlikely they'll make this change to the DynamoDB client in V2. Furthermore, this is not a problem in V3 of the AWS SDK since the DynamoDB document client is implemented in the same way as other clients and will be able to be traced appropriately. Please keep an eye on #298 for our support of V3 of the AWS SDK. |
How exactly do I use this workaround? there is something missing here |
@paul-uz If you're asking about the If you're asking how this client is actually traced, you'll notice after creating the DocumentClient, they use:
To enable X-Ray tracing the underlying DynamoDB client. |
But this way the return type is DynamoDB not DocumentClient which in my case is no good as it requires me to rewrite my whole service to use the DynamoDB client API instead of DocumentClient one. So there is currently no way to directly instrument DocumentClient? |
@mkapiczy in this solution: const AWSXray = require('aws-xray-sdk');
const AWS = require('aws-sdk');
const client = new AWS.DynamoDB.DocumentClient({
service: new AWS.DynamoDB({...})
});
AWSXray.captureAWSClient(client.service);
|
My bad, I thought it doesn't modify given object but returns new instrumented instance. Works! Thanks! |
I am using this: import AWS from "aws-sdk"
import AWSXRay from "aws-xray-sdk"
AWSXRay.setContextMissingStrategy(() => {})
// clients
const isTest = process.env.JEST_WORKER_ID
const config: AWS.DynamoDB.ClientConfiguration = {
...(isTest && {
endpoint: "localhost:8000",
sslEnabled: false,
region: "local-env",
}),
}
// instantiate xray-instrumented dynamoDB document client
export const documentClient = new AWS.DynamoDB.DocumentClient({
...config,
})
// this adds x-ray instrumentation to dynamo's DocumentClient
// but also adds 5s of startup time to the lambda???
AWSXRay.captureAWSClient((documentClient as any).service) The problem is if I comment out the |
Are you using a bundler like webpack to bundle the lambda and/or source maps by any chance? |
I am using the bundler that https://docs.aws.amazon.com/cdk/api/latest/docs/aws-lambda-nodejs-readme.html#local-bundling uses - esbuild |
@revmischa the performance issue described is definitely new - does it impact all clients or just the document client? Since this is a workaround though it's not surprising there's some odd behavior. |
The following code does not work:
In theory, it should be just the same as encapsulating
new AWS.DynamoDB()
coming straight fromaws-sdk
, but it does not work.This is specially problematic in AWS Lambda environment, where loading more than you need makes you use more memory and more time.
The text was updated successfully, but these errors were encountered: