feat(node): Add ESM support for postgres.js instrumentation #17961
Conversation
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
onurtemizkan
force-pushed
the
onur/postgres-js-esm-support
branch
2 times, most recently
from
f635159 to
96641f9
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds ESM (ECMAScript Module) support for postgres.js instrumentation by refactoring the patching approach from file-based module instrumentation to module export wrapping. The instrumentation now wraps the main postgres export function to intercept SQL instance creation and instrument query methods dynamically.
Key changes:
- Refactored instrumentation to wrap the main postgres export instead of patching internal files
- Added comprehensive ESM test coverage alongside existing CJS tests
- Enhanced SQL query sanitization to handle PostgreSQL placeholders ($1, $2, etc.)
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/node/src/integrations/tracing/postgresjs.ts | Complete rewrite of instrumentation using module export wrapping; adds ESM support and improves query/connection handling |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/test.ts | Added ESM tests, requestHook tests, URL initialization tests, and unsafe query tests |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario.mjs | New ESM scenario file for testing ESM imports |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario.cjs | New CJS scenario file with consolidated initialization |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario.js | Enhanced with additional query test cases |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario-url.mjs | New ESM test scenario for URL-based postgres initialization |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario-url.cjs | New CJS test scenario for URL-based postgres initialization |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario-unsafe.cjs | New test scenario for sql.unsafe() method |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario-requestHook.mjs | New ESM test scenario for requestHook functionality |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario-requestHook.js | New CJS test scenario for requestHook functionality |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/instrument.mjs | New ESM instrumentation file for tests |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/instrument.cjs | New CJS instrumentation file for tests |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/instrument-requestHook.mjs | New ESM instrumentation with requestHook configuration |
| dev-packages/node-integration-tests/suites/tracing/postgresjs/instrument-requestHook.cjs | New CJS instrumentation with requestHook configuration |
Comments suppressed due to low confidence (1)
dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario.js:83
- The floating promise should be handled with proper error handling. Consider using
run().catch(console.error)or wrapping in an async IIFE to ensure errors are not silently swallowed.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
run();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let spanName = sanitizedSqlQuery?.trim() || ''; | ||
| if (!spanName) { | ||
| // Fallback: try to extract operation from the sanitized query | ||
| const operationMatch = sanitizedSqlQuery?.match(SQL_OPERATION_REGEX); | ||
| spanName = operationMatch?.[1] ? `db.${operationMatch[1].toLowerCase()}` : 'db.query'; |
There was a problem hiding this comment.
Logic error: spanName is set to the trimmed sanitizedSqlQuery on line 363, which means the condition !spanName on line 364 will only be true if sanitizedSqlQuery is empty or undefined. However, line 366 attempts to match the same sanitizedSqlQuery against SQL_OPERATION_REGEX. This code path is unreachable when sanitizedSqlQuery has content. The fallback logic appears to be intended for cases where sanitizedSqlQuery exists but is not suitable as a span name, but the current implementation doesn't achieve this.
| let spanName = sanitizedSqlQuery?.trim() || ''; | |
| if (!spanName) { | |
| // Fallback: try to extract operation from the sanitized query | |
| const operationMatch = sanitizedSqlQuery?.match(SQL_OPERATION_REGEX); | |
| spanName = operationMatch?.[1] ? `db.${operationMatch[1].toLowerCase()}` : 'db.query'; | |
| let spanName: string; | |
| const operationMatch = sanitizedSqlQuery?.match(SQL_OPERATION_REGEX); | |
| if (operationMatch?.[1]) { | |
| spanName = `db.${operationMatch[1].toLowerCase()}`; | |
| } else if (sanitizedSqlQuery?.trim()) { | |
| spanName = sanitizedSqlQuery.trim(); | |
| } else { | |
| spanName = 'db.query'; |
| InstrumentationBase, | ||
| InstrumentationNodeModuleDefinition, | ||
| InstrumentationNodeModuleFile, | ||
| safeExecuteInTheMiddle, |
There was a problem hiding this comment.
The import of safeExecuteInTheMiddle should be removed from '@opentelemetry/instrumentation' and imported from '@sentry/core' instead, as indicated by the usage pattern at line 394. However, checking the actual usage shows it is used correctly. If this import is not used elsewhere in the file, it should be removed.
dev-packages/node-integration-tests/suites/tracing/postgresjs/test.ts
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
dev-packages/e2e-tests/test-applications/cloudflare-astro/package.json
Outdated
Show resolved
Hide resolved
onurtemizkan
force-pushed
the
onur/postgres-js-esm-support
branch
from
d9a0dfa to
35bf27d
Compare
onurtemizkan
force-pushed
the
onur/postgres-js-esm-support
branch
from
35bf27d to
5143c10
Compare
| spanName = sanitizedSqlQuery.trim(); | ||
| } else { | ||
| spanName = 'db.query'; | ||
| } |
There was a problem hiding this comment.
Bug: Span description uses operation prefix instead of full query
The span name logic sets spanName to db.${operation} (e.g., db.create, db.select) for recognized SQL operations, but this value becomes the span's description when serialized (as confirmed in sentrySpan.ts line 226: description: this._name). The tests in this same PR explicitly expect the span description to be the full sanitized SQL query (e.g., 'CREATE TABLE "User" (...)'), not the abbreviated db.create format. This mismatch means the detailed test expectations will fail.
onurtemizkan
force-pushed
the
onur/postgres-js-esm-support
branch
from
5143c10 to
898802b
Compare
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| sqlQuery | ||
| .replace(/\s+/g, ' ') | ||
| .trim() // Remove extra spaces including newlines and trim | ||
| .substring(0, 1024) // Truncate to 1024 characters | ||
| .replace(/--.*?(\r?\n|$)/g, '') // Single line comments | ||
| // Remove comments first (they may contain newlines and extra spaces) | ||
| .replace(/--.*$/gm, '') // Single line comments (multiline mode) | ||
| .replace(/\/\*[\s\S]*?\*\//g, '') // Multi-line comments |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
onurtemizkan
force-pushed
the
onur/postgres-js-esm-support
branch
from
b0cf889 to
29ff73f
Compare
onurtemizkan
force-pushed
the
onur/postgres-js-esm-support
branch
from
299da8a to
11a366d
Compare
|
...any updates? |
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
Thanks for making the changes. This looks good to me for now, though I'd like us to improve parameterization in a follow-up PR (see slack convo).
dev-packages/node-integration-tests/suites/tracing/postgresjs/test.ts
Outdated
Show resolved
Hide resolved
dev-packages/node-integration-tests/suites/tracing/postgresjs/scenario-requestHook.js
Outdated
Show resolved
Hide resolved
| }), | ||
| }; | ||
|
|
||
| await createRunner(__dirname, 'scenario-requestHook.js') |
There was a problem hiding this comment.
Bug: CJS requestHook test references non-existent scenario file
The CJS requestHook test at line 536 references scenario-requestHook.js, but this file is not created in the PR. The diff shows scenario-requestHook.mjs being added for ESM testing, but no corresponding CJS file (scenario-requestHook.js or scenario-requestHook.cjs) exists. This inconsistency with the other scenario files (which have both CJS and ESM versions) means the test will fail at runtime due to a missing file.
Resolves: #17889
This PR rewrites the
postgresjsinstrumentation with a new architecture:Added ESM support via
replaceExportsMoved to main export wrapping instead of internal module patching
connection.jsandquery.jsinternal modulesConnection context is now stored directly on sql instances using
CONNECTION_CONTEXT_SYMBOLQuery.prototypefallback (CJS only)Query.prototype.handleas a fallback for pre-existing sql instancesQUERY_FROM_INSTRUMENTED_SQLmarker to prevent duplicate spansAlso,
portattribute is now stored as a number per OTEL semantic conventionscommandisn't available