[NodeJS] More broken things with release 0.1.0

[tom-pytel]

Please answer these questions before submitting your issue.

• Why do you submit this issue?
• Question or discussion
• Bug
• Requirement
• Feature or performance improvement

---

Bug

There seems to be more things broken in 0.1.0 compared to pre-release. I have some more detailed tests which were doing concurrent requests from various server types using various request methods which have stopped working correctly. Attached are two images of a snippet of a test before and after release and in this example you can see spans being cut off and component type not being set correctly, and possibly concurrency issues.

So far the changes to HttpPlugin seem to be a big part, but not all of the problem. The thing is that http sits underneath all the other plugins and must account for the fact that they may override it, but there is more to the errors than just the plugin I think, I am investigating.

Here is before (what it should be):

And here is after (broken, note entry span /doall marked as Http - http instead of Http - express and the exit spans cut off, as well as one of the standalone error exit spans also cut off):

• NOTE: If you want I can provide you the messy source of this test so you can check on your side as well.

[tom-pytel]

One other change I am not certain about, the change from plugin require() using the form PluginInstaller.require('axios').default to require('axios').default. The PluginInstaller.require() function is specifically set to run in the context of the top-level module. I did this because I had a test case (don't remember details exactly) where the skywalking plugin could not "see" a module the main app was using through its require() function and its dir search tree, which led to the module not being instrumented since skywalking could not require() it from where it was. So I added this function so that skywalking can require() in the same exact way the main module does and sees everything that it does.

[EDIT}: There is also the possibility when using skywalking via. npm link that it may have its own different version / instance of a module compared to the main executing application (which again would wind up not instrumenting the modules the main app is using). Basically anything that is going to be instrumented, and even some stuff that may not be (like probably on-finish), should be required from the point of view of the top-level module to make sure it is the same as the top-level module uses.

[tom-pytel]

I am preemptively sending you my test suite since I will probably be asleep by the time you see all of this. Its a simple npm install and ./run.sh to run the tests assuming your OAP server is local, otherwise set collectorAddress in client.js and srv_*.js. Also you need to explicitly npm install or npm link whatever skywalking you are using.

I suggest first checking commit e06e5c5 to see what the test should be before comparing to current broken master.
tests.tar.gz

P.S. Adding such a test may be beneficial to your current test suite.

P.P.S. If you have a slower machine you may need to increase the sleep time in run.sh to ensure the servers are fully started before the client is run.

[wu-sheng]

FYI @tom-pytel @kezhenxu94 As we have finished the vote, and release(from ASF perspective) is done, https://dist.apache.org/repos/dist/release/skywalking/node-js/0.1.0/ exists already. I am going to tag this as 0.2.0.

You two could make a decision whether we should market the 0.1.0 publicly from the quality perspective. But it is there :P

[tom-pytel]

FYI @tom-pytel @kezhenxu94 As we have finished the vote, and release(from ASF perspective) is done, https://dist.apache.org/repos/dist/release/skywalking/node-js/0.1.0/ exists already. I am going to tag this as 0.2.0.

You two could make a decision whether we should market the 0.1.0 publicly from the quality perspective. But it is there :P

Yes, sorry I didn't get to verifying this earlier, this release was timed perfectly with holiday season where I live 🥳 (and also Cyberpunk 2077 :)

I think this is ok to release publicly as a 0.1.0 version, single level calls seem to work fine, it is the more complicated situations which break, sometimes.

[wu-sheng]

e06e5c5 is truly set the component id as HTTP, and it exists still
https://github.com/apache/skywalking-nodejs/blob/c491d5cbdbea965a4ed640a93d9c232fafb99056/src/plugins/HttpPlugin.ts#L76-L80

The trace broken will need local debugging :)

[tom-pytel]

Also I think this is a good indicator that the regression tests could be made a little more robust (something I mentioned working on the Python agent). A cascaded test like I have included (more than just two endpoints with intermediates), testing multiple simultaneous threaded and multiple concurrent async requests, as well as fail-case tests (especially 404 and 500 for http cases). But that would be gold-standard and doesn't need to be implemented all at once or necessarily this early...

[wu-sheng]

I think the most challenge thing will be multiple simultaneous threaded case. The CI includes 3C8G, so, whether we could provide enough concurrency requests and how to detect illegal(broken) trace in the hundreds of request, some of them maybe correct.

[tom-pytel]

e06e5c5 is truly set the component id as HTTP, and it exists still
https://github.com/apache/skywalking-nodejs/blob/c491d5cbdbea965a4ed640a93d9c232fafb99056/src/plugins/HttpPlugin.ts#L76-L80

And in the same way testing for span.depth === 1 yes, except I think the difference may be that the higher level span has ended and so the depth will be 1 where it was not before since this was changed to onFinished() it is probably processed in a different order from before. In any case, you can see the different behavior in the images I sent up, the earler tags as express and the new one as Http.

[tom-pytel]

I think the most challenge thing will be multiple simultaneous threaded case. The CI includes 3C8G, so, whether we could provide enough concurrency requests and how to detect illegal(broken) trace in the hundreds of request, some of them maybe correct.

Even though my standalone tests are easier because I have full control, and I am not familiar with the testing framework you are using so can't give an opinion on that, but I can say that threaded is even problematic for me since order of connections is not guaranteed so checking is a bit more tricky.

[kezhenxu94]

@tom-pytel can you share the test codes with me, I’d like to check, reproduce and verify

[wu-sheng]

@kezhenxu94 I think this is, #6109 (comment)