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 10 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.87%. Comparing base (baccd98) to head (e6818f6).
Report is 1 commits behind head on main.

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.80%   90.87%   +0.06%     
==========================================
  Files         169      169              
  Lines        8026     8042      +16     
  Branches     1635     1635              
==========================================
+ Hits         7288     7308      +20     
+ Misses        738      734       -4     
Files with missing lines Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts 95.00% <93.75%> (+4.78%) ⬆️

... and 1 file with indirect coverage changes

@onurtemizkan onurtemizkan force-pushed the pg-lib-and-native-support branch from 71e08b3 to 5cd3b7c Compare December 5, 2024 19:10
import { PgInstrumentation } from '../../build/src/index.js';

const CONFIG = {
user: process.env.POSTGRES_USER || 'postgres',

Check failure

Code scanning / CodeQL

Hard-coded credentials Critical test

The hard-coded value "postgres" is used as
user name
.

const CONFIG = {
user: process.env.POSTGRES_USER || 'postgres',
password: process.env.POSTGRES_PASSWORD || 'postgres',

Check failure

Code scanning / CodeQL

Hard-coded credentials Critical test

The hard-coded value "postgres" is used as
password
.
@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
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.

2 participants