Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make mysql2 3.x.x function #3301

Merged
merged 7 commits into from May 1, 2023
Merged

Make mysql2 3.x.x function #3301

merged 7 commits into from May 1, 2023

Conversation

firecow
Copy link
Contributor

@firecow firecow commented Apr 30, 2023

From what I can tell, mysql 3.x.x doesn't contain any changes to createConnection, query or execute.

Fixes: #3151

Checklist

  • Implement code
  • Update documentation
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

@cla-checker-service
Copy link

cla-checker-service bot commented Apr 30, 2023

💚 CLA has been signed

@firecow firecow marked this pull request as draft April 30, 2023 18:54
@github-actions github-actions bot added agent-nodejs Make available for APM Agents project planning. community triage labels Apr 30, 2023
@firecow firecow marked this pull request as ready for review April 30, 2023 19:19
@trentm trentm removed the triage label May 1, 2023
@trentm trentm self-assigned this May 1, 2023
@trentm
Copy link
Member

trentm commented May 1, 2023

@firecow Thanks for the PR! I'm looking now.

I hope you are okay with me pushing some tweaks to your branch. For example, it looks like your PR changed the package-lock.json to Windows line endings which artificially makes it look like a 27k line diff. :)

@trentm
Copy link
Member

trentm commented May 1, 2023

@firecow Are you able to sign our CLA? (See #3301 (comment).)

@trentm
Copy link
Member

trentm commented May 1, 2023

Finding what versions of node mysql2 supports is a little interesting. It has:

  "engines": {
    "node": ">= 8.0"
  },

however:

  • The changelog for version 2.3.1 says "build: update to node 12/14/16, migrate from travis-ci and appveyor to GH actions, add perf benchmarking workflow"
  • The latest mysql2@2 is using a lru-cache dep with a min supported node version of v10.
  • The latest mysql2@3 is using lru-cache@8 which has a min support node version of v16.14.

@trentm
Copy link
Member

trentm commented May 1, 2023

  • The latest mysql2@3 is using lru-cache@8 which has a min support node version of v16.14.

Attempting with node 8.6 crashes in lru-cache.

% node test/instrumentation/modules/mysql2/pool-release-1.test.js
/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index.js:51
    heap;
        ^

SyntaxError: Unexpected token ;
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:588:28)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Module.require (module.js:568:17)
    at Module.Hook._require.Module.require (/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/require-in-the-middle/index.js:179:39)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index-cjs.js:5:36)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Module.require (module.js:568:17)
    at Module.Hook._require.Module.require (/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/require-in-the-middle/index.js:179:39)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/mysql2/lib/connection.js:25:13)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Module.require (module.js:568:17)
    at Module.Hook._require.Module.require (/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/require-in-the-middle/index.js:179:39)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/mysql2/index.js:5:20)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Module.require (module.js:568:17)
    at Module.Hook._require.Module.require (/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/require-in-the-middle/index.js:179:39)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/trentm/el/apm-agent-nodejs-firecow/test/instrumentation/modules/mysql2/pool-release-1.test.js:22:13)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)
    at tryModuleLoad (module.js:508:12)
    at Function.Module._load (module.js:500:3)
    at Function.Module.runMain (module.js:665:10)
    at startup (bootstrap_node.js:187:16)
    at bootstrap_node.js:607:3

Similarly with node v12:

% node12 test/instrumentation/modules/mysql2/pool-release-1.test.js
/Users/trentm/el/apm-agent-nodejs-firecow/node_modules/mysql2/node_modules/lru-cache/dist/cjs/index.js:359
    #initializeTTLTracking() {
                          ^

SyntaxError: Unexpected token '('
    at wrapSafe (internal/modules/cjs/loader.js:915:16)
...

It seems to work with node 14.6.0 (crashes with node 14.5.0).

The lru-cache min dep on node v16.14 was added for specifics of its abort handling (isaacs/node-lru-cache@b3b6d24). From a quick grep it doesn't look like node-mysql2 is using any "Abort"-related support in its usage of lru-cache. So at a guess, node 14 is the effective min supported node version.

@trentm
Copy link
Member

trentm commented May 1, 2023

@elasticmachine, run elasticsearch-ci/docs

@trentm trentm merged commit e90e25b into elastic:main May 1, 2023
33 checks passed
@trentm
Copy link
Member

trentm commented May 1, 2023

Thanks, @firecow!

@firecow
Copy link
Contributor Author

firecow commented May 2, 2023

Thanks, @firecow!

You are welcome, and thank you for maintaining this project 🤟

v1v added a commit to v1v/apm-agent-nodejs that referenced this pull request May 8, 2023
…re/support-specific-modules

* 'main' of github.com:elastic/apm-agent-nodejs: (54 commits)
  chore: fix dev-utils/ci-tav-slow-jobs.sh (elastic#3319)
  test: reduce TAV test matrix for slowest jobs (elastic#3321)
  chore: sync package-lock so 'npm ci' can work (elastic#3318)
  docs: document `useElasticTraceparentHeader` config var (elastic#3316)
  chore, test: test driver improvements (elastic#3293)
  test: drop node 14 from RC tests now that it is EOL (elastic#3315)
  test: fix running fastify.test.js with node v8 (elastic#3317)
  feat: add @apollo/server@4 support (elastic#3203)
  chore: update nvm (elastic#3309)
  tests: stop testing 'express-graphql' instrumentation (elastic#3304)
  chore: fix bitrot.js dev util for recent changes (elastic#3308)
  test: restore testing of Azure Functions on node >=18.x (elastic#3307)
  fix: support Lambda instrumentation for `contextManager: 'patch'`; refactor Lambda tests (elastic#3305)
  test: fix fastify TAV test failures (elastic#3314)
  test: fix @aws-sdk/client-s3 TAV test failures (elastic#3312)
  feat: add instrumentation for aws-sdk S3 client (elastic#3287)
  feat(fastify): add captureBody support (elastic#2681)
  feat: mysql2@3 support (elastic#3301)
  chore(deps): bump @opentelemetry/exporter-prometheus from 0.37.0 to 0.38.0 in /test/opentelemetry-metrics/fixtures (elastic#3295)
  chore(deps-dev): bump fastify from 4.16.3 to 4.17.0 (elastic#3296)
  ...
trentm added a commit that referenced this pull request May 16, 2023
mysql2@3 doesn't appear to contain any changes to createConnection, query,
or execute -- the exports that are instrumented.

Fixes: #3151
Co-authored-by: Trent Mick <trent.mick@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning. community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mysql2@3 support
2 participants