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

Redesign AWS SDK V3 instrumentation to use middleware #416

Merged
merged 23 commits into from
May 11, 2021

Conversation

willarmiros
Copy link
Contributor

Issue #, if available:
#411

Description of changes:
This change redesigns the AWS SDK V3 client instrumentation to use the AWS SDK's Pluggable interface, which makes all the instrumentation middleware-based instead of a hybrid solution of middleware and monkey-patching. There was an original plan to vend solely a plugin, which customers could use like:

s3.middlewareStack.use(AWSXRay.getXRayPlugin());

However, I decided to stick with the original user experience of:

const s3 = new S3({});
AWSXRay.captureAWSv3Client(s3);

This is for 3 reasons:

  1. The former was very difficult to make work with manual mode, where a Segment must be manually passed in to the plugin by the user. I was constantly running into this issue when trying to test it with a sample app: Type 'DynamoDBClient' does not satisfy the constraint 'SmithyClient'. aws-sdk-js-v3#1803
  2. The latter experience is more idiomatic for X-Ray users, compared to how instrumentation was done for AWS SDK v2 and all other AWS SDKs across languages
  3. The latter is more extensible to OpenTelemetry, which requires an Otel-based API to be used for instrumentation as opposed to using a library's API (like middlewareStack)

I was still unable to get the latter API to work perfectly with TypeScript, because I would still get the error described in aws/aws-sdk-js-v3#1803. But a workaround was to just cast the client to any while instrumenting, which is not ideal, but worked once I did it.

const s3 = new S3({});
AWSXRay.captureAWSv3Client(s3 as any);

Also, tests will not pass because I've removed the transpiled JS from source control, I will add the necessary lifecycle hooks to get tests & publishing working later. Just wanted eyes on the code ASAP.

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

@willarmiros willarmiros changed the title Remove type Redesign AWS SDK V3 instrumentation to use middleware May 5, 2021
@willarmiros
Copy link
Contributor Author

@trivikr @AllanZhengYP please review

packages/core/lib/patchers/aws3_p.ts Outdated Show resolved Hide resolved
packages/core/lib/patchers/aws3_p.ts Outdated Show resolved Hide resolved
packages/core/lib/patchers/aws3_p.ts Outdated Show resolved Hide resolved
packages/core/lib/patchers/aws3_p.ts Outdated Show resolved Hide resolved
packages/core/lib/patchers/aws3_p.ts Outdated Show resolved Hide resolved
packages/core/lib/patchers/aws3_p.ts Outdated Show resolved Hide resolved
packages/core/lib/patchers/aws3_p.ts Outdated Show resolved Hide resolved
William Armiros added 2 commits May 5, 2021 15:20
region: string,
res: any,
): Promise<[ServiceSegment, HttpResponse]> => {
const { extendedRequestId, requestId, httpStatusCode: statusCode, attempts } = res.output?.$metadata || res.$metadata;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const { extendedRequestId, requestId, httpStatusCode: statusCode, attempts } = res.output?.$metadata || res.$metadata;
const { extendedRequestId, requestId, httpStatusCode: statusCode, attempts } = res.output?.$metadata;

The $metadata is always stored in output, so res.$metadata can be removed.

Example deserialize function of listTables command which populates $metadata https://github.com/aws/aws-sdk-js-v3/blob/e6080b303b0934ecec36c9284e0530d4129bb79e/clients/client-dynamodb/protocols/Aws_json1_0.ts#L3379-L3394

This parsed data from response is stored in the output in deseiralizerMiddleware https://github.com/aws/aws-sdk-js-v3/blob/e6080b303b0934ecec36c9284e0530d4129bb79e/packages/middleware-serde/src/deserializerMiddleware.ts#L20-L24

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually that parameter can be either a response object or an error object, and the error object has $metadata populated on it directly. However, I agree that this is a bit of an anti-pattern design so I'll make separate parameters for them.

packages/core/lib/patchers/aws3_p.ts Outdated Show resolved Hide resolved

const errObj = { message: err.message, name: err.name, stack: err.stack || new Error().stack };
subsegment.close(errObj, true);
throw err;
Copy link
Member

Choose a reason for hiding this comment

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

Is throwing an Object instead of Error expected in Xray SDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? We construct the errObj to record it in the X-Ray subsegment, so it conforms to our data model. But after recording it we just throw the original caught error.

Copy link
Member

Choose a reason for hiding this comment

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

My bad, I mistakenly read as errObj being thrown. There's no issue here.

willarmiros and others added 2 commits May 7, 2021 14:19
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
@willarmiros willarmiros marked this pull request as draft May 11, 2021 05:16
@willarmiros willarmiros marked this pull request as ready for review May 11, 2021 05:18
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (aws-v3-support@cf24754). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##             aws-v3-support     #416   +/-   ##
=================================================
  Coverage                  ?   54.33%           
=================================================
  Files                     ?       36           
  Lines                     ?     2650           
  Branches                  ?        0           
=================================================
  Hits                      ?     1440           
  Misses                    ?     1210           
  Partials                  ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf24754...d7bfcf3. Read the comment docs.

@willarmiros willarmiros merged commit 2a474f1 into aws:aws-v3-support May 11, 2021
willarmiros added a commit that referenced this pull request May 12, 2021
* Revert "Revert PR #386 (#412)"

This reverts commit 9e20d2e.

* Add proper TypeScript build logic (#417)

* build core files to dist directory

* fixed unit tests

* updated workflows and readme

* try to fix windows

* fix ls

* slashes

* no more ls

* zstd decompress

* see what bin has

* removed rsync

* fixes

* added sh

* try xargs instead

* cleanups

* fixed publishing logic

* Redesign AWS SDK V3 instrumentation to use middleware (#416)

* updated deps and type file

* removed type keyword added deps

* remove changes to js

* updated versions

* finished redesign of aws sdk v3 instrumentation

* refactored buildAttributes signature

* add compile back to workflow

* bumped tsd version instead

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

* removed dependencies and added docs

* add back most changes, update types

* removed aws-v3 branch from CI

* fixed codecov to use dist directory

* added blog post link

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
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.

None yet

3 participants