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

Fixing async and preparing child entry/exit #36

Merged
merged 8 commits into from
Mar 8, 2021

Conversation

tom-pytel
Copy link
Contributor

@tom-pytel tom-pytel commented Mar 4, 2021

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.

Also, span.resync() no longer needed before span.stop(), a span can be errored or stopped while async.

Not for merge just yet.

@tom-pytel
Copy link
Contributor Author

Ok, this PR is ready. To reiterate, the point of this plugin inheritance scheme is to make sure things like:

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:

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.

@kezhenxu94 kezhenxu94 added the enhancement New feature or request label Mar 5, 2021
@kezhenxu94 kezhenxu94 added this to the 0.2.0 milestone Mar 5, 2021
@kezhenxu94 kezhenxu94 changed the title WIP: fixing async and preparing child entry/exit Fixing async and preparing child entry/exit Mar 5, 2021
@tom-pytel
Copy link
Contributor Author

Hold off on merging a bit, still working out some details.

@tom-pytel
Copy link
Contributor Author

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.

@tom-pytel
Copy link
Contributor Author

Ok, NOW it looks good, any other errors that might pop up can be fixed in other PRs.

@tom-pytel tom-pytel force-pushed the master branch 3 times, most recently from 835adb1 to 5b0cebe Compare March 5, 2021 18:05
@tom-pytel tom-pytel force-pushed the master branch 4 times, most recently from 8f9cebb to 0990d85 Compare March 7, 2021 21:30
@kezhenxu94
Copy link
Member

Tests under Node 10 failed, although I've rerun it, still failed, can you recheck?

@tom-pytel
Copy link
Contributor Author

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.

@tom-pytel
Copy link
Contributor Author

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?

@kezhenxu94 kezhenxu94 merged commit 2ffe7ca into apache:master Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants