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(instr-mysql2): Fix mysql2 instrumentation connection prototype #2578

Conversation

david-luna
Copy link
Contributor

@david-luna david-luna commented Dec 4, 2024

Which problem is this PR solving?

mysql2@3.11.5 introduced a internal refactoring to solve circular dependencies within the package.
ref: sidorares/node-mysql2#3081

The changes include a new BaseConnection class which contains the methods we need to patch (query, execute). The Connection class is extending it therefore the instrumentation gets the wrong prototype when patching.

Fixes: #2572

Short description of the changes

  • bump mysql2 to v3.11.5
  • added a function to extract the proper prototype when patching/unpatching

@david-luna david-luna requested a review from a team as a code owner December 4, 2024 09:54
@github-actions github-actions bot added pkg:instrumentation-mysql2 pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found. labels Dec 4, 2024
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.75%. Comparing base (8bfa21f) to head (b5b72ec).

Files with missing lines Patch % Lines
...etry-instrumentation-mysql2/src/instrumentation.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2578      +/-   ##
==========================================
- Coverage   90.75%   90.75%   -0.01%     
==========================================
  Files         169      169              
  Lines        8018     8023       +5     
  Branches     1632     1633       +1     
==========================================
+ Hits         7277     7281       +4     
- Misses        741      742       +1     
Files with missing lines Coverage Δ
...etry-instrumentation-mysql2/src/instrumentation.ts 93.75% <85.71%> (-0.92%) ⬇️

@pichlermarc pichlermarc linked an issue Dec 4, 2024 that may be closed by this pull request
pichlermarc
pichlermarc previously approved these changes Dec 4, 2024
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I also just finished fixing this on my working copy before heading out to lunch, looks like you beat my by half an hour 🙂

Looks good, thanks 🙌 Let's wait for TAV to complete and then we should be good to go 🙂

@pichlermarc
Copy link
Member

Huh, weird - tests in TAV seem to be still failing but it's not yet possible to see which versions they're failing at. 🤔

@pichlermarc pichlermarc dismissed their stale review December 4, 2024 12:48

I think I missed something that keeps failing tests

@david-luna
Copy link
Contributor Author

ah I also just finished fixing this on my working copy before heading out to lunch, looks like you beat my by half an hour 🙂

Looks good, thanks 🙌 Let's wait for TAV to complete and then we should be good to go 🙂

Forgot to link the issue in the PR, sorry. TAV tests seem to be stuck so maybe my fix is not solving all issues :(

@pichlermarc
Copy link
Member

ah I also just finished fixing this on my working copy before heading out to lunch, looks like you beat my by half an hour 🙂
Looks good, thanks 🙌 Let's wait for TAV to complete and then we should be good to go 🙂

Forgot to link the issue in the PR, sorry. TAV tests seem to be stuck so maybe my fix is not solving all issues :(

No worries 🙂

I've also pushed my change (it's very similar to yours) - the only difference I can tell is that I'm checking that the base is BaseConnection. Let's see if that passes the TAV script 🤔

#2579


// if `baseClass` has no prototype means connection is not extending
// another class so the functions we need to patch are already there
if (typeof baseClass.prototype === 'undefined') {
Copy link
Member

@pichlermarc pichlermarc Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think in older versions you get EventEmitter as a base. That may be why the TAV tests are failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted the check to make it more explicit. It will return the base class prototype if it has the query function (execute goes together). Otherwise it falls back to the Connection prototype.

Note: I had trouble running it locally since docker image mysql:5.7 has no support for linux/arm64/v8 platform. Is it possible to update the image version in @opentelemetry/contrib-test-utils?

@david-luna
Copy link
Contributor Author

closing this one in favor of #2579

@david-luna david-luna closed this Dec 4, 2024
@david-luna david-luna deleted the fix-mysql2-instrumentation-connection-prototype branch December 11, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:instrumentation-mysql2 pkg-status:unmaintained This package is unmaintained. Only bugfixes may be acceped until a new owner has been found.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[instrumentation-mysql2] tests time out for mysql2@3.11.5
2 participants