elastic / apm-agent-nodejs

https://www.elastic.co/guide/en/apm/agent/nodejs/current/index.html
BSD 2-Clause "Simplified" License
582 stars 224 forks source link

Knex database spans not recorded when using async/await #873

Open thrcorrea opened 5 years ago

thrcorrea commented 5 years ago

Describe the bug When a define a route wich uses knex to connect do the database, and use async await at the function definition, the apm agent does not send the sql span, not showing the sql info at the request timeline at kibana. The apm agent does not support async/await

To Reproduce Steps to reproduce the behavior:

  1. Use this config '...'
  2. Then call '....'
  3. Then do '....'
  4. See error

Expected behavior To show the sql span at the request timeline.

Environment (please complete the following information)

How are you starting the agent? (please tick one of the boxes)

Additional context Add any other context about the problem here.

watson commented 5 years ago

Does the database span it self show up - just without the SQL query - or is the entire database span missing from the transaction?

Would you mind sharing a code snippet of how you're making the query?

thrcorrea commented 5 years ago

The entire database span is missing. I created a project with an example:

https://github.com/thrcorrea/apm-test

watson commented 5 years ago

Thanks for sharing the test app. I've been able to reproduce the problem now. I don't think it's directly async/await related, but you're right that this is what triggers it in this case. Other apps with async/await do work though. So I guess it's something with the combination of how knex operates. I'll investigate further...

watson commented 5 years ago

Update: It's somehow related to a combination of async/await and async hooks. A temporary workaround is to set asyncHooks: false in the agent config.

I'll continue to investigate to find a more permanent solution...

watson commented 5 years ago

This issue might be the same as discussed in #897

MartinKolarik commented 5 years ago

Same problem here, knex + async/await + asyncHooks (+ something else? not sure) = no active transaction for mysql spans. I'm on node 12. Turning off asyncHooks solves this issue but as I understand, it might break context propagation for some other cases?

jsumners commented 5 years ago

We are seeing this problem. Screenshot 1 here is with ELASTIC_APM_ASYNC_HOOKS=true:

image

Screenshot 2 is the same transaction path with ELASTIC_APM_ASYNC_HOOKS=false:

image

This is just a series of SQL statements being executed in order via await db.query. They are not executed within a database transaction.

Note: in the first screenshot there is a SET statement happening from the mysql2 module's onConnect hook to set the timezone for the session.

ping: @Qard

PaulMougel commented 4 years ago

FWI, we have the same issue when we use Objection.js (an SQL ORM): queries are not properly associated with the current transaction.

The trace logs show that no transaction is found, even though in our case we are processing a route response from hapi. If we manually create a span it is attached to the correct transaction, and knex requests are properly instrumented and associated with the transaction.

What is the recommended course of action?

tgriesser commented 4 years ago

I've been subscribed to this issue because I was curious if there was a solution (proper tracing in node is a difficult thing to do). So this is mostly just a complete guess here, but I wonder if this is because knex now uses a module called "tarn" as its pooling library:

https://github.com/vincit/tarn.js/

It looks like there's a instrumentation for generic-pool, which knex previously used:

https://github.com/elastic/apm-agent-nodejs/blob/master/lib/instrumentation/modules/generic-pool.js

Perhaps adding a similar shim for tarn would help associate the trace?

Qard commented 4 years ago

It's because the promise hooks feature in V8, which Node.js relies on for async_hooks, does not work with Thenables, which both Knex and Objection.js are using. This is unfixable without investing some time in fixing the issue in V8. See: https://github.com/nodejs/node/issues/22360

brandoncollins7 commented 4 years ago

Having this issue as well now, which led me to this thread. Are there any workarounds?

JCMais commented 4 years ago

The issue mentioned above by @Qard seems to be fixed on this PR https://github.com/nodejs/node/pull/33778 (by @Qard). Looks like the fix will be released with Node.js v14.6.0 https://github.com/nodejs/node/pull/34371

Qard commented 4 years ago

Yep, it's fixed in 14.6.x+. It hasn't been backported to 12.x yet, but shouldn't be too hard to backport.

JCMais commented 4 years ago

Can confirm that this issue is fixed when running the apm agent on Node.js >= 14.6