fix(core): capture additional command data with SDK v3

[SergeyPoluektov]

Issue #, if available: #460

Description of changes:

Additional data is added to a AWS segment via Capturer.capture method. The method uses a whitelist for allowed services and operations to be captured. The capturer is used by a AWS service segment to build a segment with additional request data. The first argument there – the response object from the AWS call, is used to add metadata to a segment with capturer.

The AWS SDK v3 patcher passes a response object to a AWS service segment without data and request.params attributes. These attributes are used by the capturer to obtain additional metadata from the service call.

The response.data attribute is used to capture response_parameters or response_descriptors such as ConsumedCapacity (see Capturer.capture).
And the response.request.params is used to capture request_parameters or request_descriptors such as TableName (see Capturer.capture)

This PR adds data and request.params properties to a first parameter passed to the AWS service segment:

• the client response output res.output is passed to a data property of the AWS service segment to capture response parameters and descriptors.
• the command input command.input is passed to a request.params property of the AWS service segment to capture request parameters and descriptors. Assuming that every Command in AWS SDK v3 has input property

I used this example app (lambda writing to DynamoDB) to test traces.
Trace before changes:

Trace after changes:

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

[codecov-commenter]

Codecov Report

Merging #527 (f18d6ab) into master (95602ca) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #527   +/-   ##
=======================================
  Coverage   82.34%   82.34%           
=======================================
  Files          36       36           
  Lines        1756     1756           
=======================================
  Hits         1446     1446           
  Misses        310      310           

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

[carolabadeer]

Look good so far! I think adding some unit tests to cover this new attribute would be helpful as well

[carolabadeer]

Is it possible to modify this to account for the possibility of the Command not having an input property?

[SergeyPoluektov]

AFAIK all the commands from AWS SDK v3 inherits from the abstract class Command from internal package smithy-client. Meaning there shouldn't be a case when the Command doesn't have an input prop.

But, ofc, we can be a bit more defensive – introduce a few conditions and fallback to a previous behavior.

[SergeyPoluektov]

Look good so far! I think adding some unit tests to cover this new attribute would be helpful as well

Thank you for review @carolabadeer I'll add tests for the attribute, sure.

[scorsi]

Unfortunately, I tried your fix @SergeyPoluektov and it seems that it don't works well. I'm requesting on the exact same table and both node are created from that. There are get (working), put (not working) and delete (idk) operations into that Lambda. 😭
Note: that operations rans inside a transaction.

The patch file I created

diff --git a/node_modules/aws-xray-sdk-core/dist/lib/patchers/aws3_p.js b/node_modules/aws-xray-sdk-core/dist/lib/patchers/aws3_p.js
index 74a7aa5..935d15e 100644
--- a/node_modules/aws-xray-sdk-core/dist/lib/patchers/aws3_p.js
+++ b/node_modules/aws-xray-sdk-core/dist/lib/patchers/aws3_p.js
@@ -13,15 +13,17 @@ const logger = require('../logger');
 const { safeParseInt } = require('../utils');
 const utils_1 = require("../utils");
 const XRAY_PLUGIN_NAME = 'XRaySDKInstrumentation';
-const buildAttributesFromMetadata = async (service, operation, region, res, error) => {
+const buildAttributesFromMetadata = async (service, operation, region, commandInput, res, error) => {
     var _a, _b, _c;
     const { extendedRequestId, requestId, httpStatusCode: statusCode, attempts } = ((_a = res === null || res === void 0 ? void 0 : res.output) === null || _a === void 0 ? void 0 : _a.$metadata) || (error === null || error === void 0 ? void 0 : error.$metadata);
     const aws = new aws_1.default({
         extendedRequestId,
         requestId,
         retryCount: attempts,
+        data: res?.output,
         request: {
             operation,
+            params: commandInput,
             httpRequest: {
                 region,
                 statusCode,
@@ -60,10 +62,12 @@ function addFlags(http, subsegment, err) {
 const getXRayMiddleware = (config, manualSegment) => (next, context) => async (args) => {
     const segment = contextUtils.isAutomaticMode() ? contextUtils.resolveSegment() : manualSegment;
     const { clientName, commandName } = context;
-    const operation = commandName.slice(0, -7); // Strip trailing "Command" string
+    const { input: commandInput } = args;
+    const commandOperation = commandName.slice(0, -7); // Strip trailing "Command" string
+    const operation = commandOperation.charAt(0).toLowerCase() + commandOperation.slice(1);
     const service = clientName.slice(0, -6); // Strip trailing "Client" string
     if (!segment) {
-        const output = service + '.' + operation.charAt(0).toLowerCase() + operation.slice(1);
+        const output = service + '.' + operation;
         if (!contextUtils.isAutomaticMode()) {
             logger.getLogger().info('Call ' + output + ' requires a segment object' +
                 ' passed to captureAWSv3Client for tracing in manual mode. Ignoring.');
@@ -88,7 +92,7 @@ const getXRayMiddleware = (config, manualSegment) => (next, context) => async (a
         if (!res) {
             throw new Error('Failed to get response from instrumented AWS Client.');
         }
-        const [aws, http] = await buildAttributesFromMetadata(service, operation, await config.region(), res, null);
+        const [aws, http] = await buildAttributesFromMetadata(service, operation, await config.region(), commandInput, res, null);
         subsegment.addAttribute('aws', aws);
         subsegment.addAttribute('http', http);
         addFlags(http, subsegment);
@@ -97,7 +101,7 @@ const getXRayMiddleware = (config, manualSegment) => (next, context) => async (a
     }
     catch (err) {
         if (err.$metadata) {
-            const [aws, http] = await buildAttributesFromMetadata(service, operation, await config.region(), null, err);
+            const [aws, http] = await buildAttributesFromMetadata(service, operation, await config.region(), commandInput, null, err);
             subsegment.addAttribute('aws', aws);
             subsegment.addAttribute('http', http);
             addFlags(http, subsegment, err);

EDIT: when looking deeper :

The proposed fixe works for all uniques operations but not for transactions.

When looking at the xray (not v3) logs, it seems Xray had never pass the table_name in case of transaction...

2022-09-12T19:24:59.661Z	fae23a19-f176-4446-8589-c9b32ff7ff20	DEBUG	UDP message sent: {"id":"fa2dc63ecf121c87","name":"dynamodb","start_time":1663010698.662,"namespace":"aws","aws":{"operation":"PutItem","region":"eu-west-1","request_id":"GEQVQ4E4IA9HKE4NG7KOV4LCQFVV4KQNSO5AEMVJF66Q9ASUAAJG","retries":0,"table_name":"Test1"},"http":{"response":{"status":200,"content_length":"2"}},"end_time":1663010699.523,"type":"subsegment","parent_id":"5433e28db52177b1","trace_id":"1-631f8789-5391dc222c46ca9e71a84e48"}
2022-09-12T19:24:59.961Z	fae23a19-f176-4446-8589-c9b32ff7ff20	DEBUG	UDP message sent: {"id":"54f917bc3c031e3b","name":"dynamodb","start_time":1663010699.662,"namespace":"aws","aws":{"operation":"TransactWriteItems","region":"eu-west-1","request_id":"094OOP0HQABC9698MKVEJI0KKFVV4KQNSO5AEMVJF66Q9ASUAAJG","retries":0},"http":{"response":{"status":200,"content_length":"2"}},"end_time":1663010699.936,"type":"subsegment","parent_id":"5433e28db52177b1","trace_id":"1-631f8789-5391dc222c46ca9e71a84e48"}

[willarmiros]

Hi @SergeyPoluektov - any chance you could follow up with the unit tests?

[andres-sinapsis]

Any news or progress about this PR? We really need to visualize the nodes by resource.

[burakince]

Hi @SergeyPoluektov and @carolabadeer ,

Any update on this PR? Do you need any help? We really like to have this feature.

Best,

[carolabadeer]

@SergeyPoluektov can you follow up with unit tests? If not, we are open to any contributions to this PR.

[bilalq]

This really feels like something AWS themselves should put resources behind instead of deferring to community volunteers. X-Ray is a paid service (an expensive one at that) and now that AWS SDK v2 is going into maintenance mode, this is essentially a service degradation.

[SchollSimon]

I totally agree this better be up and running fast. That is such a nogo first AWS encouraging developers to move to AWS SDKv3 then not delivering on the core features. We need this information for tracing distributed software systems it basically doesn't add any value at all at the moment.

[danno-s]

Hi, commenting here for visibility:
Since this PR looks abandoned and this change is important to maintain feature parity with the AWS SDK v2, I took the changes made here and added unit tests in #611 to validate that command inputs and outputs get mapped correctly into segments.

[carolabadeer]

closing as #611, which builds on the implementation in this PR, has been merged