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

Context propagation for google-gax #404

Merged
merged 1 commit into from Mar 7, 2017
Merged

Context propagation for google-gax #404

merged 1 commit into from Mar 7, 2017

Conversation

matthewloring
Copy link
Contributor

This should allow tracing of pubsub, vision, monitoring, spanner,
speech, and nlp.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 20, 2017
kjin
kjin approved these changes Mar 2, 2017
View reviewed changes
Copy link
Contributor

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

@matthewloring
Copy link
Contributor Author

@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.

@matthewloring
Copy link
Contributor Author

/cc @jmuk

@jmuk
Copy link

jmuk commented Mar 6, 2017

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 funcWithAuth promise result, which could destroy the callstack unfortunately.

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.

kjin
kjin reviewed Mar 6, 2017
View reviewed changes
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.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Context propagation for google-gax
edb63eb 
This should allow tracing of pubsub, vision, monitoring, spanner,
speech, and nlp.
@matthewloring
Copy link
Contributor Author

@jmuk I believe there may be opportunities to refactor gax and simplify things here but I'll have to dig a little further.

@matthewloring matthewloring merged commit dc41a8b into googleapis:master Mar 7, 2017
@matthewloring matthewloring deleted the speech branch March 7, 2017 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjin kjin kjin 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.

None yet
4 participants
@matthewloring @jmuk @kjin @googlebot