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

fix: Updated mysql instrumentation to properly wrap the connection pool.getConnection and poolCluster.of #1647

Merged
merged 12 commits into from May 23, 2023

Conversation

bizob2828
Copy link
Member

@bizob2828 bizob2828 commented May 22, 2023

Description

Note: We need to add more information to the commit description on squash and merge to call out the fact we also instrument Poolnamespace.prototype.query now too.

This PR fixes the setup helper method in mysql by running before the agent instrumentation is loaded and busts the require cache. This was found to hide the root cause of this PR which was a mistake on a symbol defaulting to true. By fixing setup it also exposed an issue with PoolNamespace.prototype.query where it isn't getting instrumented thus some failing tests.

How to Test

Restore this bit flip, and the tests should fail

Related Issues

Closes #1645

@bizob2828 bizob2828 changed the title fix: Updated mysql instrumentation to properly wrap the connection in PoolConnection.prototype.query fix: Updated mysql instrumentation to properly wrap the connection pool.getConnection and poolCluster.of May 22, 2023
mrickard and others added 10 commits May 22, 2023 17:21
Signed-off-by: mrickard <maurice@mauricerickard.com>
…ypos

Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
…asic-pool.tap.js

Signed-off-by: mrickard <maurice@mauricerickard.com>
…re closely with mysql/basic-pool.tap.js

Signed-off-by: mrickard <maurice@mauricerickard.com>
…n uses a non-pooled connection

Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Merging #1647 (c75f383) into main (dd6ab9a) will increase coverage by 0.41%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1647      +/-   ##
==========================================
+ Coverage   96.23%   96.64%   +0.41%     
==========================================
  Files         199      200       +1     
  Lines       39028    39065      +37     
  Branches        9       24      +15     
==========================================
+ Hits        37557    37756     +199     
+ Misses       1470     1309     -161     
+ Partials        1        0       -1     
Flag Coverage Δ
esm-unit-tests-14.x 47.80% <ø> (ø)
esm-unit-tests-16.x 92.11% <ø> (?)
esm-unit-tests-18.x 92.11% <ø> (?)
integration-tests-14.x 78.97% <0.00%> (-0.01%) ⬇️
integration-tests-16.x 79.07% <0.00%> (?)
integration-tests-18.x 79.07% <0.00%> (?)
unit-tests-14.x 90.66% <0.00%> (-0.01%) ⬇️
unit-tests-16.x 90.72% <0.00%> (?)
unit-tests-18.x 90.70% <0.00%> (?)
versioned-tests-14.x 75.34% <100.00%> (+0.03%) ⬆️
versioned-tests-16.x 76.64% <100.00%> (+0.04%) ⬆️
versioned-tests-18.x 76.64% <100.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/instrumentation/mysql/mysql.js 95.34% <100.00%> (+13.63%) ⬆️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

test/versioned/mysql/basic-pool.tap.js Outdated Show resolved Hide resolved
test/versioned/mysql/basic-pool.tap.js Outdated Show resolved Hide resolved
test/versioned/mysql2/basic-pool.tap.js Show resolved Hide resolved
…pool.tap.js files

Signed-off-by: mrickard <maurice@mauricerickard.com>
Signed-off-by: mrickard <maurice@mauricerickard.com>
@mrickard mrickard requested a review from jmartin4563 May 23, 2023 14:17
@bizob2828 bizob2828 merged commit 4caf1db into main May 23, 2023
36 checks passed
Node.js Engineering Board automation moved this from Needs PR Review to Done: Issues recently completed May 23, 2023
@bizob2828 bizob2828 deleted the mysql-fix branch May 23, 2023 15:33
@github-actions github-actions bot mentioned this pull request May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Node.js Engineering Board
  
Done: Issues recently completed
Development

Successfully merging this pull request may close these issues.

Lack of instrumentation and accurate test for mysql poolCluster
3 participants