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

feat: knex instrumentation #506

Merged
merged 40 commits into from Jun 9, 2021

Conversation

rauno56
Copy link
Member

@rauno56 rauno56 commented May 28, 2021

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 possible awaits and use that as a parent in the query method.

@rauno56 rauno56 requested a review from a team as a code owner May 28, 2021 15:47
@rauno56
Copy link
Member Author

rauno56 commented May 28, 2021

... adding a feature for limiting the length of the statement.

@codecov
Copy link

codecov bot commented May 28, 2021

Codecov Report

Merging #506 (7ace5d8) into main (c7df125) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #506   +/-   ##
=======================================
  Coverage   94.84%   94.84%           
=======================================
  Files         152      152           
  Lines        9226     9226           
  Branches      918      918           
=======================================
  Hits         8750     8750           
  Misses        476      476           

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vmarchaud vmarchaud requested a review from a team June 1, 2021 20:28
@rauno56
Copy link
Member Author

rauno56 commented Jun 4, 2021

Thanks, @blumamir, I think I addressed everything!

@dyladan dyladan added the enhancement New feature or request label Jun 4, 2021
@dyladan
Copy link
Member

dyladan commented Jun 4, 2021

Will wait for @blumamir approval before merging this since he was the most involved approver

@vmarchaud vmarchaud requested a review from blumamir June 4, 2021 17:23
@blumamir
Copy link
Member

blumamir commented Jun 5, 2021

Will wait for @blumamir approval before merging this since he was the most involved approver

Added few more minor comments, but overall LGTM.
I don't want to hold the PR, but was reviewing it by just browsing the code. Didn't actually run it in a real environment.
If not urgent, I can find time tomorrow to play with it on my setup.
But that's completely optional - can be skiped if you want to merge it already for the v0.20.0 release.

@blumamir
Copy link
Member

blumamir commented Jun 5, 2021

Will wait for @blumamir approval before merging this since he was the most involved approver

Added few more minor comments, but overall LGTM.
I don't want to hold the PR, but was reviewing it by just browsing the code. Didn't actually run it in a real environment.
If it's not urgent, I can find time tomorrow to play with it on my setup.
But that's completely optional - can be skiped it if you want to merge it already for the v0.20.0 release.

oops, see now that contrib v0.20.0 was already released yesterday. thought it was still in proposal.

Copy link
Member

@blumamir blumamir left a 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.

@vmarchaud vmarchaud merged commit 19d5fa4 into open-telemetry:main Jun 9, 2021
@rauno56 rauno56 deleted the feat/knex-instrumentation branch June 9, 2021 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants