aws / aws-xray-sdk-node

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

captureHTTPsGlobal now results in `TypeError: Cannot redefine property: request` #487

Open mrowles opened 2 years ago

mrowles commented 2 years ago

When packaging and running my AWS Lambda function using serverless + webpack with new versions of aws-sdk and aws-xray-sdk-core, I receive this error: TypeError: Cannot redefine property: request

import * as AWS from 'aws-sdk';
// import * as http from 'http';
import * as https from 'https';
import { ObjectType, PermissionRole, PrivacyRating } from './gql-types';
import axios from 'axios';
import { decrypt } from './utils/kms.service';

captureAWS(AWS);
// captureHTTPsGlobal(http, true) ; <-- having this uncommented causes the issue
// captureHTTPsGlobal(https, true); <-- having this uncommented causes the issue

AWS.config.update({
  httpOptions: {
    agent: new https.Agent({
      keepAlive: true,
      maxSockets: Infinity,
    }),
  },
});

Package versions:

"node": "14",
"aws-xray-sdk-core": "3.3.4",
"aws-sdk": "2.1074.0"
mrowles commented 2 years ago

I think reverting to "3.2.0" fixes this? I'm going to do more testing and confirm.

Edit: yes, can confirm, downgrading to 3.2.0 fixes this.

willarmiros commented 2 years ago

Hi @mrowles,

Thanks for raising this. Definitely seems like a problem because we added TypeScript support, which changed the way we bundle our final package in 3.3.x of this SDK. It's possible that this relates to Webpack, but probably more likely that this is similar to #450. Could you try the ts-ignore workarounds suggested in that post to see if that helps with the issue?

This way we can see if the same fix would apply.

jmalins commented 2 years ago

I am seeing the same issue with CDK 2.15.0 (which uses esbuild), when the bundling output format is set to OutputFormat.ESM. Reverting to CommonJS (OutputFormat.CJS) resolve it, at the expense of much larger bundle sizes.

Lambda code:

import { DynamoDBClient } from '@aws-sdk/client-dynamodb';
import { DynamoDBDocumentClient } from '@aws-sdk/lib-dynamodb';
import { captureAWSv3Client, captureHTTPsGlobal } from 'aws-xray-sdk';
import * as https from 'https';

const dynamoDbClient = new DynamoDBClient({ });
const dynamoDb = DynamoDBDocumentClient.from(dynamoDbClient);
captureAWSv3Client(dynamoDbClient);
captureHTTPsGlobal(https); // <-- fails here at runtime

CDK resource:

const apiHandlerFn = new njs.NodejsFunction(this, 'AccountDomainApiHandler', {
  timeout: Duration.seconds(10),
  runtime: lambda.Runtime.NODEJS_14_X,
  entry: path.join(__dirname, '../src/account-domain-api/index.ts'),
  bundling: {
    sourceMap: true,
    target: 'es2020',
    format: njs.OutputFormat.ESM, // <-- `njs.OutputFormat.CJS` works
    mainFields: [ 'module', 'main' ]
  },
  environment: {
    NODE_OPTIONS: '--enable-source-maps',
  },
  logRetention: logs.RetentionDays.TWO_WEEKS,
  tracing: lambda.Tracing.ACTIVE
});
willm commented 2 years ago

I'm also hitting this issue when bundling using esbuild, I'm not using cdk but my tscofnig.json is using commonjs already so the suggested fix above doesn't help

alexan commented 1 year ago

I also use commonjs bundles an run into this issue. How can I resolve this?

jepetko commented 1 year ago

I migrated to aws-sdk v3, thus I had to upgrade aws-xray-sdk-node because of AWSXRay.captureAWSv3Client(...). I had to hack around and finally ended up with a modified copy of http_p.js file (see gist https://gist.github.com/jepetko/cde1eb40493c6a19dd9ce29de39cbdd8) which I use instead of the original one.

I don't recommend to use it because is hacky & ugly. I also intent to remove it asap.

Is there a plan how to tackle this issue?

Thank you.

jepetko commented 1 year ago

it turned out that the redefinition of properties (https://github.com/aws/aws-xray-sdk-node/blob/master/packages/core/lib/patchers/http_p.js#L212) works on default export (while it doesn't when importing the entire module using import * as ... syntax).

MattCorwin commented 1 year ago

I'm seeing the same issue after moving to esbuild from webpack. Reverting to 3.2.0 for the time being.

benheymink commented 1 year ago

Also just hit this. I have not been able to get the ts-ignore workaround to work yet (it compiles/deploys, but no tracing, but that might be an issue our side).

wmegel commented 1 year ago

Any news about this issue ? having the same issue after moving to esbuild from webpack !

carolabadeer commented 1 year ago

Hi all, I created a backlog item for us to investigate this issue. If you are able to share any reproduction code with us and include details about your environments, that would be very helpful!

wmegel commented 1 year ago

it's difficult for me to share all the code of my client but maybe I can extract some part. Can you tell me how I can help you and where to share some parts of the code ?

jj22ee commented 11 months ago

This seems to be an issue related with migrating to ESM from CJS. More specifically, the import isn't compatible with the patching instrumentation that is currently done via require like AWSXRay.captureHTTPsGlobal(require('http'));

The following summary in the OTel JS issue describes the problem clearly: https://github.com/open-telemetry/opentelemetry-js/issues/1946#issuecomment-1295493673 The XRay SDKs currently tries to update the HTTP/HTTPS modules via captureHTTPsGlobal to enable tracing on all HTTP clients, but it seems this is not possible to do at runtime via import dependencies. I suppose rather than captureHTTPsGlobal, captureHTTPs could still work although this will enable tracing only on the returned and newly created http module.

There is no near-term plan for full ESM support in XRay SDK. However, in the linked OTel SDK discussion, an experimental feature was developed to support this ESM/import use case. OTel SDK is an alternative tracing instrumentation that can be used to send trace data to AWS XRay, and will need to be used alongside the ADOT Collector rather than the X-Ray Daemon.