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.

Sign up for GitHub

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

Jump to bottom

instrumentation-knex doesn't handle errors raised by better-sqlite3 #2297

Open
jmadureira opened this issue Jun 25, 2024 · 3 comments · May be fixed by #2298
Open

instrumentation-knex doesn't handle errors raised by better-sqlite3 #2297

jmadureira opened this issue Jun 25, 2024 · 3 comments · May be fixed by #2298
Assignees
@AbhiPrasad
Labels
bug Something isn't working

Comments

@jmadureira
Copy link

What version of OpenTelemetry are you using?

"@opentelemetry/api": "^1.9.0",
"@opentelemetry/auto-instrumentations-node": "^0.47.1",
"@opentelemetry/exporter-prometheus": "^0.52.1",
"@opentelemetry/exporter-trace-otlp-grpc": "^0.52.1",
"@opentelemetry/sdk-node": "^0.52.0",

What version of Node are you using?

v18.18.2

What did you do?

Instrument any application that uses knex and better-sqlite3 and trigger any SQL error.

What did you expect to see?

A span or trace should be correctly created and emitted.

What did you see instead?

A TypeError: Expected second argument to be a string is thrown instead and the application probably stops. Either way not span gets created.

Additional context

Looking at the stackstrace:

cause: TypeError: Expected second argument to be a string
      at new SqliteError (###/node_modules/better-sqlite3/lib/sqlite-error.js:9:9)
      at Object.cloneErrorWithNewMessage (###/node_modules/@opentelemetry/instrumentation-knex/src/utils.ts:48:25)
      at <anonymous> (###/node_modules/@opentelemetry/instrumentation-knex/src/instrumentation.ts:179:39)
      at async Runner.ensureConnection (###/node_modules/knex/lib/execution/runner.js:318:14)
      at async Runner.run (###/node_modules/knex/lib/execution/runner.js:30:19)
      at async ###/node_modules/knex/lib/execution/batch-insert.js:31:30

The problem seems to be between https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-knex/src/utils.ts#L48 that creates a new instance of the underlying error and https://github.com/WiseLibs/better-sqlite3/blob/master/lib/sqlite-error.js#L9 which requires callers to provide 2 arguments. The 2nd argument (code) is not being passed by the knex instrumenter.
@jmadureira jmadureira added the bug Something isn't working label Jun 25, 2024
@AbhiPrasad AbhiPrasad linked a pull request Jun 25, 2024 that will close this issue
Open
@AbhiPrasad
Copy link
Member

I opened #2298 to address this! Reviews appreciated :)

@pichlermarc
Copy link
Member

@AbhiPrasad thanks for working on the fix, would you mind if I assign you? 🙂

@AbhiPrasad
Copy link
Member

For sure!

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

@AbhiPrasad AbhiPrasad

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(instrumentation-knex): Support better-sqlite3 errors getsentry/opentelemetry-js-contrib
3 participants
@jmadureira @AbhiPrasad @pichlermarc