-
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
Context propagation for google-gax #404
Conversation
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. There is an untraced HTTP auth request from gcloud, but we can track that separately (it's an issue that surfaced as a result from but is independent of this plugin).
@kjin Did you get a chance to look through the gax implementation while reviewing this? There are usually many possible patches that could be applied to preserve context. I want to make sure this approach makes the most sense in addition to solving the problem. |
/cc @jmuk |
I am not familiar with how the wrapping works, but patching like this would be great. GAX also has a logic of retrying or bundling inside of the I am also wondering how refactoring gax could save this situation (in other words, what types of changes on google-gax side can simplify the patch like this). Let me know if you have any proposals. |
src/plugins/plugin-google-gax.js
Outdated
}; | ||
var apiCallInner = createApiCall.call(this, funcWithAuth, settings, optDescriptor); | ||
return function apiCallInner_trace(request, callOptions, callback) { | ||
return apiCallInner.call(this, request, callOptions, api.wrap(callback)); |
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.
This should allow tracing of pubsub, vision, monitoring, spanner, speech, and nlp.
@jmuk I believe there may be opportunities to refactor gax and simplify things here but I'll have to dig a little further. |
This should allow tracing of pubsub, vision, monitoring, spanner,
speech, and nlp.