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

Update type declaration of captureAWSv3Client to fix TS errors #575

Merged

Conversation

carolabadeer
Copy link
Contributor

Issue #, if available: #439, #547

Description of changes: This PR updates the TypeScript definition of captureAWSv3Client to fix errors reported by customers in the issues above:

Argument of type 'SSMClient' is not assignable to parameter of type 'Client<any, any, any>'.
  The types of 'middlewareStack.concat' are incompatible between these types.
     ... and many other lines...

This error was due to an incompatibility between the middlewareStack types used by the Client type defined in the @aws-sdk/types package. The root cause behind this incompatibility is conflicting nested versions of the @aws-sdk/types package being used in this repo. These versions can be verified using the following command:

npm ls @aws-sdk/types 

Which currently produces the following output:

├─┬ @aws-sdk/config-resolver@3.12.0
│ ├─┬ @aws-sdk/signature-v4@3.12.0
│ │ └── @aws-sdk/types@3.12.0 
│ └── @aws-sdk/types@3.12.0 
├─┬ @aws-sdk/node-config-provider@3.12.0
│ ├─┬ @aws-sdk/property-provider@3.12.0
│ │ └── @aws-sdk/types@3.12.0 
│ └── @aws-sdk/types@3.12.0 
├─┬ @aws-sdk/smithy-client@3.15.0
│ └── @aws-sdk/types@3.15.0 
└─┬ aws-xray-sdk-core@3.4.1 
  └── @aws-sdk/types@3.6.1 

To fix the error reported above, a minimal type can be used to extract the middlewareStack from the AWS client being passed into captureAWSv3client. This will ensure that middlewareStack.use and middlewareStack.remove exist on the client (which are the necessary components for the implementation) while avoiding the minor version incompatibilities between their types.

This change was tested using the following sample app:

import { DynamoDB, ListTablesCommand } from "@aws-sdk/client-dynamodb";
import {enableManualMode, setDaemonAddress, captureAWSv3Client, Segment} from "aws-xray-sdk"

(async () => {
  const dynamoDBClient = new DynamoDB({ region: "us-west-2" });
  
  enableManualMode();
  setDaemonAddress('0.0.0.0:2000');

  const segment = new Segment('myApplication'); 
  const instrumentedClient = captureAWSv3Client(dynamoDBClient, segment)

  try {
    const results = await instrumentedClient.send(new ListTablesCommand({}));
    console.log(results.TableNames.join("\n"));
  } catch (err) {
    console.error(err);
  } 

  segment.close();

})();

Prior to the changes, the const instrumentedClient = captureAWSv3Client(dynamoDBClient, segment) generated the error reported above. The error no longer persists after these changes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@carolabadeer carolabadeer requested a review from a team as a code owner March 8, 2023 06:30
@codecov-commenter
Copy link

Codecov Report

Merging #575 (4d072a6) into master (3abe727) will not change coverage.
The diff coverage is n/a.

❗ Current head 4d072a6 differs from pull request most recent head 5013a65. Consider uploading reports for the commit 5013a65 to get more accurate results

@@           Coverage Diff           @@
##           master     #575   +/-   ##
=======================================
  Coverage   83.38%   83.38%           
=======================================
  Files          37       37           
  Lines        1794     1794           
=======================================
  Hits         1496     1496           
  Misses        298      298           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Awesome, and thanks for the detailed writeup!

@carolabadeer carolabadeer merged commit 7e92f33 into aws:master Mar 9, 2023
@carolabadeer carolabadeer deleted the fix-captureAWSv3Client-TS-errors branch March 9, 2023 21:50
@jasonterando
Copy link
Contributor

Hi, when is your next release lined up? Could really use this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants