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 for gRPC time tracing #267

Merged
merged 1 commit into from
Jun 28, 2016
Merged

Support for gRPC time tracing #267

merged 1 commit into from
Jun 28, 2016

Conversation

misterpoe
Copy link
Contributor

No description provided.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Jun 23, 2016
@misterpoe
Copy link
Contributor Author

misterpoe commented Jun 23, 2016 •
edited
Loading

@matthewloring @cristiancavalli @ofrobots PTAL

@misterpoe
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 23, 2016
matthewloring
matthewloring reviewed Jun 23, 2016
View reviewed changes
lib/hooks/userspace/hook-grpc.js
},
unpatch: function(extension) {
shimmer.unwrap(extension.Call.prototype, 'startBatch');
shimmer.unwrap(client, 'makeClientConstructor');

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@misterpoe misterpoe force-pushed the grpc branch 3 times, most recently from dc85462 to 03606d5 Compare June 23, 2016 21:20
matthewloring
matthewloring reviewed Jun 23, 2016
View reviewed changes
lib/hooks/userspace/hook-grpc.js
// TODO: maybe we only want to do this if a root context exists.
return startBatch.call(this, thing, cls.getNamespace().bind(callback));
function wrapClientMethod(name, method, class_options) {
return function() {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@misterpoe misterpoe force-pushed the grpc branch 3 times, most recently from 22de6a4 to b7f9aff Compare June 23, 2016 22:06
matthewloring
matthewloring reviewed Jun 23, 2016
View reviewed changes
lib/hooks/userspace/hook-grpc.js
} else {
cbIndex = arguments.length - 1;
}
if (cbIndex >= 0 && cbIndex < arguments.length) {

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

@matthewloring
Copy link
Contributor

Everything looks good on my end. Since this is a bigger code review lets get at least one other LGTM before landing this.

ofrobots
ofrobots reviewed Jun 24, 2016
View reviewed changes
lib/hooks/userspace/hook-grpc.js
return function startBatchTrace(thing, callback) {
// TODO: maybe we only want to do this if a root context exists.
return startBatch.call(this, thing, cls.getNamespace().bind(callback));
function clientMethodWrap(name, method, class_options) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@ofrobots
Copy link
Contributor

@murgatroid99 FYI. This is how we are starting to instrument grpc so that we get latency traces for Stackdriver trace. PTAL. A cleaner approach (for future gRPC versions) would be if you could give us an API to gather the data we need around RPCs.

@misterpoe misterpoe force-pushed the grpc branch 3 times, most recently from 8d30151 to 5116d80 Compare June 24, 2016 22:43
@matthewloring
Copy link
Contributor

LGTM. Did you get a chance to verify that this traces the gcloud module?

ofrobots
ofrobots reviewed Jun 27, 2016
View reviewed changes
lib/hooks/userspace/hook-grpc.js
unpatch: function(client) {
// Note that already wrapped client methods will not be unwrapped here.
// Only the client constructor is unwrapped, so that future grpc.load's
// will not wrap client methods with tracing.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

ofrobots
ofrobots reviewed Jun 27, 2016
View reviewed changes
lib/hooks/userspace/hook-grpc.js
makeClientConstructor) {
return function makeClientConstructorTrace(methods) {
var Client = makeClientConstructor.apply(this, arguments);
// If version < 0.14, we need to use the old argument order.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@misterpoe
Copy link
Contributor Author

@matthewloring I have tested this on one of the example Bookshelf apps and it shows tracing of gRPC calls when I require('@google/cloud-trace').

@ofrobots
Copy link
Contributor

LGTM

@matthewloring matthewloring merged commit 693b6ae into googleapis:master Jun 28, 2016
@misterpoe misterpoe deleted the grpc branch June 28, 2016 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
4 participants
@misterpoe @googlebot @matthewloring @ofrobots