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

add plugin mysql #29

Closed
wants to merge 1 commit into from
Closed

Conversation

zijin-m
Copy link
Contributor

@zijin-m zijin-m commented Jan 28, 2021

In addition, use the resolve package to read package.json because mysql2 does not add package.json in the exports field of the package.json file

@@ -33,7 +34,7 @@ while (topModule.parent) {
export default class PluginInstaller {
private readonly pluginDir: string;
readonly require: (name: string) => any = topModule.require.bind(topModule);
private readonly resolve = (request: string) => (module.constructor as any)._resolveFilename(request, topModule);
private readonly resolve = (request: string) => resolve.sync(request);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question about this, the current form (module.constructor as any)._resolveFilename(request, topModule); ensures that the resolution is done from the point of view of the topmost module (the main app). This is necessary because if for example you have skwalking npm linked into a project instead of installed alongside all its dependancies then normal resolve will get the wrong path, one that sits in the skywalking node_modules path instead of the applicatiuon. Does the resolve package do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed this point. According to the documentation resolve, this can be done by
private readonly resolve = (request: string) => resolve.sync(request, {basedir: path.dirname(topModule.id) });
if You agree i will update later

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, well I checked your fix and it seems to work, so I am ok with this.

@tom-pytel
Copy link
Contributor

In addition, use the resolve package to read package.json because mysql2 does not export the file package.json

What do you mean by "does not export"? It HAS a package.json.

@zijin-m
Copy link
Contributor Author

zijin-m commented Feb 25, 2021

In addition, use the resolve package to read package.json because mysql2 does not export the file package.json

What do you mean by "does not export"? It HAS a package.json.

I'm sorry my description is not clear enough, I have updated it.
According to the node documentation modules

Bare specifiers like 'some-package' or 'some-package/shuffle'. They can refer to the main entry point of a package by the package name, or a specific feature module within a package prefixed by the package name as per the examples respectively. Including the file extension is only necessary for packages without an "exports" field.

while mysql2 defines the exports field in its package.json file, similar to issue

@tom-pytel
Copy link
Contributor

tom-pytel commented Feb 25, 2021

Some ideas for your plugin:

  • Wrap entire section after newExitSpan() with try / catch so that you can catch any exception from _query.apply() or even your own code and span.error() that and then span.stop() cleanly.
  • Put static init stuff like span.component = Component.MYSQL before the query call so that if there is an exception the span will have as much information as possible.
  • Does mysql2 do row streaming like mysql? If so it should be handled.
  • And a really silly detail, you don't need span.errored = true before span.error(err), that is set by the .error() function.

@kezhenxu94 asked me to review this because I also recently added a mysql plugin, just the first one, not mysql2, so you can look at that for some ideas of what I am talking about, its in master.

@tom-pytel
Copy link
Contributor

Actually, better look at #31, master had some errors.

@tom-pytel
Copy link
Contributor

Hey @zijin-m, going back to #27 where you said:

After testing, if two requests are repeatedly sent in a short time, the subsequent requests may use the AsyncState initialized by the previous request. It is temporarily unclear why store.enterWith(undefined as unknown as AsyncState) in the clear method not work(The store cannot be reset), I tried to add an invalid flag, which works normally, I hope to discuss the feasibility of this implementation with you, thx

I realize some time has passed, but is there any chance you can provide a short snippet case of this happening before the invalid flag fix? I am looking at some other things regarding async plugins living together peacefully.

@kezhenxu94
Copy link
Member

Closing in favor of #54

@kezhenxu94 kezhenxu94 closed this May 20, 2021
@kezhenxu94 kezhenxu94 added this to the 0.3.0 milestone May 20, 2021
@kezhenxu94 kezhenxu94 added the duplicate This issue or pull request already exists label May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants