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(knex): nested queries result in wrong span names #1537

Conversation

nachogiljaldo
Copy link
Contributor

Fixes #1535

Which problem is this PR solving?

  • instrumentation-knex generates wrong span names when the knex query has a nested query in its first element

Short description of the changes

  • consider the possibility of nested queries and add the corresponding unit tests to deal with it

@nachogiljaldo nachogiljaldo requested a review from a team June 15, 2023 08:16
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 15, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from rauno56 June 15, 2023 08:17
@nachogiljaldo nachogiljaldo force-pushed the fix_table_calculation_on_nested_queries branch from 9521964 to 1caddef Compare June 15, 2023 08:19
@nachogiljaldo nachogiljaldo marked this pull request as draft June 15, 2023 08:22
@nachogiljaldo nachogiljaldo changed the title Fixes calculation of the table on nested queries. fix(knex): nested queries result in wrong span names Jun 15, 2023
@nachogiljaldo nachogiljaldo marked this pull request as ready for review June 15, 2023 08:23
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Generally looks good to me but I would prefer if we could remove the any in favor of a more descriptive type

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #1537 (50c395b) into main (3f2bfe8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1537   +/-   ##
=======================================
  Coverage   91.37%   91.38%           
=======================================
  Files         127      127           
  Lines        6345     6350    +5     
  Branches     1274     1275    +1     
=======================================
+ Hits         5798     5803    +5     
  Misses        547      547           
Files Changed Coverage Δ
...emetry-instrumentation-knex/src/instrumentation.ts 98.78% <100.00%> (ø)
...de/opentelemetry-instrumentation-knex/src/utils.ts 90.24% <100.00%> (+1.35%) ⬆️

@nachogiljaldo
Copy link
Contributor Author

Hey @rauno56 I believe your review is requested in order to merge this. Any chance you can have a look in the near future?

@nachogiljaldo
Copy link
Contributor Author

@rauno56 any chance you have a look at this?

@haddasbronfman
Copy link
Member

Hi. @rauno56 is no longer / very rarely working on this library.
But you need only one approval (from an approver or from the code owner).
I would ask @dyladan or @pichlermarc to resolve the issue above and then any other approver can merge this for you.

@dyladan
Copy link
Member

dyladan commented Jul 27, 2023

I'm out of office until Tuesday sorry

@nachogiljaldo nachogiljaldo requested a review from blumamir August 7, 2023 09:39
@nachogiljaldo
Copy link
Contributor Author

@blumamir I am afraid I can't merge because @rauno56 is still requested and I can't remove him. Anything you can do?

@blumamir blumamir merged commit f4df836 into open-telemetry:main Aug 10, 2023
@blumamir
Copy link
Member

@blumamir I am afraid I can't merge because @rauno56 is still requested and I can't remove him. Anything you can do?

It's ok I merged it 👍

@dyladan dyladan mentioned this pull request Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

instrumentation-knex table calculated incorrectly on nested tables
6 participants