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(mysql2): support Connection.execute #1028

Merged
merged 3 commits into from
May 31, 2022

Conversation

aptomaKetil
Copy link
Contributor

Which problem is this PR solving?

The current instrumentation for mysql2 does not track queries made with Connection.execute, which is the helper method for preparing and executing a statement.

Short description of the changes

Since Connection.query and Connection.execute have the same signature and response formats (where it matters), all the existing wrapper code can simply be reused.

Tests for Connection.query and Pool.query have been mirrored for .execute.

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.

@aptomaKetil aptomaKetil requested a review from a team May 18, 2022 21:54
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 18, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: aptomaKetil / name: Ketil Øvrebø (081058e)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 19, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: aptomaKetil / name: Ketil Øvrebø (081058e)
  • ✅ login: Rauno56 / name: Rauno Viskus (6520476)

Copy link
Member

@rauno56 rauno56 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks, @aptomaKetil, and congrats for the first contribution!
I'll keep the PR open for a day or so more for others to see and react to it.

Please sign the CLA.

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #1028 (32b4984) into main (0dc580d) will increase coverage by 0.11%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #1028      +/-   ##
==========================================
+ Coverage   95.91%   96.02%   +0.11%     
==========================================
  Files          13       16       +3     
  Lines         856     1006     +150     
  Branches      178      206      +28     
==========================================
+ Hits          821      966     +145     
- Misses         35       40       +5     
Impacted Files Coverage Δ
...etry-instrumentation-mysql2/src/instrumentation.ts 94.36% <80.00%> (ø)
.../opentelemetry-instrumentation-mysql2/src/utils.ts 100.00% <0.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 97.95% <0.00%> (ø)

@blumamir
Copy link
Member

blumamir commented May 24, 2022

Edit: I see now that the options argument has the sql property on it so it should be compatible and it is covered in tests. LGTM, thanks for the contribution.

> Since Connection.query and Connection.execute have the same signature.

I am not sure that's right. execute accepts calls like this:

// execute(options, cb)

Where the sql value is set in options and not as first function parameter

@dyladan
Copy link
Member

dyladan commented May 27, 2022

I thought in some versions of this query actually called execute which would result in double spans. Are you sure this is not the case?

@aptomaKetil
Copy link
Contributor Author

I thought in some versions of this query actually called execute which would result in double spans. Are you sure this is not the case?

The tests for execute all have assertions on the number of spans produced by the instrumentation, so this is covered. I'm unaware of any previous discussion around this. I've taken a brief look at the mysql2 source and as far as I can tell, query and execute are on completely different code paths.

@rauno56
Copy link
Member

rauno56 commented May 31, 2022

Thanks, @aptomaKetil and congrats for your first contribution!

@rauno56 rauno56 merged commit 3e2f9c5 into open-telemetry:main May 31, 2022
@dyladan dyladan mentioned this pull request May 31, 2022
@aptomaKetil aptomaKetil deleted the mysql2-support-execute branch August 11, 2022 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants