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

fix: use SQL verb for mysql2 span name when query object is used #923

Merged
merged 4 commits into from
Mar 2, 2022

Conversation

nozik
Copy link
Contributor

@nozik nozik commented Feb 28, 2022

Which problem is this PR solving?

  • The span name behaves differently in case the query is provided as a string and as an object. It should behave the same.

Short description of the changes

  • Align the behavior, and update the tests accordingly

Checklist

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

@nozik nozik requested a review from a team February 28, 2022 14:08
assert.strictEqual(spans[0].name, 'SELECT');
assert.strictEqual(
spans[0].attributes[SemanticAttributes.DB_STATEMENT],
query
Copy link
Member

Choose a reason for hiding this comment

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

What is the type of this variable? Is this an object or a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a string. Code is now updated

@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #923 (2389b76) into main (1b3bb5f) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
+ Coverage   95.91%   96.10%   +0.18%     
==========================================
  Files          13       16       +3     
  Lines         856     1000     +144     
  Branches      178      205      +27     
==========================================
+ Hits          821      961     +140     
- Misses         35       39       +4     
Impacted Files Coverage Δ
.../opentelemetry-instrumentation-mysql2/src/utils.ts 100.00% <100.00%> (ø)
...tapackages/auto-instrumentations-node/src/utils.ts 97.87% <0.00%> (ø)
...etry-instrumentation-mysql2/src/instrumentation.ts 95.52% <0.00%> (ø)

@nozik nozik force-pushed the fix_mysql2_span_name branch from ebe8be2 to 38a18b6 Compare February 28, 2022 14:21
@dyladan dyladan changed the title fix: mysql2 span name fix: use SQL verb for mysql2 span name when query object is used Feb 28, 2022
@nozik nozik force-pushed the fix_mysql2_span_name branch from c21005c to c54657c Compare March 1, 2022 06:35
@nozik
Copy link
Contributor Author

nozik commented Mar 1, 2022

@blumamir Can you please merge? I've addressed your comment. Thanks!

@blumamir
Copy link
Member

blumamir commented Mar 1, 2022

@blumamir Can you please merge? I've addressed your comment. Thanks!

Yeah I just want to give a chance to other people that might want to review and comment.
We usually wait 2-3 days to make sure everyone gets to see the PR before merging.

@dyladan
Copy link
Member

dyladan commented Mar 2, 2022

It can't be merged right now anyway as it is out of date with the base branch and you have not granted permission for maintainers to update your branch so I can't merge the latest main.

@nozik
Copy link
Contributor Author

nozik commented Mar 2, 2022

@dyladan Rebased now. I'm having some issues with granting the permissions. Thanks

@dyladan
Copy link
Member

dyladan commented Mar 2, 2022

Thanks. I'll merge when the checks pass

@dyladan dyladan merged commit 3d1388b into open-telemetry:main Mar 2, 2022
@dyladan dyladan mentioned this pull request Mar 2, 2022
@nozik nozik deleted the fix_mysql2_span_name branch March 2, 2022 18:12
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.

3 participants