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 2021/03/04 14:13:54 UTC
[GitHub] [skywalking-nodejs] tom-pytel opened a new pull request #36: WIP: fixing async and preparing child entry/exit
tom-pytel opened a new pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36
The changes here are the result of preparing for allowing Exit/Exit and Entry/Entry parent child relationships during which I discovered and am in the process of fixing and improving some async functionality.
The source of the problem which necessitated #27 was discovered and fixed. The `http` and `axios` modules did not properly `async()` on returning a connection. Also, `async()` itself now duplicates the spans list for the current task in order that async tasks created before the `async()` don't have their copies of the list corrupted by subsequent operations in the current task. This all removes the necessity for a separate `invalid` flag, which flag itself was only a partial patch for the main problem which would still manifest in other ways.
The functionality of the `http` module is also being expanded so that errors on `'data'` events are properly attributed to the span.
Not for merge just yet.
----------------------------------------------------------------
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-nodejs] tom-pytel commented on pull request #36: Fixing async and preparing child entry/exit
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36#issuecomment-791413063
I restored this because apart from covering up some of the errors in async before this flag was doing one other important thing that didn't have to do with those errors.
----------------------------------------------------------------
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-nodejs] tom-pytel commented on pull request #36: Fixing async and preparing child entry/exit
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36#issuecomment-792756665
Ok, well this is in a good-ish place now, all my tests are running correctly. I will probably do more work on this but maybe this would be a good place to checkpoint and merge as this PR is getting a little big and already has several different feature changes?
----------------------------------------------------------------
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-nodejs] tom-pytel commented on pull request #36: WIP: fixing async and preparing child entry/exit
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36#issuecomment-790839419
Ok, this PR is ready. To reiterate, the point of this plugin inheritance scheme is to make sure things like:
```js
app.get('/mysqlexit', function (req, res, next) {
http.request(`http://somewhere.com/`, (r) => {
mysqlConnection.query('SELECT * FROM `table`', function (error, results, fields) {
if (error)
next(error);
else
res.end(JSON.stringify(results));
});
}).end();
});
```
work correctly. Previously the `http.request()` and `mysqlConnection.query()` exit spans would be merged into a single one, now the db exit span will be a child of the http exit span since it is created in user code which is actually processing the http request.
With something like Axios which stops its span before passing control back to the user:
```js
app.get('/mysqlexit', function (req, res, next) {
axios.get(`http://localhost:${portOut}/`).then((r) => {
mysqlConnection.query('SELECT * FROM `table`', function (error, results, fields) {
if (error)
next(error);
else
res.end(JSON.stringify(results));
});
});
});
```
The db exit span will be created after the http span finishes and so will be a sibling of the http span and a child of the `app.get()` entry span.
----------------------------------------------------------------
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-nodejs] tom-pytel commented on pull request #36: Fixing async and preparing child entry/exit
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36#issuecomment-791435656
Ok, NOW it looks good, any other errors that might pop up can be fixed in other PRs.
----------------------------------------------------------------
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-nodejs] tom-pytel commented on pull request #36: Fixing async and preparing child entry/exit
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36#issuecomment-792718212
> Tests under Node 10 failed, although I've rerun it, still failed, can you recheck?
Its a legitimate problem, not just config, something that's different between N10 and 12/14 is causing the spans to not stop in http, looking at it.
----------------------------------------------------------------
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-nodejs] kezhenxu94 commented on pull request #36: Fixing async and preparing child entry/exit
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36#issuecomment-792586364
Tests under Node 10 failed, although I've rerun it, still failed, can you recheck?
----------------------------------------------------------------
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-nodejs] tom-pytel commented on pull request #36: Fixing async and preparing child entry/exit
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36#issuecomment-791363054
Hold off on merging a bit, still working out some details.
----------------------------------------------------------------
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-nodejs] kezhenxu94 merged pull request #36: Fixing async and preparing child entry/exit
Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #36:
URL: https://github.com/apache/skywalking-nodejs/pull/36
----------------------------------------------------------------
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