Fetch #590

jasonterando · 2023-05-09T14:25:41Z

Issue #, if available: 585

Description of changes: Add fetch patch, including support for built-in fetch API for NodeJS current versions and node-fetch for older NodeJS versions.

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

…all to createSqlData to send those values from the arguments made to the sql call

carolabadeer · 2023-05-15T21:34:11Z

packages/core/package.json

@@ -1,6 +1,6 @@
 {
  "name": "aws-xray-sdk-core",
-  "version": "3.5.0",
+  "version": "3.4.1",


This corresponds to the current release version (3.5.0) and should never be changed to a prior version - this version number is only modified by the X-Ray team when preparing a new release. I imagine this is what is causing most of the CI failures.

To fix this, please keep this version as is - 3.5.0. We have an sdk_contrib directory that houses all community contributed instrumentations, so it would be great if this instrumentation could be moved there.

Also thought it would be worth mentioning since we do not usually maintain instrumentations in sdk_contrib, would you be willing to maintain it going forward and keep track of any issues that come in?

Q: Do you want to create a patchers directory in sdk_contrib for this, since all of the existing contributions are all middleware hooks?

carolabadeer · 2023-05-15T21:37:59Z

package.json

@@ -21,6 +21,7 @@
    "aws-xray-sdk-core": "3.5.0",
    "aws-xray-sdk-express": "3.5.0",
    "chai": "^4.2.0",
+    "chai-as-promised": "^7.1.1",


Following up to the comment on moving to sdk_contrib, the dependencies that are unique to this instrumentation would then be placed in the package.json of the new instrumentation package in sdk_contrib.
You can reference this fastify instrumentation PR as a guide to help

carolabadeer · 2023-05-15T21:38:38Z

packages/core/lib/patchers/fetch_p.js

+ * This module patches the global fetch instance for NodeJS 18+
+ */
+
+var contextUtils = require('../context_utils');


nit: use const instead of var

carolabadeer · 2023-05-15T21:42:59Z

packages/core/package.json

+    "mocha": "^10.2.0",
+    "semver": "^5.3.0",
+    "tsd": "^0.28.1",
+    "typescript": "^5.0.4"


mocha, tsd, and typescript are not needed here since they get pulled from the root package.json

jasonterando · 2023-05-16T03:22:09Z

Hi, my wife and I are on vacation for our 25th anniversary. I had to leave the computer at home lol. I will be able to knock these out at the end of the month, sorry for the delay.

…ou really need it to cleanly test async with chai; also fixed some lint findings

codecov-commenter · 2023-05-30T16:38:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.49%. Comparing base (4644c81) to head (216f75e).

❗ Current head 216f75e differs from pull request most recent head dd7f457. Consider uploading reports for the commit dd7f457 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #590   +/-   ##
=======================================
  Coverage   83.49%   83.49%           
=======================================
  Files          37       37           
  Lines        1805     1805           
=======================================
  Hits         1507     1507           
  Misses        298      298

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jasonterando · 2023-11-23T03:50:41Z

I'm going to try and resurrect this. Let me know if this seems feasible or if I should abandon. Thanks!

jj22ee · 2023-12-18T16:25:00Z

Hi, can you address @carolabadeer's comment:
#590 (comment)

jasonterando · 2023-12-18T21:04:19Z

Hi, can you address @carolabadeer's comment: #590 (comment)

Done and committed.

sdk_contrib/fetch/lib/fetch_p.js

jj22ee · 2023-12-19T01:28:19Z

Hi, can you address @carolabadeer's comment: #590 (comment)

Done and committed.

You may have addressed the comment below the comment I linked. chai-as-promised can be removed from the package.json/package-lock.json.

packages/core/tsconfig.debug.json

Was this file intended to be removed when you moved your contributions to sdk_contrib folder, which I see you've added the same tsconfig.debug.json there? If so, can you remove this file.

jasonterando · 2023-12-19T02:09:37Z

Hi, can you address @carolabadeer's comment: #590 (comment)

Done and committed.

You may have addressed the comment below the comment I linked. chai-as-promised can be removed from the package.json/package-lock.json.

packages/core/tsconfig.debug.json

Was this file intended to be removed when you moved your contributions to sdk_contrib folder, which I see you've added the same tsconfig.debug.json there? If so, can you remove this file.

Committed removal of chai-as-promised and tsconfig.debug.json

jj22ee · 2023-12-20T03:52:06Z

Can you look into the failing tests? There seems to be an issue with activeFetch.
Also, packages/core/tsconfig.debug.json wasn't actually removed in your latest commit.

All similar:

   1) Unit tests
       captured fetch
         short circuits if headers include trace ID:
     TypeError: activeFetch is not a function
      at Context.<anonymous> (test\unit\fetch_p.test.js:186:13)
      at processImmediate (node:internal/timers:466:21)
      at process.callbackTrampoline (node:internal/async_hooks:130:17)

  2) Unit tests
       captured fetch
         calls base function when no parent and automatic mode:
     TypeError: activeFetch is not a function
      at Context.<anonymous> (test\unit\fetch_p.test.js:195:13)
      at processImmediate (node:internal/timers:466:21)
      at process.callbackTrampoline (node:internal/async_hooks:130:17)

sdk_contrib/fetch/lib/fetch_p.js

sdk_contrib/fetch/package.json

jj22ee · 2024-01-23T19:03:55Z

Thanks, just a few more small changes in addition to the new comments.

A previous request was to remove chai-as-promised from package.json/package-lock.json. Looks like you instead removed it from sdk_contrib/fetch/package.json instead. Can you move this dependency back into sdk_contrib?

Also please remove the following files

packages/core/tsconfig.debug.json
.dockerignore
sdk_contrib/fetch/package-lock.json (usually we just keep the top level package-lock.json)
packages/core/package-lock.json

sdk_contrib/fetch/lib/fetch_p.js

sdk_contrib/fetch/test-d/index.test-d.ts

jasonterando · 2024-01-23T21:17:34Z

Ugh... I'll have to look at the tests failing after the updated npm test script later tonight

jasonterando · 2024-01-23T23:31:57Z

I believe this line is failing:
TypeError: Cannot stub non-existent property resolveManualSegmentParams
because the updated aws-xray.js which adds the export for resolveManualSegmentParams is probably not being used in the pipeline. Do we need to release that first before it's available for use by sdk_contrib, or is there some "trick" I can do in package.json to make this work?

jj22ee · 2024-01-24T21:06:22Z

Do we need to release that first before it's available for use by sdk_contrib, or is there some "trick" I can do in package.json to make this work?

Yeah, releasing "export for resolveManualSegmentParams" would be the more proper way of addressing this issue. The riskier and less ideal workaround would be to merge+release your PR, then hope that the Continuous Build Github Workflows will pass after the new release. We'll see if there's a better alternative.

jasonterando · 2024-01-24T22:44:01Z

I've gone ahead and put in a PR to add the resolveManualSegementParams export. If this is the way we need to go on this, I can wait until this is "live" before picking the fetch merge back up. On Wednesday, January 24, 2024 at 03:06:35 PM CST, Jonathan Lee ***@***.***> wrote: Do we need to release that first before it's available for use by sdk_contrib, or is there some "trick" I can do in package.json to make this work? Yeah, releasing "export for resolveManualSegmentParams" would be the more proper way of addressing this issue. The riskier and less ideal workaround would be to merge+release your PR, then hope that the Continuous Build Github Workflows will pass after the new release. We'll see if there's a better alternative. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

jj22ee · 2024-02-27T17:46:13Z

@jasonterando
New X-Ray SDK version is released, and I added a couple code changes to unblock your PR's unit tests
https://github.com/aws/aws-xray-sdk-node/blob/master/CHANGELOG.md#354

I looked into the segment generated by this PR, and I noticed that the Fetch Response information was missing in the segment.

"subsegments": [
    {
        "id": "22a15354bc8781eb",
        "name": "random-endpoint.com",
        "start_time": 1708993528.985,
        "end_time": 1708993529.44,
        "http": {
            "request": {
                "url": "",
                "method": "GET"
            },
            "response": {}
        },
        "namespace": "remote"
    }
]

I saw that this is because the subsegment.addRemoteRequestData() method was designed specifically for the built-in HTTP module Request/Response, and Fetch has a different Request/Response object. Therefore the data (request url/method and response status/content-length) must be obtained differently.

See how it is done for HTTP:

aws-xray-sdk-node/packages/core/lib/segments/attributes/subsegment.js

Lines 252 to 258 in 4644c81

    
           Subsegment.prototype.addRemoteRequestData = function addRemoteRequestData(req, res, downstreamXRayEnabled) { 
        
             this.http = new RemoteRequestData(req, res, downstreamXRayEnabled); 
        
             if ('traced' in this.http.request) { 
        
               this.traced = this.http.request.traced; 
        
               delete this.http.request.traced; 
        
             } 
        
           };

aws-xray-sdk-node/packages/core/lib/segments/attributes/remote_request_data.js

Lines 16 to 29 in 4644c81

    
           RemoteRequestData.prototype.init = function init(req, res, downstreamXRayEnabled) { 
        
             this.request = { 
        
               url: (req.agent && req.agent.protocol) ? (req.agent.protocol + '//' + (req.host || req.getHeader('host')) +  stripQueryStringFromPath(req.path)) : '', 
        
               method: req.method || '', 
        
             }; 
        
             if (downstreamXRayEnabled) { 
        
               this.request.traced = true; 
        
             } 
        
             if (res) { 
        
               this.response = getHttpResponseData(res); 
        
             } 
        
           };

Do you have bandwidth anytime to implement extracting Fetch request/response data into subsegment and corresponding unit tests? Otherwise I could take a stab at it later.

jasonterando · 2024-02-27T19:03:47Z

@jj22ee - yeah I can take a look at it, probably not until the weekend though, day job is hectic. Should I work from fetch-pr?

jj22ee · 2024-02-28T00:18:26Z

You can still work from this PR's fetch branch. As for adding your own custom addRemoteRequestData() method for Fetch, you can add it to fetch_p.js to modify the subsegment's http property

jasonterando · 2024-03-08T02:57:08Z

Hi, just committed updates that write to subsegment.http. One thing that wasn't clear was how traced is supposed to work. It looks like RemoteRequestData.init sets request.traced to true if downstreamXRayEnabled) is true, but then traced gets subsequently moved to the subsegment in addRemoteRequestData. In my code, I'm just setting subsegment.traced to true if downstreamXRayEnabled is true. If that's wrong, and I need to do the "two-step", let me know.

jj22ee · 2024-03-09T03:47:09Z

sdk_contrib/fetch/lib/subsegment_fetch.js

+Subsegment.prototype.addFetchRequestData = function addFetchRequestData(request, response, downstreamXRayEnabled) {
+  this.http = {
+    request: {
+      url: request.url.toString(),


Suggested change

url: request.url.toString(),

url: (request.url) ? request.url.toString() : '',

Ok, done. I also put in a non-set check for method. .d.ts file is added for Subsegment.addFetchRequestData

jj22ee · 2024-03-09T03:59:11Z

Thanks for adding subsegment_fetch.js! Added one suggestion above.
Also, can you add the corresponding subsegment_fetch.d.ts file. After that, everything looks good to merge.

…r method are not set store '' in http data

jj22ee

LGTM!

jj22ee · 2024-03-09T04:56:04Z

TYSM for the contribution @jasonterando!

am29d · 2024-03-20T09:50:51Z

Thanks a lot @jasonterando for the effort and @jj22ee for the review. Truly appreciate the merge ❤️

Jason Terando added 8 commits January 26, 2023 15:15

Add support for including sql query in sql subsegment for MySQL

2f3efc8

Update createSqlData to accept values and sql as arguments, and the c…

a8228e5

…all to createSqlData to send those values from the arguments made to the sql call

Add function comments to mysqL_p.createSqlData

08ad919

Merge remote-tracking branch 'upstream/master'

9c1758e

Merge remote-tracking branch 'upstream/master'

3f53f08

Working Node 14 and 18 tests for fetch

9855c14

Auto/manual fetch support

ef5108a

Add typescript defs for fetch, insure node-fetch tests run in NodeJS 14

02248f3

jasonterando requested a review from a team as a code owner May 9, 2023 14:25

carolabadeer suggested changes May 15, 2023

View reviewed changes

Jason Terando added 6 commits May 28, 2023 21:58

Merge remote-tracking branch 'upstream/master' into fetch

1b96361

Moved fetch patcher to sdk_contrib

8ecfd6f

Added integration testing for captureFetch

2b42c8f

Revert tests to Javascript

925ea85

Add chai-as-promised to base package.json dev dependencies, because y…

9de92ee

…ou really need it to cleanly test async with chai; also fixed some lint findings

Try fixing package*.json

5d0c457

RaphaelManke mentioned this pull request Jul 21, 2023

Feature Request: support for tracing NodeJs 18 native fetch calls aws-powertools/powertools-lambda-typescript#1619

Closed

Merge remote-tracking branch 'upstream/master' into fetch

4097c51

Change var to const

8a1eb78

jj22ee reviewed Dec 18, 2023

View reviewed changes

sdk_contrib/fetch/lib/fetch_p.js Outdated Show resolved Hide resolved

Removed captureFetch per @jj22ee

49951cb

Removed chai-as-promised and tsconfig.debug.json

675921f

remove docker files used for diag

002defc