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.

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

fix(auth): pass project id to gax clients #447

Merged
merged 3 commits into from
Jan 26, 2019
Merged

fix(auth): pass project id to gax clients #447

merged 3 commits into from
Jan 26, 2019

Conversation

callmehiphop
Copy link
Contributor

@callmehiphop callmehiphop commented Jan 25, 2019

Fixes #288
Fixes #445

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Looks like not passing the project ID into the gapic clients causes a bunch of future getProjectId calls which makes CPU usage spike pretty high

 [Summary]:
   ticks  total  nonlib   name
   3380   67.2%   71.3%  JavaScript

 [Bottom up (heavy) profile]:
   ticks parent  name
    922   18.3%  LazyCompile: *auth.getProjectId /Users/taco/projects/temp/node_modules/@google-cloud/pubsub/build/src/index.js:599:36
    921   99.9%    LazyCompile: *getProjectIdAsync.then.r /Users/taco/projects/temp/node_modules/google-auth-library/build/src/auth/googleauth.js:73:43
    921  100.0%      T node::RunMicrotasks(v8::FunctionCallbackInfo<v8::Value> const&)
    921  100.0%        LazyCompile: ~_tickCallback internal/process/next_tick.js:41:25

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 25, 2019
@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 25, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 25, 2019
@stephenplusplus
Copy link
Contributor

Do the GAPIC clients just need to cache the project ID that they fetch? In any case, this PR is correct-- we should provide to the GAPIC what the user provides, which could be different from what the environment detects. Just curious if there's a simple optimization that would make GAPIC perform better.

@callmehiphop
Copy link
Contributor Author

callmehiphop commented Jan 26, 2019

@stephenplusplus The auth client should cache the project ID (at least it looks like it does). When a gapic client is created it makes an auth.getClient call for every service stub it creates. I haven't looked too deep into this, but it seems like we might be hitting the auth client with concurrent requests for credentials, which might explain the spike in CPU usage. If I had to guess, I think caching the auth client would help. Alternatively I think we could also pass our auth client to the gapic client, at which point the credentials would already be cached.

@callmehiphop callmehiphop merged commit 1f2b586 into googleapis:master Jan 26, 2019
@callmehiphop callmehiphop deleted the dg--288 branch January 26, 2019 06:00
praveenqlogic pushed a commit to praveenqlogic/nodejs-pubsub that referenced this pull request Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants