You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/12/30 18:38:15 UTC
[GitHub] [skywalking] tom-pytel opened a new issue #6109: [NodeJS] More broken things with release 0.1.0
tom-pytel opened a new issue #6109:
URL: https://github.com/apache/skywalking/issues/6109
Please answer these questions before submitting your issue.
- Why do you submit this issue?
- [ ] Question or discussion
- [x] 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):
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752800093
`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 :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel edited a comment on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752798487
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel edited a comment on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752722626
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel edited a comment on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752722626
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 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752747987
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](https://github.com/apache/skywalking/files/5755608/tests.tar.gz)
P.S. Adding such a test may be beneficial to your current test suite.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752801761
> 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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752795063
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752811005
@tom-pytel can you share the test codes with me, I’d like to check, reproduce and verify
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752798487
> 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 🥳
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel edited a comment on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752747987
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](https://github.com/apache/skywalking/files/5755608/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752811141
@kezhenxu94 I think this is, https://github.com/apache/skywalking/issues/6109#issuecomment-752747987
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752800817
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel edited a comment on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752722626
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752722626
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 closed issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
kezhenxu94 closed issue #6109:
URL: https://github.com/apache/skywalking/issues/6109
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752801256
> `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`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel edited a comment on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752722626
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.
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] tom-pytel commented on issue #6109: [NodeJS] More broken things with release 0.1.0
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on issue #6109:
URL: https://github.com/apache/skywalking/issues/6109#issuecomment-752800184
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...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org