fix(pg): avoid disjoint spans from pg instrumentation #1122
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1122 +/- ##
==========================================
- Coverage 96.07% 95.67% -0.41%
==========================================
Files 14 18 +4
Lines 892 1156 +264
Branches 191 233 +42
==========================================
+ Hits 857 1106 +249
- Misses 35 50 +15
|
| @@ -50,16 +50,20 @@ function getCommandFromText(text?: string): string { | |||
| return words[0].length > 0 ? words[0] : 'unknown'; | |||
| } | |||
|
|
|||
| export function getJDBCString(params: PgClientConnectionParams) { | |||
There was a problem hiding this comment.
What is the motivation to replace PgClientConnectionParams?
There was a problem hiding this comment.
Removed this change and reused the same type albeit with changing the fields to possibly be undefined. In client.connect case the client itself is passed to getConnectionString and the client has all of these fields as optional. Perhaps the PgClientConnectionParams fields should've been optional since the beginning.
| this: pgTypes.Client, | ||
| err: Error | ||
| ) { | ||
| if (err) { |
There was a problem hiding this comment.
maybe also record exception?
span.recordExcpetion(err)
| @@ -236,3 +240,22 @@ export function patchCallbackPGPool( | |||
| cb.call(this, err, res, done); | |||
| }; | |||
| } | |||
|
|
|||
| export function patchConnectErrorCallback( | |||
There was a problem hiding this comment.
Is this callback only for errors?
There was a problem hiding this comment.
Renamed it to patchClientConnectCallback
| if (!(connectResult instanceof Promise)) { | ||
| return connectResult; | ||
| } | ||
|
|
||
| const connectResultPromise = connectResult as Promise<unknown>; | ||
| return context.bind( | ||
| context.active(), | ||
| connectResultPromise | ||
| .then( | ||
| result => | ||
| new Promise(resolve => { | ||
| span.end(); | ||
| resolve(result); | ||
| }) | ||
| ) | ||
| .catch((error: Error) => { | ||
| return new Promise((_, reject) => { | ||
| span.setStatus({ | ||
| code: SpanStatusCode.ERROR, | ||
| message: error.message, | ||
| }); | ||
| span.end(); | ||
| reject(error); | ||
| }); | ||
| }) | ||
| ); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
This block is a duplicate of the code in _getPoolConnectPatch.
Could be nice to extract into a function and invoke from both places
There was a problem hiding this comment.
Refactored this into a single function named handleConnectResult
| @@ -236,3 +240,22 @@ export function patchCallbackPGPool( | |||
| cb.call(this, err, res, done); | |||
| }; | |||
| } | |||
|
|
|||
| export function patchConnectErrorCallback( | |||
There was a problem hiding this comment.
There is already a function patchCallbackPGPool which does almost the same thing. The only difference is the arguments list to the callback which we don't use.
Can we reuse one function for both cases?
Maybe write it with arguments array parameter instead of listing the different arguments for each case
There was a problem hiding this comment.
I am not sure about this, think there is some value in keeping descriptive function names (i.e. the created callback). I did rename the function though
|
Seem to be unrelated unit tests failing atm |
plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
Which problem is this PR solving?
pg.Client.connectwas uninstrumented. Withnetanddnsinstrumentations in use it meant that whenclient.connect()was called with no active context, it produced 2 disjoint traces, one fornetand one fordns. This now produces a new span calledpg.connect.pg.Pool.connecthad the same problem where it did not activate the newly created context, causing disjoint spans fornetanddns.Downstream spans from
pgare now linked topgspans correctly.Misc: When instrumenting
pg.Client.connectthe connection string containedjdbcprefix (which I removed). How did this end up in a Node.js instrumentation? 🤔Checklist
npm run test-all-versionsfor the edited package(s) on the latest commit if applicable.