Fetch

[jasonterando]

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.

[jasonterando]

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.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jasonterando]

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

[jj22ee]

Hi, can you address @carolabadeer's comment: https://github.com/aws/aws-xray-sdk-node/pull/590#discussion_r1194387982

[jasonterando]

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

Done and committed.

[jj22ee]

Hi, can you address @carolabadeer's comment: https://github.com/aws/aws-xray-sdk-node/pull/590#discussion_r1194387982

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]

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]

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)

[am29d]

Hey @jasonterando , appreciate the work you have put into this PR! Is there a chance I can help you move it forward, fixing tests or other work? It'd be great to push this PR to release. Let me know how I can assist.

Best Alex

[jasonterando]

Hey @jasonterando , appreciate the work you have put into this PR! Is there a chance I can help you move it forward, fixing tests or other work? It'd be great to push this PR to release. Let me know how I can assist.

Best Alex

Hi Alex. Yeah, I'm stuck on getting the test to run, trying to get lerna set up and running in Node 14

Hey @jasonterando , appreciate the work you have put into this PR! Is there a chance I can help you move it forward, fixing tests or other work? It'd be great to push this PR to release. Let me know how I can assist.

Best Alex

Hi Alex,

You're the first person that's shown any appreciation for any of this. I appreciate that. Yeah, I'm stuck on the testing part of it. Happy to work with somebody on getting this out, but it seems like it's one papercut after another, ex. the demand to "use const instead of var" even though var is used everywhere else in the code base. I had it all working at one point and then jj22ee insisted that I removed captureFetch, which broke all the testing in Node 14. I messed around with trying to get Node 14 running locally in a container, getting lerna going, etc. and ran into version errors, etc. doing an npm install. I can't even run mocha in the Docker Node 14 container, which is insisting I rename chai.js to .cjs.

Just kind of gave up...

[armenr]

Super appreciative of this work. We're eagerly waiting for tracing support of fetch...

[jj22ee]

Hey @jasonterando, I don't mind the reintroduction of the generic captureFetch function. In this case, can you update its previous comment about the condition in which the global instance will be patched rather than the module. e.g. "in Node18+ the built-in Fetch instance will be patched rather than the node-fetch module".

[jasonterando]

I was doing another look at the code and remembered that I had to do this to get it working:

const contextUtils = require('aws-xray-sdk-core/lib/context_utils');
const utils = require('aws-xray-sdk-core/lib/utils');
const logger = require('aws-xray-sdk-core/lib/logger');

I'm not sure if the links into aws-xray-sdk-core/lib are acceptable or not. I'm thinking not.

The reason I had to go that route is that I was replicating what the http_p.js hook does, which is this at line 99:

const parent = contextUtils.resolveSegment(contextUtils.resolveManualSegmentParams(options));

The method resolveSegment is availalbe via the aws-xray-sdk-core's exports, but resolveManualSegmentParams is not.

Seems like I have three choices, if I want to mirror what's being done in the http patcher:

1. Add resolveManualSegmentParams to aws-xray-sdk-core's exports in aws-xray.js
2. Move the fetch support from /sdk_contrib back to /packages/core/lib/patchers
3. Leave the links to /lib and leave the fetch support in sdk_contrib

Thoughts? (and thanks)

[jj22ee]

I agree, I don't think the links into aws-xray-sdk-core/lib will work out properly (I tried from manually testing).

Adding resolveManualSegmentParams to the exports list is the preferable solution, I don't see risk in exposing this method. This way you can get all the needed methods from contextUtils, utils, and logger straight from aws-xray-sdk-core. Thank you for continuing on this effort.

[jj22ee]

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

[jasonterando]

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

[jasonterando]

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]

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]

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]

@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:

• https://github.com/aws/aws-xray-sdk-node/blob/4644c81171bfce63671ac5201cafe5a66f34f7d0/packages/core/lib/segments/attributes/subsegment.js#L252-L258
• https://github.com/aws/aws-xray-sdk-node/blob/4644c81171bfce63671ac5201cafe5a66f34f7d0/packages/core/lib/segments/attributes/remote_request_data.js#L16-L29

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]

@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]

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]

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]

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.

[jj22ee]

TYSM for the contribution @jasonterando!

[am29d]

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