-
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 for gRPC time tracing #267
Conversation
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! |
CLAs look good, thanks! |
}, | ||
unpatch: function(extension) { | ||
shimmer.unwrap(extension.Call.prototype, 'startBatch'); | ||
shimmer.unwrap(client, 'makeClientConstructor'); |
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.
dc85462
to
03606d5
Compare
// 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.
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.
22de6a4
to
b7f9aff
Compare
} else { | ||
cbIndex = arguments.length - 1; | ||
} | ||
if (cbIndex >= 0 && cbIndex < arguments.length) { |
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.
Everything looks good on my end. Since this is a bigger code review lets get at least one other LGTM before landing this. |
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.
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.
@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. |
8d30151
to
5116d80
Compare
LGTM. Did you get a chance to verify that this traces the |
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.
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.
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.
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.
@matthewloring I have tested this on one of the example Bookshelf apps and it shows tracing of gRPC calls when I |
LGTM |
No description provided.