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(pg): Patch client inside lib and lib/pg-native #2563

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

onurtemizkan
Copy link
Contributor

@onurtemizkan onurtemizkan commented Nov 28, 2024

Which problem is this PR solving?

Ref:

Debugging the reproduction of getsentry/sentry-javascript#14238, it seems @opentelemetry/instrumentation-pg fails to pick up the pg module while it successfully picks up and patches pg-pool.

Digging into node_modules of the reproduction, pg package did not have an index on its root, and the whole implementation resides in lib. Which made me suspect that relying on the root exports may not be enough with certain bundlers (Vite in this case) and runtimes. Instrumenting the client file instead worked well and spans were created successfully.

The same applies to pg-native which pg has bindings inside lib/native. So it's possible to patch it from the pg itself. That is similar to the approach Sentry was using to instrument pg-native before migrating to OTEL.

Short description of the changes

  • Added file patchers for lib/client.js and lib/native/client.js on top of the original implementation.
  • Tested both locally.

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.91%. Comparing base (c2ad0af) to head (6ac36ac).

Files with missing lines Patch % Lines
...elemetry-instrumentation-pg/src/instrumentation.ts 93.75% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2563      +/-   ##
==========================================
+ Coverage   90.79%   90.91%   +0.11%     
==========================================
  Files         169      169              
  Lines        8059     8075      +16     
  Branches     1645     1645              
==========================================
+ Hits         7317     7341      +24     
+ Misses        742      734       -8     
Files with missing lines Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts 95.00% <93.75%> (+4.78%) ⬆️

@onurtemizkan onurtemizkan force-pushed the pg-lib-and-native-support branch from 71e08b3 to 5cd3b7c Compare December 5, 2024 19:10
@onurtemizkan onurtemizkan force-pushed the pg-lib-and-native-support branch 3 times, most recently from 8ea56bd to 13bbbcc Compare December 5, 2024 23:38
@onurtemizkan onurtemizkan force-pushed the pg-lib-and-native-support branch from 13bbbcc to 21840f9 Compare December 6, 2024 11:00
@onurtemizkan onurtemizkan force-pushed the pg-lib-and-native-support branch from 44988a3 to ba9fafa Compare December 6, 2024 14:49
@onurtemizkan onurtemizkan force-pushed the pg-lib-and-native-support branch from 4dfc927 to e6818f6 Compare December 6, 2024 17:29
@onurtemizkan onurtemizkan marked this pull request as ready for review December 6, 2024 17:54
@onurtemizkan onurtemizkan requested a review from a team as a code owner December 6, 2024 17:54
@danielabel
Copy link

I'd very much like to support this PR getting merged. It solves a problem I am seeing where pg-native is not sending traces when the native flag is used in Sequelize

@pichlermarc
Copy link
Member

cc @maryliag (component owner) 🙂

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Hi, nice work working on this. I added a few questions :)

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

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.

4 participants