Skip to content

Add support for knex v0.18.0+ #1190

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

Closed
watson opened this issue Jul 3, 2019 · 12 comments
Closed

Add support for knex v0.18.0+ #1190

watson opened this issue Jul 3, 2019 · 12 comments
Labels
agent-nodejs Make available for APM Agents project planning. enhancement

Comments

@watson
Copy link
Contributor

watson commented Jul 3, 2019

No description provided.

@christhegrand
Copy link

christhegrand commented Jan 3, 2020

Hi,
I noticed recently that when we upgraded our app to Knex 0.19.5 from Knex 0.16.3, we stopped getting information about the SQL queries that we are running in APM. For example, previously I would be able to see, when looking at a given transaction, separate spans showing "begin", "select * from user where user_id=123", "commit", etc., whereas now I am only seeing the "begin" and "commit" spans and the actual queries are missing. Is this caused by the lack of support for Knex 0.18.0+ mentioned in this issue?

Thanks,
Chris

@lreuven
Copy link

lreuven commented Mar 28, 2020

Hi Sundar,
thanks for the request, we've added it to our backlog and will give it a try in the near future.
fill free ton help & contribute.
Lior

@JCMais
Copy link
Contributor

JCMais commented May 7, 2020

I'm having the same issue than @christhegrand, is there anything we can do to make this issue move faster?

Looks like #1497 has not solved this issue, as I'm using the latest version of the agent.

I'm available to work on this if necessary. I've started by updating the pg instrumentation to support pg >= 8: #1735


Looks like this here:

var span = agent.startSpan('SQL', 'db', 'postgresql', 'query')
var id = span && span.transaction.id

Cannot create a new span because it cannot find an existing transaction (messages are on reverse order, check timestamp):
image
image

@JCMais
Copy link
Contributor

JCMais commented Jul 28, 2020

The above issue was caused due to this #873

It's fixed with the latest version of Node.js

I suppose this issue can be closed as v0.18.0+ is already supported

@trentm trentm self-assigned this Nov 10, 2020
@trentm trentm added [zube]: Inbox agent-nodejs Make available for APM Agents project planning. and removed [zube]: Inbox labels Nov 10, 2020
@Nisgrak
Copy link

Nisgrak commented Jul 29, 2021

I'm having the same issue with knex 0.21.19.

I use node 14.17.0 and 3.18.0 of elastic-apm-node, load the data with ELASTIC_APM_X=Y npm run start this way:

// main.ts
import elasticApmNode from "elastic-apm-node/start";

....

It's a NestJS + Mikro-ORM project and I try to set the asyncHooks to false, as say in other issues but the span for knex isn't show.

image
image
image

@astorm
Copy link
Contributor

astorm commented Jul 30, 2021

@Nisgrak can you provide an example query using knex code that reproduces the behavior you're describing? That's usually the quickest way for us to figure out what's going on and why a span isn't being generated.

@Nisgrak
Copy link

Nisgrak commented Jul 30, 2021

An example repository?

I execute a manual query in knex like this:

async function findAll() {
	console.log("Test"); // This shows in the log above
	const test = this.repository.getKnex(); // Araw Knex instance

	return test.from("blacklist").select();
}

And the elastic log:
image

@astorm
Copy link
Contributor

astorm commented Jul 30, 2021

@Nisgrak A small-but-full program that reproduces the behavior you're seeing would be best. Also (especially if it's not possible for you to share a small-but-full program) if you could share the version of knex you're using, which database client driver (pg, mysql, etc.) you're using, and which version of the client driver that's at it'd be a big help.

Basically we want/need to be able to run a program and see the same "not generating a span" behavior you're seeing. If we can do that, we can do some debugging, figure out the problem, and get a fix in.

Does that make sense?

@Nisgrak
Copy link

Nisgrak commented Aug 1, 2021

@astorm The version of knex is 0.21.19.

I create a test repo https://github.com/Nisgrak/test-nest-mikroorm-apm, anything you need please ask 😄

Thanks you!

@astorm
Copy link
Contributor

astorm commented Aug 2, 2021

Thanks for the reproduction @Nisgrak -- looking things over it appears the code in https://github.com/Nisgrak/test-nest-mikroorm-apm uses the @mikro-orm/sqlite package to make its queries which, in turn, uses the sqlite3 package to make its queries.

Our instrumentation of knex is (per supported technology) to get better stack trace visualization. The underlying DB queries still rely on our instrumentation of individual database packages.

The Node.js Agent doesn't currently instrument the sqlite3 package. This is why you're not seeing spans.

Was sqlite3 the only package where you weren't seeing spans, or were there other db-drivers where this happened? If so, could you share which ones?

@Nisgrak
Copy link

Nisgrak commented Aug 3, 2021

Hi @astorm,

In fact I'm using the @mikro-orm/mariadb package, which uses mariadb internally.
I switch to @mikro-orm/mysql (mysql2 based) and now shows the spans...

If I make a PR to make mariadb compatible it would be accepted? I really need that compatibility.

Thanks for all your help!

@astorm
Copy link
Contributor

astorm commented Aug 3, 2021

@Nisgrak We'd absolutely accept a PR with support for the mariadb package. We've opened a feature request here: #2174. I'm going to close this issue and move the discussion over there.

@astorm astorm closed this as completed Aug 3, 2021
@trentm trentm removed their assignment Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning. enhancement
Projects
None yet
Development

No branches or pull requests

7 participants