aws / aws-xray-sdk-node

The official AWS X-Ray SDK for Node.js.
Apache License 2.0
267 stars 156 forks source link

Update type declaration of captureAWSv3Client to fix TS errors #575

Closed carolabadeer closed 1 year ago

carolabadeer commented 1 year ago

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.

codecov-commenter commented 1 year ago

Codecov Report

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

:exclamation: 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.

jasonterando commented 1 year ago

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