feat: patch connections created by a pool's getConnection

[M-TGH]

Issue #, if available: #87

Description of changes:

Adds a simple patchObject call around connections returned from a pools getConnection.
Far as I could tell mysql doesn't take any other arguments for this, so it assumes the first argument is the callback.

I'm unsure what mysql2 makes of this, but I assume they promisified it which I didn't yet support. Would be nice to hear from someone who uses mysql2 what changes are still needed to support mysql2. I'll be looking into adding it but would like confirmation on what parameters it takes and what it returns.

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

[M-TGH]

Added what I think should be all that's needed for mysql2 support, still wouldn't mind if someone who's more familiar with mysql2 could take a look at it as well.

[darcoli]

Tried using this within our application which uses typeORM with the mysql driver, however I kept getting the following exception:

'RangeError: Maximum call stack size exceeded',
'    at Subsegment.init (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-core/lib/segments/attributes/subsegment.js:32:1)',
'    at new Subsegment (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-core/lib/segments/attributes/subsegment.js:25:1)',
'    at Segment.addNewSubsegment (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-core/lib/segments/segment.js:179:1)',
'    at PoolConnection.__query (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-mysql/lib/mysql_p.js:152:1)',
'    at PoolConnection.__query (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-mysql/lib/mysql_p.js:178:1)',
'    at PoolConnection.__query (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-mysql/lib/mysql_p.js:178:1)',
'    at PoolConnection.__query (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-mysql/lib/mysql_p.js:178:1)',
'    at PoolConnection.__query (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-mysql/lib/mysql_p.js:178:1)',
'    at PoolConnection.__query (/var/task/dist/webpack:/application/node_modules/aws-xray-sdk-mysql/lib/mysql_p.js:178:1)' ] } } }   

I did get it to work by adding further conditions to avoid the infinite recursion - changing the function patchObject() as follows:

function patchObject(connection) {
  if (connection.query instanceof Function) {
    if (!connection.__query) {
      connection.__query = connection.query;
      connection.query = captureOperation('query');
    }
  } 
    
  if (connection.execute instanceof Function) {
    if (!connection.__execute) {
      connection.__execute = connection.execute;
      connection.execute = captureOperation('execute');
    }
  }
      
  if (connection.getConnection instanceof Function) { 
    if (!(connection['__getConnection']) ) {
      patchGetConnection(connection);
    }
  }
  return connection;
}

[M-TGH]

Thanks @darcoli, I've added your feedback in e0fadd4.

[darcoli]

@M-TGH thanks for adding the changes - did not get any issues so far ;)

Any chance of getting this merged any time soon?

[chrisradek]

Would this also work with PoolCluster if instead of grabbing the callback from the 1st argument, you grabbed it from the last argument?

https://github.com/mysqljs/mysql#poolcluster

[M-TGH]

Looks like it would work, I'll add it into this PR, if you prefer me to split it up into a separate PR let me know (it may make the PR a bit big in size).

[M-TGH]

@chrisradek I've added basic support for mysql.createPoolCluster in dfc1c55 (getConnection & query). Let me know if you would prefer it in a separate PR.

I've also added support for poolCluster.of in db8e782, however I'm on the fence about patching of this way since it's rather ambiguous.

[chrisradek]

@M-TGH
I just had one comment on the code. And thank you for writing all of these tests!

I get your concerns about patching of in this way. Unfortunately, it doesn't seem that mysql exposes a clear way to determine if we're working with a Pool vs PoolCluster, so your implementation does look reasonable to me. Maybe a comment specifying that of exists on a PoolCluster and returns a Pool would help explain to future devs why it is being patched.

[M-TGH]

Good catch on the args, added a fix in 5bb11c8.

[chrisradek]

@M-TGH
Sorry, I missed one thing around promises earlier. If you can make that last change, I'll be happy to merge this in!

[M-TGH]

Updated the check in 1937797.
Whilst replacing this instanceof Promise check I noticed there's 2 more:

aws-xray-sdk-node/packages/mysql/lib/mysql_p.js

Line 39 in a8741c0

if (connection instanceof Promise) {

aws-xray-sdk-node/packages/mysql/lib/mysql_p.js

Line 57 in a8741c0

if (pool instanceof Promise) {

I'll replace those in a separate PR since they weren't added in this one, however let me know if you prefer it included in this PR.

[chrisradek]

@M-TGH
Awesome, thank you for making those changes, and for your contribution! Merging this in!