Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support Knex #468

Merged
merged 14 commits into from
May 23, 2017
Merged

Support Knex #468

merged 14 commits into from
May 23, 2017

Conversation

DominicKramer
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 14, 2017
@DominicKramer
Copy link
Contributor Author

PTAL

matthewloring
matthewloring reviewed Apr 16, 2017
View reviewed changes
Copy link
Contributor

@matthewloring matthewloring left a 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.

Sign in to view

This comment was marked as spam.

Sign in to view

src/plugins/plugin-bluebird.js Outdated
}

function patchPromise(Promise, api) {
shimmer.wrap(Promise.prototype, '_addCallbacks', function(original) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

matthewloring
matthewloring suggested changes May 11, 2017
View reviewed changes
src/plugins/plugin-knex.js
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.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

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.

Sign in to view

This comment was marked as spam.

Sign in to view

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.

Sign in to view

This comment was marked as spam.

Sign in to view

@DominicKramer
Initial commit
698af35
@DominicKramer
Fix the test-config-plugins.js test
6b9b5da
@DominicKramer
Fix context loss in knex without patching bluebird
a7c6bdf 
I think this patch is specific to mysql.  I will investigate this
further.
@DominicKramer
Fix the test-config-plugins.js test
f80681f
@DominicKramer
Intercept the module root to patch transaction
aa7279a 
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.
matthewloring
matthewloring suggested changes May 17, 2017
View reviewed changes
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.

Sign in to view

This comment was marked as spam.

Sign in to view

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.

Sign in to view

This comment was marked as spam.

Sign in to view

matthewloring
matthewloring approved these changes May 23, 2017
View reviewed changes
Copy link
Contributor

@matthewloring matthewloring left a 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.

Sign in to view

This comment was marked as spam.

Sign in to view

@DominicKramer DominicKramer merged commit dd7bc9b into googleapis:master May 23, 2017
vmarchaud pushed a commit to keymetrics/trassingue that referenced this pull request Jun 1, 2017
@DominicKramer @vmarchaud
Support Knex (#468)
b5c7603 
This commit includes support and tests for knex satsifying `>=0.10 <=0.13`.

PR-URL: googleapis/cloud-trace-nodejs#468
@matthewloring matthewloring mentioned this pull request Jun 7, 2017
Closed
yoshi-automation added a commit that referenced this pull request Apr 7, 2020
@yoshi-automation
fix: apache license URL (#468)
933f429 
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)
gcf-merge-on-green bot pushed a commit that referenced this pull request Apr 7, 2020
@yoshi-automation
fix: apache license URL (#468) (#1232)
ac7e886 
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
Mistic92 pushed a commit to Mistic92/cloud-trace-nodejs that referenced this pull request Apr 15, 2020
@yoshi-automation @Mistic92
fix: apache license URL (googleapis#468) (googleapis#1232)
6ea2bae 
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
@release-please release-please bot mentioned this pull request Jun 30, 2022
Merged
@WontonSam WontonSam mentioned this pull request Sep 19, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewloring matthewloring matthewloring approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DominicKramer @matthewloring @googlebot