[GitHub] [skywalking-nodejs] tom-pytel commented on pull request #20: Fix bug that some requests of express / axios are not close correctly

[GitBox <gi...@apache.org>]

tom-pytel commented on pull request #20:
URL: https://github.com/apache/skywalking-nodejs/pull/20#issuecomment-752935083


   > @tom-pytel this seems to be a problem caused by the removal of `topModule.require`, I removed it because of the reason I stated in [#13 (comment)](https://github.com/apache/skywalking-nodejs/pull/13#issue-542917887) , so again I added it back in this PR with a little refactor for both (env var and `topModule.require`), please take a look
   
   Ok, well this is interesting, according to the image you attach the bug is not  exactly fixed - note the missing entry spans for the non-error `/` endpoint. However, trying this branch on my side works fine, so unless you attached an older image by accident then we have differing behavior (which is a bug in and of itself).
   
   More generally I am worried about HttpPlugin, why was it refactored? Specifically I am worried about the change to use `on-finished` from local `node_modules` which may change execution order with overriding plugins (express). But worse, I am worried the `on-finished` module used by skywalking may be different instance from what main app uses. If this is the case and `on-finished` has internal state which controls its behavior then this could give undefined behavior. And this is not a case where you can just require the `on-finished` module using `topModule.require()` because for just the http plugin it is not guaranteed the main app will have this installed (for express this is guaranteed because express uses it so it is safe to `topModule.require()` in that plugin).
   
   So for HttpPlugin, since I did actually spend a good bit of time making sure the order of execution was good with potential higher level modules and the actual higher level express, I would say that if there was no concrete reason for the refactor of HttpPlugin then that should be reverted, especially because of the potential problematic use of `on-finished`.


----------------------------------------------------------------
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