-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Knex #468
Support Knex #468
Conversation
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good overall but I agree that we should understand what async work is being done by knex to cause this context loss (even though this bluebird patch does fix this problem).
test/plugins/test-trace-knex.js
Outdated
databaseResultReportingSize: RESULT_SIZE | ||
}); | ||
assert = require('assert'); | ||
knex = require('./fixtures/knex0.12')({ |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/plugins/plugin-bluebird.js
Outdated
} | ||
|
||
function patchPromise(Promise, api) { | ||
shimmer.wrap(Promise.prototype, '_addCallbacks', function(original) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
425818b
to
6010c4b
Compare
shimmer.wrap(Client.prototype, 'runner', function(original) { | ||
return function() { | ||
var runner = original.apply(this, arguments); | ||
runner.query = api.wrap(runner.query); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/plugins/plugin-knex.js
Outdated
function interceptKnex(Knex, api) { | ||
return function() { | ||
var result = Knex.apply(this, arguments); | ||
var proto = Object.getPrototypeOf(result.client); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/plugins/plugin-knex.js
Outdated
var proto = Object.getPrototypeOf(result.client); | ||
shimmer.wrap(proto, 'transaction', function(original) { | ||
return function() { | ||
var args = Array.prototype.slice.call(arguments).map(function(item) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
I think this patch is specific to mysql. I will investigate this further.
This commit intercepts the module root to patch the `transaction` method. This works because the `knex` module exports a constructor and the object that the constructor constructs has a `client` property that is the client being used. This client can have its `transaction` method updated in a way to preserve context.
80d3264
to
aa7279a
Compare
src/plugins/plugin-knex.js
Outdated
var util = require('util'); | ||
var is = require('is'); | ||
|
||
var VERSIONS = '0.12.x'; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
src/plugins/plugin-knex.js
Outdated
|
||
function interceptTransaction(Transaction, api) { | ||
function WrappedTransaction(client, container, config, outerTx) { | ||
Transaction.call(this, client, wrapIfFn(container, api), config, outerTx); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nit once appveyor and circle are passing (travis failures are unrelated).
test/plugins/test-trace-knex.js
Outdated
enhancedDatabaseReporting: true, | ||
databaseResultReportingSize: RESULT_SIZE | ||
}); | ||
assert = require('assert'); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This commit includes support and tests for knex satsifying `>=0.10 <=0.13`. PR-URL: googleapis/cloud-trace-nodejs#468
googleapis/synthtool@1df68ed commit 1df68ed6735ddce6797d0f83641a731c3c3f75b4 Author: Alexander Fenster <fenster@google.com> Date: Mon Apr 6 16:17:34 2020 -0700 fix: apache license URL (#468)
This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/9c94202f-63a5-4df0-9d76-871a00f99b85/targets
This PR was generated using Autosynth. 🌈 Synth log will be available here: https://source.cloud.google.com/results/invocations/9c94202f-63a5-4df0-9d76-871a00f99b85/targets
No description provided.