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

Using with express async-await controller creates unlinked spans #29

Closed
nirsky opened this issue Apr 12, 2020 · 8 comments · Fixed by #34
Closed

Using with express async-await controller creates unlinked spans #29

nirsky opened this issue Apr 12, 2020 · 8 comments · Fixed by #34
Assignees
Labels
bug Something isn't working
Milestone

Comments

@nirsky
Copy link

nirsky commented Apr 12, 2020

Hey! First thing, thanks for taking the time and effort for creating this plugin.

I noticed using async-await on the query does not link the mongoose span with the express span unless the query is executed with exec().

Here are some example:

Using then() - Links correctly

app.get('/mongo-then', (_req, res) => {
  userCollection.findOne({}).then((u) => res.status(200).send(u));
});

Using async-await with exec - Links correctly

app.get('/mongo-exec', async (_req, res) => {
  const user = await userCollection.findOne({}).exec();
  res.status(200).send(user);
});

Using async-await - Created 2 separate spans

  • 1 span called get /mongo-async-await.
  • 1 span called mongoose.users.findOne.
app.get('/mongo-async-await', async (_req, res) => {
  const u = await userCollection.findOne({});
  res.status(200).send(u);
});

Zipkin Screenshot:

image


I'm using TypeScript, attaching my set up for reference:
Typescript version: 3.8.3
tsconfig:

{
    "compilerOptions": {
        "target": "ES2017",
        "lib": ["ES2019", "DOM"],
        "rootDir": "lib",
        "outDir": "dist",
        "module": "commonjs",
        "moduleResolution": "node",
        "strict": false,
        "declaration": true,
        "sourceMap": true,
        "inlineSources": true,
        "types": ["node"],
        "allowSyntheticDefaultImports": true,
        "esModuleInterop": true
    }
}
@wdalmut
Copy link
Owner

wdalmut commented Apr 12, 2020

Hi @nirsky, I think that is related to #16 i found the same problem in other projects open-telemetry/opentelemetry-js#940 and a the base pr is this one: open-telemetry/opentelemetry-js#926

Other interesting issues: open-telemetry/opentelemetry-js#752

I keep digging in this and i will report on this and #16

wdalmut added a commit that referenced this issue Apr 12, 2020
@wdalmut wdalmut self-assigned this Apr 12, 2020
@wdalmut wdalmut added the bug Something isn't working label Apr 12, 2020
@wdalmut wdalmut added this to the 0.1.0 milestone Apr 12, 2020
wdalmut added a commit that referenced this issue Apr 12, 2020
@wdalmut
Copy link
Owner

wdalmut commented Apr 12, 2020

I have added a test case to cover this issue in #30

@nirsky
Copy link
Author

nirsky commented Apr 12, 2020

Thanks for your quick reply!
I also encountered similar issues with mongodb-plugin.
Over there it didn't even report on some cases.

Is #30 containing a fix or only tests? (I guess only tests so far by looking at the code).

@wdalmut
Copy link
Owner

wdalmut commented Apr 12, 2020

sadly only test cases to cover your issue. i am following this PR open-telemetry/opentelemetry-js#926 that should resolve this issue. i suspect that every open telemetry implementation actually suffer this problem in JS... I have reported the same for mysql...

@vmarchaud
Copy link

Even if open-telemetry/opentelemetry-js#926 resolves some bugs, i'm pretty sure it will not fixes this one because mongoose returns a "thenables" which is isn't covered by async hooks due a bug in V8 (see nodejs/node#22360).
What you could do however is patches every mongoose op to use true promise instead of thenables but that's kinda hard due to how the chaining works in mongoose.

@wdalmut
Copy link
Owner

wdalmut commented Apr 12, 2020

@vmarchaud oh thank you for your feedback i digging more in the mongoose method list and try to prepare a big patch...

@wdalmut
Copy link
Owner

wdalmut commented Apr 13, 2020

yep, @vmarchaud is totally right the problem is strictly related to "thenables", in the test case:

Promise.all([
  User
   .find({id: "_test"})
   .skip(1)
  .limit(2)
  .sort({email: 'asc'}),
  User.countDocuments()
])

if i force a true promise with .exec() method

Promise.all([
  User
   .find({id: "_test"})
   .skip(1)
  .limit(2)
  .sort({email: 'asc'}).exec(),
  User.countDocuments().exec()
])

everything is correctly linked together. Same with your use case with .exec() and async...

This is not simple and i actually i don't understand how to change the mongoose flow in true promises and keeping the project workflow unchanged...

@wdalmut
Copy link
Owner

wdalmut commented May 23, 2020

This should be fixed by #34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment