aws / aws-xray-sdk-node

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

MySQL capture failing for mysql2 pool queries with promises #452

Closed simoncollins closed 2 years ago

simoncollins commented 3 years ago

We're looking to switch our existing code that uses mysql2 to use X-Ray instrumentation. We have hit issues with using either the parent mysql2 module or mysql2/promise.

Our code is essentially one of these two scenarios (both fail):

With parent mysql2 module and .promise()

const rawMysql = require('mysql2');
const captureMySQL = require('aws-xray-sdk-mysql');

(async function() {
  const mysql = captureMySQL(rawMysql);

  const pool = mysql.createPool({
      // ... connection setting ...
  }).promise();

  await pool.query("SELECT ...");
})();

This fails with:

Error: You have tried to call .then(), .catch(), or invoked await on the result of query that is not a promise, which is a programming error

at mysql2/lib/commands/query.js:41:11 because the mysql2 Query class has a stubbed then method that throws this error.

In captureOperation in mysql_p.js the code tries to detect whether the command object is promise or event emitter based and this stubbed method confuses it into thinking it's actually a promise.

I'm not sure what can be done about this since there's no reliably better way to detect a promise.

With the mysql2/promise module

const rawMysql = require('mysql2/promise');
const captureMySQL = require('aws-xray-sdk-mysql');

(async function() {
  const mysql = captureMySQL(rawMysql);

  const pool = mysql.createPool({
      // ... connection setting ...
  });

  await pool.query("SELECT ...");
})();

This fails for a different reason in captureOperation where it tries to grab the connection config object using:

var config = this.config.connectionConfig || this.config

This is because the mysql2/promise module returns a PromisePool instance which delegates to the actual pool object holding the connection config object and there is no config property on the base object.

This does seem like something that could be addressed in the operation patching code by detecting and handling this scenario. Something like:

var config = this.pool ?
        this.pool.config.connectionConfig :
        (this.config.connectionConfig || this.config);

Has anyone else experienced this issue? I noticed that there is some support for mysql2 already so I'm wary that I'm doing something wrong having not used either library before. Alternatively it might be that it's our use of the pool object that is not covered by existing scenarios?

simoncollins commented 3 years ago

For the moment this workaround of patching the pool object works for me:

function patchPool(promisePool) {
    return {
        ...promisePool,
        config: promisePool.pool.config
    }
}

const pool = patchPool(mysql.createPool({
      // .. connection settings ...
}));
srprash commented 3 years ago

Hi @simoncollins Apologies for missing on this issue. It seems like the usage of pool with promise may not be fully supported in the SDK, although there is support for mysql2. Thanks for providing the repro code and details. Allow me some time to debug it locally to understand better and I'll provide an update soon.

simoncollins commented 3 years ago

No worries, thanks @srprash - for now the workaround mentioned above is working for us so all good.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs in next 7 days. Thank you for your contributions.

tpiqweb commented 2 years ago

Bump to make sure it doesn't go stale.

mickdekkers commented 2 years ago

Hi! I made a PR (#501) that should fix the first problem scenario you described (or at least prevent the error you got)