-
Notifications
You must be signed in to change notification settings - Fork 534
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
feat: knex instrumentation #506
feat: knex instrumentation #506
Conversation
... adding a feature for limiting the length of the statement. |
Codecov Report
@@ Coverage Diff @@
## main #506 +/- ##
=======================================
Coverage 94.84% 94.84%
=======================================
Files 152 152
Lines 9226 9226
Branches 918 918
=======================================
Hits 8750 8750
Misses 476 476 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
Thanks, @blumamir, I think I addressed everything! |
Will wait for @blumamir approval before merging this since he was the most involved approver |
plugins/node/opentelemetry-instrumentation-knex/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
Added few more minor comments, but overall LGTM. |
oops, see now that contrib v0.20.0 was already released yesterday. thought it was still in proposal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it locally, works great :) thanks @rauno56 good job.
Beside from the few minor open comments that I added, I think it's good to go.
Which problem is this PR solving?
This adds auto-instrumentation for
knex
.Short description of the changes
It emits spans from SQL queries regardless of the driver. Knex makes its query builder a custom thenable instance so
await knex(table)...
executes the query. Elegant API, but it is a lost cause for context propagation. To battle this I'm storing context before possibleawait
s and use that as a parent in thequery
method.