Port mysql plugin to support mysql2

[julienvincent]

This PR adds support for the mysql2 npm module (as per #582).

I am unsure on how to get the test suite up and running correctly, if someone could point me in the right direction I'd be happy to also write tests for the added plugin 👍

I will put comments on the relevant lines that changed from plugin-mysql.js

[kjin]

Thanks for the PR! I'm taking a look now... in the meantime, there's a script bin/docker-trace.sh that contains commands for launching docker containers for each of these databases. To make unit tests work you'll need to have all four services running. This should be the only pre-requisite to running unit tests with npm test.

As the APIs are similar it may be sufficient to edit test/plugins/test-trace-mysql.ts to make it so that every unit test is run twice, once for mysql and mysql2 each. The mysql library is currently being loaded from a fixture directory named at fixtures/mysql2, which might be confusing -- this refers to mysql@2. You should probably rename it so that there are co-existing fixtures mysql-2 and mysql2-1.

If you don't have time to do this that's fine, don't hesitate to let me know and I will update the tests.

[julienvincent]

Great! That sounds simple enough - I will finish up the tests tomorrow morning then 👍

[julienvincent]

I'm getting the following failed test when running locally:

  1) test-trace-connect
       should remove trace frames from stack:

      Uncaught AssertionError [ERR_ASSERTION]: 'Namespace.runAndReturn [as runAndReturn]' == 'middleware'
      + expected - actual

      -Namespace.runAndReturn [as runAndReturn]
      +middleware

      at ClientRequest.<anonymous> (/Users/julienvincent/code/cloud-trace-nodejs/test/plugins/test-trace-connect.ts:127:16)
      at HTTPParser.parserOnIncomingClient [as onIncoming] (_http_client.js:565:21)
      at HTTPParser.parserOnHeadersComplete (_http_common.js:116:23)
      at Socket.socketOnData (_http_client.js:454:20)
      at addChunk (_stream_readable.js:252:12)
      at readableAddChunk (_stream_readable.js:239:11)
      at Socket.Readable.push (_stream_readable.js:197:10)
      at TCP.onread [as _originalOnread] (net.js:589:20)
      at TCP.onread (/Users/julienvincent/code/cloud-trace-nodejs/node_modules/async-listener/glue.js:188:31)

As far as I can see this has nothing to do with the changes I made. Any ideas?

[kjin]

Oops - this test failure has to do with the GCLOUD_TRACE_NEW_CONTEXT environmental variable; this should be set to 1. Sorry -- that slipped my mind earlier.

[julienvincent]

I'm struggling with a strange issue now that I can't piece together. @kjin Perhaps you could help me understand whats happening here:

Defining a patcher for lib/pool.js of mysql2 breaks the constructor of Pool. This seems to happen whether or not I alter properties on its prototype - simply defining the patcher breaks it. The error I get in the tests is as follows:

  test-trace-mysql-2
    ✓ should perform basic operations
    ✓ should propagate context
    ✓ should remove trace frames from stack
    ✓ should work with events
    ✓ should work without events or callback (54ms)
    ✓ should perform basic transaction

  test-trace-mysql2-1
    1) "before all" hook
    2) "after all" hook


  6 passing (368ms)
  2 failing

  1) test-trace-mysql2-1
       "before all" hook:
     TypeError: Pool is not a constructor
      at Object.module.exports.createPool (test/plugins/fixtures/mysql2-1/node_modules/mysql2/index.js:17:10)
      at Context.<anonymous> (/Users/julienvincent/code/cloud-trace-nodejs/test/plugins/test-trace-mysql.ts:44:20)

  2) test-trace-mysql2-1
       "after all" hook:
     TypeError: Cannot read property 'end' of undefined
      at Context.<anonymous> (/Users/julienvincent/code/cloud-trace-nodejs/test/plugins/test-trace-mysql.ts:48:11)

and the exports of the plugin-mysql2.ts look as follows:

module.exports = [
  {
    file: 'lib/connection.js',
    versions: SUPPORTED_VERSIONS,
    patch: function(Connection, api) {
      shimmer.wrap(Connection, 'createQuery', createCreateQueryWrap(api));
    },
    unpatch: function(Connection) {
      shimmer.unwrap(Connection, 'createQuery');
    }
  },
  {
    file: 'lib/pool.js',
    versions: SUPPORTED_VERSIONS,
    patch: function(Pool, api) {
      // shimmer.wrap(Pool.prototype, 'getConnection',
      //              createWrapGetConnection(api));
    },
    unpatch: function(Pool) {
      // shimmer.unwrap(Pool.prototype, 'getConnection');
    }
  }
];

As you can see the patcher itself is not doing anything to Pool, this leads me to believe that something is happening within trace-plugin-loader.ts to cause this - though I can't see anything obvious that would cause this.

What I have done for now is change the pool patcher to target index.js which seems to work as expected, but I would still like to figure out whats happening here as it may reveal a bug in the plugin loader.

[julienvincent]

I don't think the tests that just failed on CircleCI were due to my changes - could you retry the build? I think circle was just slow and the timeout was exceeded.

[kjin]

@julienvincent I looked into the lib/pool.js patch issue you mentioned yesterday... I'm as baffled as you are. Investigating more today.

[julienvincent]

I can say that I was able to patch that file fine while using version 2.2.0 of this module within my personal project (before starting this PR) and using the above plugin as an external plugin. Perhaps a change has been made since then to cause this?

[kjin]

Yes, it seems like it.

I think I understand the issue now -- will open a bug imminently. In the meantime, if the patch to index.js is sufficient, let's use that.

[julienvincent]

Aha! Circular references - that makes sense now.

Yes the patch to index.js seems to work fine 👍

[kjin]

LGTM. Thanks! (I'll fix #618, and use your initial version patching lib/pool.js to see if it works)

[julienvincent]

Great! Thanks for your help, I will follow #618 for updates 🎉