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
| .replace(/\b\d+\b/g, '?') // Replace standalone numbers | ||
| // Collapse IN and in clauses (eg. IN (?, ?, ?, ?) to IN (?)) | ||
| .replace(/\bIN\b\s*\(\s*\?(?:\s*,\s*\?)*\s*\)/gi, 'IN (?)') | ||
| .substring(0, 1024) // Truncate to 1024 characters AFTER sanitization |
There was a problem hiding this comment.
m: let's avoid truncating the query string. We decided to avoid truncation as much as possible in the SDK (and rather have Relay do that).
| .substring(0, 1024) // Truncate to 1024 characters AFTER sanitization | |
| .substring(0, 1024) // Truncate to 1024 characters AFTER sanitization |
| // Remove comments first (they may contain newlines and extra spaces) | ||
| .replace(/--.*$/gm, '') // Single line comments (multiline mode) |
There was a problem hiding this comment.
l: Let's add a couple of unit tests for the sanitization
| op: 'db', | ||
| }, | ||
| (span: Span) => { | ||
| addOriginToSpan(span, 'auto.db.otel.postgres'); |
There was a problem hiding this comment.
l: maybe I've missed something but this is our instrumentation, right? Or did we fork it from OTel?
| addOriginToSpan(span, 'auto.db.otel.postgres'); | |
| addOriginToSpan(span, 'auto.db.postgresjs'); |
There was a problem hiding this comment.
Yes, actually, that was a mistake in the initial implementation. I updated the origin to auto.db.postgresjs. But not sure whether that may make this a breaking change?
onurtemizkan
force-pushed
the
onur/postgres-js-esm-support
branch
from
299da8a to
11a366d
Compare
| .replace(/\/\*[\s\S]*?\*\//g, '') // Multi-line comments | ||
| .replace(/;\s*$/, '') // Remove trailing semicolons | ||
| .replace(/\b\d+\b/g, '?') // Replace standalone numbers | ||
| // Collapse whitespace to a single space | ||
| // Collapse whitespace to a single space (after removing comments) | ||
| .replace(/\s+/g, ' ') | ||
| // Collapse IN and in clauses | ||
| // eg. IN (?, ?, ?, ?) to IN (?) | ||
| .replace(/\bIN\b\s*\(\s*\?(?:\s*,\s*\?)*\s*\)/g, 'IN (?)') | ||
| .trim() // Remove extra spaces and trim | ||
| // Replace standalone numbers and parameterized queries ($1, $2, etc.) BEFORE truncation | ||
| .replace(/\$\d+/g, '?') // Replace PostgreSQL placeholders ($1, $2, etc.) | ||
| .replace(/\b\d+\b/g, '?') // Replace standalone numbers | ||
| // Collapse IN and in clauses (eg. IN (?, ?, ?, ?) to IN (?)) | ||
| .replace(/\bIN\b\s*\(\s*\?(?:\s*,\s*\?)*\s*\)/gi, 'IN (?)') | ||
| ); |
There was a problem hiding this comment.
Bug: The _sanitizeSqlQuery() method is missing SQL query truncation, allowing arbitrarily long queries in span attributes.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The _sanitizeSqlQuery() method in packages/node/src/integrations/tracing/postgresjs.ts no longer truncates SQL queries to 1024 characters before assigning them to ATTR_DB_QUERY_TEXT span attributes. This is a regression from the previous implementation, which included a .substring(0, 1024) operation. The absence of truncation can lead to arbitrarily long SQL queries being stored in span attributes, increasing payload sizes and potentially impacting transmission and storage efficiency.
💡 Suggested Fix
Re-add .substring(0, 1024) to the return statement of the _sanitizeSqlQuery() method in packages/node/src/integrations/tracing/postgresjs.ts to restore the original truncation behavior.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/node/src/integrations/tracing/postgresjs.ts#L507-L520
Potential issue: The `_sanitizeSqlQuery()` method in
`packages/node/src/integrations/tracing/postgresjs.ts` no longer truncates SQL queries
to 1024 characters before assigning them to `ATTR_DB_QUERY_TEXT` span attributes. This
is a regression from the previous implementation, which included a `.substring(0, 1024)`
operation. The absence of truncation can lead to arbitrarily long SQL queries being
stored in span attributes, increasing payload sizes and potentially impacting
transmission and storage efficiency.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4826878
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