fix: add support for pg 7 changes #702
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
googlebot
added
the
cla: yes
kjin
force-pushed
the
pg-7
branch
2 times, most recently
from
99eb8fe
to
01b4244
Compare
kjin
changed the title
|
Didn't look at the code yet but CI is failing. |
kjin
force-pushed
the
pg-7
branch
2 times, most recently
from
01b4244
to
9e901cb
Compare
Codecov Report
@@ Coverage Diff @@
## master #702 +/- ##
==========================================
- Coverage 91.02% 90.77% -0.26%
==========================================
Files 29 29
Lines 1438 1485 +47
Branches 280 293 +13
==========================================
+ Hits 1309 1348 +39
- Misses 52 57 +5
- Partials 77 80 +3
Continue to review full report at Codecov.
|
jinwoo
reviewed
Mar 28, 2018
src/plugins/plugin-pg.ts
Outdated
| span.addLabel('values', pgQuery.values); | ||
| const args: pg_6.QueryReturnValue[] = | ||
| Array.prototype.slice.call(arguments, 0); | ||
| if (args.length >= 1) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/plugins/plugin-pg.ts
Outdated
| // - ...[text: string, values?: Array<any>] | ||
| // See: https://node-postgres.com/guides/upgrading | ||
|
|
||
| if (args.length >= 1) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/plugins/plugin-pg.ts
Outdated
|
|
||
| if (args.length >= 1) { | ||
| // Extract query text and values, if needed. | ||
| if (api.enhancedDatabaseReportingEnabled()) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/plugins/plugin-pg.ts
Outdated
| span.addLabel('error', err); | ||
| } | ||
| if (res) { | ||
| span.addLabel('row_count', res.rowCount); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
jinwoo
approved these changes
Mar 29, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR converts the PostGreSQL tracing plugin to TypeScript, makes changes to support the new API in pg@7, adds a few more tests, and changes the way
valuelabels are collected.pg@7.plugin-pg.tsto ES6 + type annotations, and installspg@7types. There should be no behavioral change.pg@7, based on source code and the upgrade guide.pg@6, an object is always returned byClient#query, and is simultaneously aPromise[Like], anEventEmitter, and an object{ text: string; values?: Array, callback?: Function }.pg@7, the return value ofClient#queryis either aPromise,EventEmitter, orundefined, depending on the input parameters.valuesfrom the output object ofClient#query; inpg@7, these fields are not guaranteed to exist, so we must gather them from input parameters instead. However, there is a mismatch betweenvaluesgotten from input and output; outputvaluesare coerced to strings and escaped to avoid SQL injection. So to keep things consistent, we now also getvaluesfrom the inputs toClient#queryinpg@6. (I don't think that these values being unescaped should pose a threat because their values are simply stored, not executed; feel free to dispute that.)Submittables. Converting this test to idiomatic TypeScript is out of the scope of this PR.