[fix: plugin-pg] Support Query config object again #3085
Conversation
| let pgQuery = { | ||
| text: arguments[0] | ||
| } | ||
| let pgQuery = typeof arguments[0] === 'object' ? { |
There was a problem hiding this comment.
Why clone the object instead of passing it as-is? It seems like before the regression it wasn't cloned.
There was a problem hiding this comment.
As mentioned, I don't totally understand why this was a mutable let in the first place or why we are overwriting this on line 57 from the queue (or what the queue is). It feels like an overloaded variable with two totally separate uses. So was trying to as closely preserve the original authors intent here. Feel free to update this though or take the findings and do something different with them too (there's an alternative approach I describe as well)! I'm not wedded to the specifics, was kind of just trying to show the specific issue.
There was a problem hiding this comment.
Looks like a fix for this has actually landed in #3087
Fixes #3007
What does this PR do?
There are multiple formats for querying the pg client. Previously dd-trace supported all of them, but this broke in a recent refactor. The call signatures are:
(query, ...)
(query, values, ...)
(object: {}, ...)
https://node-postgres.com/features/queries
I don't totally understand what's going on with the
queryQueuebut it seems like since we set the query as a let we are anticipating the object might get shoved onto a queue. This means we need to copy the full arguments object to initially log it and then to later apply it (i.e. just copying the query text didn't seem like enough)The bigger problem is that we have two formats for this from PG, but we are expecting one in the pg plugin itself. This could potentially be simpler if we just had that plugin check for .text on the object instead of doing it here. So feel free to take that approach too!
Motivation
This broke in 3.17 from bad code in #2938.