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: circular dependencies #3081

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tpraxl
Copy link

@tpraxl tpraxl commented Sep 27, 2024

This commit addresses circular dependencies between index.js and promise.js, as well as within files in the lib directory such as pool.js and pool_connection.js.

It introduces a new common.js module to centralize shared functionalities, thus breaking the direct dependencies among these files.

This approach resolves issues with the @opentelemetry/instrumentation-mysql2 package, which was losing several exports due to the circular dependencies.

You can reproduce the problem with this simple test repo:

https://github.com/tpraxl/test-esm-mysql-otel

Follow the instructions in the README.md to see the respective warnings.

To verify that this PR fixes the issue, check it out, run npm i && npm pack and install the fixed version into the test repo using npm i $PATH_TO_MYSQL2/mysql2-3.11.3.tgz. Run npm start again in the checked out test repo to see the warnings vanish.

This PR would fix open-telemetry/opentelemetry-js-contrib#1511

pool.js and pool_connection.js required index.js, and
index.js required pool.js and pool_connection.js.

Circular dependency can cause all sorts of problems.
This fix aims to provide a step towards fixing a problem with
@opentelemetry/instrumentation-mysql2, which looses several
exports when using its patched require.

For instance, `format` is lost and can cause an instrumented
application to crash.
index.js and promise.js require each other.

Circular dependency can cause all sorts of problems.
This commit aims to fix a problem with
@opentelemetry/instrumentation-mysql2, which looses several
exports when using its patched require.

For instance, `format` is lost and can cause an instrumented
application to crash.
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 78.84232% with 106 lines in your changes missing coverage. Please review.

Project coverage is 88.20%. Comparing base (d94bfaa) to head (127df3c).
Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
lib/promise_connection.js 63.80% 80 Missing ⚠️
lib/promise_pool.js 84.82% 17 Missing ⚠️
lib/promise_prepared_statement_info.js 78.12% 7 Missing ⚠️
lib/pool_connection.js 50.00% 1 Missing ⚠️
promise.js 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3081      +/-   ##
==========================================
+ Coverage   88.13%   88.20%   +0.06%     
==========================================
  Files          71       80       +9     
  Lines       12890    12958      +68     
  Branches     1355     1364       +9     
==========================================
+ Hits        11361    11429      +68     
  Misses       1529     1529              
Flag Coverage Δ
compression-0 88.20% <78.84%> (+0.06%) ⬆️
compression-1 88.20% <78.84%> (+0.06%) ⬆️
tls-0 87.62% <78.84%> (+0.06%) ⬆️
tls-1 87.96% <78.84%> (+0.06%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Sep 27, 2024

Thanks, @tpraxl 🚀

I remember experiencing an error with rollup because of this.

Just a small recommendation, moving from ./common.js to ./lib/common.js 🙋🏻‍♂️

@tpraxl
Copy link
Author

tpraxl commented Sep 28, 2024

@wellwelwel You have a point there. In that case however, it feels weird to arbitrarily stuff some exported functions together in lib/common.js .

It made sense when we were talking about commons for index.js and promise.js, but in lib, I would split common.js up into the following files and get rid of common.js

  • lib/create_connection.js
  • lib/create_pool.js
  • lib/create_pool_cluster.js

I'm working on it and will push the result

The proposal to put common.js into lib was good, but it felt
weird to arbitrarily stuff just some exported functions in
lib/common.js. This made sense when common.js was meant
to provide commons for index.js and promise.js. But
in the lib, this felt like a weird scope.

I therefore split common.js into

- lib/create_connection.js
- lib/create_pool.js
- lib/create_pool_cluster.js

Also made `require` more consistent in all affected files: all
`require` files now have a js suffix when they refer to a
single local file.
@wellwelwel
Copy link
Collaborator

wellwelwel commented Oct 2, 2024

Hey @tpraxl, I tested your changes with rollup:

Screenshot 2024-10-02 at 17 31 02
  • index.js
import mysql from 'mysql2/promise';

mysql.createPool({});
  • rollup.config.js
import { defineConfig } from 'rollup';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import json from '@rollup/plugin-json';

export default defineConfig({
  input: 'index.js',
  output: {
    file: 'dist.js',
  },
  plugins: [nodeResolve(), commonjs(), json()],
});
  • package.json
{
  "type": "module",
  "devDependencies": {
    "@rollup/plugin-commonjs": "^28.0.0",
    "@rollup/plugin-json": "^6.1.0",
    "@rollup/plugin-node-resolve": "^15.3.0",
    "rollup": "^4.24.0"
  },
  "dependencies": {
    "mysql2": "file:../node-mysql2"
  }
}

I'll try to investigate this further.

promise.js was required by lib sources, and promise.js required
the same lib sources eventually. Extracted the respective
classes to lib. Also extracted functions that are
shared between several of the new files.

Decided to put each exported class / function into its own file.
This may bloat the lib folder a bit, but it provides clarity
(where to find what).
@tpraxl
Copy link
Author

tpraxl commented Oct 3, 2024

Thanks for your finding. No need to investigate further IMHO, I would propose to extract PromiseConnection, PromisePool and PromisePoolConnection into lib. Each in its own file and to resolve the circular dependency that way.

See https://github.com/tpraxl/node-mysql2/pull/1/files

If these changes are OK for you, I'll merge them into fix/circular-dependencies and update this PR.

@tpraxl
Copy link
Author

tpraxl commented Oct 11, 2024

@wellwelwel are you fine with my changes?
See https://github.com/tpraxl/node-mysql2/pull/1/files
I would merge them into this PR if you are ok with it.

@mknj
Copy link

mknj commented Oct 14, 2024

I am facing the same opentelemetry issue and i verified that this MR fixes it.

Please merge this MR.

@wellwelwel
Copy link
Collaborator

@wellwelwel are you fine with my changes?

@tpraxl, sorry for the delay (I've been away for a few days).

Testing in the new branch, I'm facing this now:

Previously there were three warnings 💡

Screenshot 2024-10-15 at 19 45 02

@wellwelwel
Copy link
Collaborator

Please merge this MR.

@mknj, these refactors involve some critical files and modules, so it's ideal to review them carefully 🙋🏻‍♂️

Please, feel free to contribute to the review 🤝

@tpraxl
Copy link
Author

tpraxl commented Oct 17, 2024

I'll investigate this further, just haven't had the time yet. Before, I only fixed the obvious circular dependencies until it was working for the instrumentation. Now, I'll use madge --circular ..

I fear, at a quick first glance, that this fix won't be possible without breaking changes. But give me some time to investigate.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Oct 17, 2024

I'll investigate this further, just haven't had the time yet.

Thanks, @tpraxl 🤝

I'll try to play in your last changes too 🙋🏻‍♂️

@tpraxl
Copy link
Author

tpraxl commented Oct 17, 2024

FYI: This seem to be the critical relations:

image

@wellwelwel
Copy link
Collaborator

wellwelwel commented Oct 17, 2024

@tpraxl, I didn't expect it to be so complex. As the warning from Rollup doesn't crashes or block the compilation, I'm fine with the current stage of this PR.

Optionally, we can use a final commit as fix: improve circular dependencies 💭

Then, when #2803 moves on or a major release milestone comes out, we can discuss it with more flexibility.
What do you think? If you agree, LGTM 🙋🏻‍♂️

The extraction of classes was performed without extracting
the patch of methods like `escape`, etc.
The patching is now performed in the files that define the
respective classes, guaranteeing patched versions when
these files are required.
@tpraxl
Copy link
Author

tpraxl commented Oct 19, 2024

I am struggling, because madge shows that in this branch, I actually increased the amount of circular dependencies when I untangled index.js / promise.js (because there are more files now).

The only version having less circular dependencies is https://github.com/tpraxl/node-mysql2/tree/fix/circular-dependencies-promise
I managed to fix that version (it caused one test to fail, because I missed to patch methods when I extracted classes). This version only has one circular dependency, as far as madge reports.

However, more problems are popping up when you go down the rabbit hole.

See https://github.com/tpraxl/test-esm-mysql-otel/blob/main/README.md for a thorough description and a way to reproduce the problems.

Basically, with the new version, @opentelemetry will not activate instrumentation for mysql2 anymore, because it does not encounter the main module when used with esm. This would be only possible with a workaround, that I also provided in https://github.com/tpraxl/node-mysql2/tree/fix/open-telemetry-not-activated
But I believe this is actually an @opentelemetry bug and should not be fixed in mysql2.

So, if we merge this PR now, instrumentation for esm will not crash anymore, it will simply be gone for esm users. Sigh.

I hope, I'll find some time to investigate the new issues and maybe I can find a way to trigger a fix.

@tpraxl
Copy link
Author

tpraxl commented Oct 20, 2024

My proposal after having thought about it again:

Let's merge https://github.com/tpraxl/node-mysql2/tree/fix/circular-dependencies-promise into this PR.
Have it reviewed thoroughly before the merge. (Next week, I am planning to test this version with a real-life project.)

Live with the fact that instrumentation is still not working for the callback variant in esm scenarios. At least, it does not crash anymore when instrumented.

Next steps:

  • I'll dive deeper into the @opentelemetry instrumentation problem (indirection needed for main module in esm) and try to propose a fix
  • After the conversion to typescript you mentioned, we'll tackle the last circular dependency left

What do you think?

@wellwelwel
Copy link
Collaborator

Let's merge https://github.com/tpraxl/node-mysql2/tree/fix/circular-dependencies-promise into this PR.

I didn't get to review tpraxl#1 (I just did a quick compilation test).

What do you think?

@tpraxl, feel free to proceed 🙋🏻‍♂️

@tpraxl
Copy link
Author

tpraxl commented Oct 22, 2024

Merged fix/circular-dependencies-promise into this branch.

Summary:
This PR fixes the crash for esm apps using the callback variant of mysql2 with open telemetry mysql2 instrumentation.
Open telemetry mysql2 instrumentation is however not yet activated for the callback variant, because it does not detect the main module 'mysql2' yet. This is most likely a bug in open telemetry's require patch and needs to be investigated and fixed there. My own tests of the callback variant with a real-life application revealed no problems with this version of mysql2. A thorough review is advised anyway.

@wellwelwel
Copy link
Collaborator

@tpraxl, I'll ask for a few days to review it carefully. Thanks in advance 🤝

Copy link

@mknj mknj left a comment

Choose a reason for hiding this comment

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

i feel like a linter

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
lib/inherit_events.js Outdated Show resolved Hide resolved
lib/make_done_cb.js Outdated Show resolved Hide resolved
lib/promise_pool.js Outdated Show resolved Hide resolved
lib/promise_pool_connection.js Outdated Show resolved Hide resolved
@tpraxl
Copy link
Author

tpraxl commented Oct 24, 2024

@mknj thanks for your findings. You are even better than npm run lint .
That should have been my job. Sorry.

@tpraxl
Copy link
Author

tpraxl commented Oct 25, 2024

Good news:
Actually reading the complete documentation of opentelemetry helps. 👀
I found out that, in order to have proper support for instrumentation in esm, you need to use the node argument --experimental-loader=@opentelemetry/instrumentation/hook.mjs. This seems to be overlooked oftentimes.

I adjusted the test repo and added the fixed start commands:
https://github.com/tpraxl/test-esm-mysql-otel

Also adjusted the README to document this issue.

So, with the fixed startup, we actually gain instrumentation for the callback mode as well and with this PR merged, the format function will be available as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"format is not a function" error when using mysql2 instrumentation with kysely
3 participants