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

feat: enable setting quota_project_id #1128

Merged
merged 12 commits into from Jul 24, 2020
Merged

feat: enable setting quota_project_id #1128

merged 12 commits into from Jul 24, 2020

Conversation

summer-ji-eng
Copy link
Contributor

@summer-ji-eng summer-ji-eng commented Jun 15, 2020

As part of extensible client options, enable quota_project_id in GAPIC. So that client is able to set quota project ID, like "Settings.NewBuilder().SetQuotaProjectID(String)"

A quota project id might set up from 4 ways:

  1. set up from setting directly: `settings.newBuilder().SetQuotaProjectID
  2. set up from header provider
  3. set up from internal header provider
  4. set up from credential

Open Question:
1st priority of getting value is from SetQuotaProjectID, if it exists, would overwrite all value set from other ways.
So, what is the priority of other 3 ways, if SetQuotaProjectID is absent.
The current implementation is based on the order of calling the method. Let me know if you have any other preference. @chingor13

Integration Test: Tested in local java-TextToSpeech repo and using the local snapshot, test passes.

@Test
  public void quotaProjectIDIntegrationTest() throws IOException {
    final String QUOTA_PROJECT_ID_KEY = "x-google-user-project";
    final String QUOTA_PROJECT_ID = "test_quota_project_id";
    final String QUOTA_PORJECT_ID_HEADER_PROVIDER = "test_quota_project_id_from_header_provider";

    final HeaderProvider headerProvider = new HeaderProvider() {
      @Override
      public Map<String, String> getHeaders() {
        return Collections.singletonMap(QUOTA_PROJECT_ID_KEY, QUOTA_PORJECT_ID_HEADER_PROVIDER);
      }
    };

    TextToSpeechSettings settingsWithQuotaOnly = TextToSpeechSettings.newBuilder()
        .setTransportChannelProvider(channelProvider)
        .setCredentialsProvider(NoCredentialsProvider.create())
        .setQuotaProjectID(QUOTA_PROJECT_ID)
        .build();
    TextToSpeechClient clientWithQuotaOnly = TextToSpeechClient.create(settingsWithQuotaOnly);

    TextToSpeechSettings settingsWithAllSource = TextToSpeechSettings.newBuilder()
        .setTransportChannelProvider(channelProvider)
        .setHeaderProvider(headerProvider)
        .setQuotaProjectID(QUOTA_PROJECT_ID)
        .build();
    TextToSpeechClient clientWithAllSource = TextToSpeechClient.create(settingsWithAllSource);

    Assert.assertEquals(clientWithQuotaOnly.getSettings().getQuotaProjectID(), QUOTA_PROJECT_ID);
    Assert.assertEquals(clientWithAllSource.getSettings().getHeaderProvider().getHeaders().get(QUOTA_PROJECT_ID_KEY), QUOTA_PORJECT_ID_HEADER_PROVIDER);
    Assert.assertEquals(clientWithQuotaOnly.getSettings().getQuotaProjectID(), QUOTA_PROJECT_ID);
  }

@googlebot googlebot added the cla: yes label Jun 15, 2020
@codecov
Copy link

@codecov codecov bot commented Jun 15, 2020

Codecov Report

Merging #1128 into master will increase coverage by 0.41%.
The diff coverage is 92.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1128      +/-   ##
============================================
+ Coverage     78.65%   79.06%   +0.41%     
- Complexity     1170     1193      +23     
============================================
  Files           204      205       +1     
  Lines          5195     5264      +69     
  Branches        417      434      +17     
============================================
+ Hits           4086     4162      +76     
+ Misses          935      930       -5     
+ Partials        174      172       -2     
Impacted Files Coverage Δ Complexity Δ
...main/java/com/google/api/gax/rpc/StubSettings.java 75.32% <80.64%> (+1.34%) 12.00 <1.00> (+1.00)
...ain/java/com/google/api/gax/rpc/ClientContext.java 92.85% <100.00%> (+11.97%) 19.00 <6.00> (+10.00)
...in/java/com/google/api/gax/rpc/ClientSettings.java 93.75% <100.00%> (+0.50%) 13.00 <1.00> (+1.00)
.../rpc/internal/QuotaProjectIdHidingCredentials.java 100.00% <100.00%> (ø) 9.00 <9.00> (?)
...le/api/gax/core/InstantiatingExecutorProvider.java 100.00% <0.00%> (+37.50%) 5.00% <0.00%> (+2.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ce0c30...7f91763. Read the comment docs.

this.streamWatchdogProvider = settings.streamWatchdogProvider;
this.streamWatchdogCheckInterval = settings.streamWatchdogCheckInterval;
this.tracerFactory = settings.tracerFactory;
}

/** Get Quota Project ID from Client Context * */
private static String getQuotaProjectIDFromClientContext(ClientContext clientContext) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we use ID in methods and variables, and Id in class names. Is this idiomatic?

Copy link
Contributor Author

@summer-ji-eng summer-ji-eng Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ID used in all methods and variables, which is really just my preference. I haven't find any guideline for naming.
Let me know if any rule need to be follow here, I could make change.

Copy link
Contributor

@chingor13 chingor13 Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@summer-ji-eng summer-ji-eng Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make change to quotaProjectId

if (clientContext.getHeaders().containsKey(QUOTA_PROJECT_ID_HEADER_KEY)) {
return clientContext.getHeaders().get(QUOTA_PROJECT_ID_HEADER_KEY);
}
if (clientContext.getInternalHeaders().containsKey(QUOTA_PROJECT_ID_HEADER_KEY)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between headers and internal headers? And is it possible for quota project to come in on either one?

Also, what is the use case for pulling it from the header? I think maybe it is not necessary to detect this (@broady ?)

Copy link
Contributor Author

@summer-ji-eng summer-ji-eng Jun 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quota project id can be gotten from header and internal header. double confirm with @chingor13

but I am not sure the use case. @broady

Copy link
Contributor

@chingor13 chingor13 Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the internal header provider is reserved for gapic and handwritten layers to inject headers. The general header provider is to allow library users to specify arbitrary extra headers.

It's not desired for library users to set the quota project via header provider, but previously that was the only mechanism for doing so. We should have an explicit strategy for how to handle that case.

Copy link
Contributor Author

@summer-ji-eng summer-ji-eng Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems ok to support this then, for backwards compatibility. And also to ensure any quota project behavior stays consistent regardless of how it's set.

Copy link
Contributor

@chingor13 chingor13 left a comment

Maybe it's just me, but I'm not seeing how the quota project id when configured from ClientSettings is actually making it into the request or connection.

@summer-ji-eng
Copy link
Contributor Author

@summer-ji-eng summer-ji-eng commented Jun 18, 2020

Maybe it's just me, but I'm not seeing how the quota project id when configured from ClientSettings is actually making it into the request or connection.

Can I have more detail about how do you use configure from ClientSetting. Try to reproduce on my side.
BTW, googleapis/java-texttospeech#176 is the draft PR to test using ClientSettings

@chingor13
Copy link
Contributor

@chingor13 chingor13 commented Jun 18, 2020

Can I have more detail about how do you use configure from ClientSetting. Try to reproduce on my side.

Just the direct configuration method:

TextToSpeechSettings settingsWithAllSource = TextToSpeechSettings.newBuilder()
        .setQuotaProjectId(QUOTA_PROJECT_ID)
        .build();

Reading the code, I see how the quotaProjectId instance variable is populated on ClientContext, but not how that actually makes it to the gRPC connection or request as a header.

For example, the credentials authorization header is supplied by providing the credentials to the ApiCallContext which is used for setting up the channel and all options.

@summer-ji-eng
Copy link
Contributor Author

@summer-ji-eng summer-ji-eng commented Jun 30, 2020

@chingor13 @bshaffer
For integration testing, I used gapic-generator generate a showcase[schema] test client library. And use local gax-java SNAPSHOP(code) to test SetQuotaProjectID feature. It passed all the tests. I push the client into my personal repo(please feel free to suggest if you have you other recommend solution). Here is the testing code.

@chingor13
Copy link
Contributor

@chingor13 chingor13 commented Jul 6, 2020

I'm still concerned that the headers are not actually configured for the TransportProvider or the CallContext. Somewhere in

if (transportChannelProvider.needsHeaders()) {
transportChannelProvider = transportChannelProvider.withHeaders(headers);
}
if (transportChannelProvider.needsEndpoint()) {
transportChannelProvider = transportChannelProvider.withEndpoint(settings.getEndpoint());
}
if (transportChannelProvider.needsCredentials() && credentials != null) {
transportChannelProvider = transportChannelProvider.withCredentials(credentials);
}
TransportChannel transportChannel = transportChannelProvider.getTransportChannel();
ApiCallContext defaultCallContext =
transportChannel.getEmptyCallContext().withTransportChannel(transportChannel);
if (credentials != null) {
defaultCallContext = defaultCallContext.withCredentials(credentials);
}
I'd expect to see how we're telling gRPC to set the x-goog-user-project header.

The testing code in the generated showcase client is inspecting the settings object (which is correct), but it's not ensuring that the header is being set on the gRPC request.

@@ -247,6 +287,10 @@ public B setCredentialsProvider(CredentialsProvider credentialsProvider) {
@BetaApi("The surface for customizing headers is not stable yet and may change in the future.")
public B setHeaderProvider(HeaderProvider headerProvider) {
this.headerProvider = headerProvider;
if (this.quotaProjectId == null

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chingor13 is it necessary to support setting quota project automatically through the incoming headers? This is not something I am familiar with, or something done in any other languages AFAIK. It also makes the precedent of these operations less intuitive.

Copy link
Contributor

@chingor13 chingor13 left a comment

I think what we want to do:

In ClientContext.create(), we want to add "x-goog-user-project" => settings.getQuotaProjectId() to the Map<String, String> headers that we provide to the transportChannelProvider. This will populate the x-goog-user-project on all the requests using that transport.

If a quotaProjectId is set, we also need to tell the Credentials to no longer send the x-goog-user-project header (it could be an incorrect value as the explicitly configured quotaProjectId setting should take precedence over the ones provided by the credentials). This will be harder and likely we'll have to do something ugly like providing our a wrapper Credentials implementation:

static class QuotaProjectHidingCredentials implements Credentials {
  private Credentials wrappedCredentials;
  QuotaProjectHidingCredentials(Credentials wrappedCredentials) {
    this.wrappedCredentials = wrappedCredentials;
  }

  public Map<String,List<String>> getRequestMetadata(URI uri) {
    Map<String,List<String>> headers = wrappedCredentials.getRequestMetadata(URI uri);
    // return new map without "x-goog-user-project" header
  }

  // ... other abstract methods delegate to the wrapped credentials
}

@summer-ji-eng summer-ji-eng requested a review from chingor13 Jul 17, 2020
build.gradle Outdated
@@ -24,7 +24,7 @@ apply plugin: 'com.github.sherter.google-java-format'
apply plugin: 'io.codearte.nexus-staging'

// TODO: Populate this from dependencies.properties version property (for proper Gradle-Bazel sync)
project.version = "1.57.1" // {x-version-update:gax:current}
project.version = "1.57.2" // {x-version-update:gax:current}
Copy link
Contributor Author

@summer-ji-eng summer-ji-eng Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chingor13 not sure if I should upgrade a version. let me know what you think. Thanks

@summer-ji-eng summer-ji-eng requested a review from bshaffer Jul 17, 2020
Copy link

@bshaffer bshaffer left a comment

One question for clarification, and a minor request. Otherwise LGTM!

if (clientContext.getHeaders().containsKey(QUOTA_PROJECT_ID_HEADER_KEY)) {
return clientContext.getHeaders().get(QUOTA_PROJECT_ID_HEADER_KEY);
}
if (clientContext.getInternalHeaders().containsKey(QUOTA_PROJECT_ID_HEADER_KEY)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems ok to support this then, for backwards compatibility. And also to ensure any quota project behavior stays consistent regardless of how it's set.

@summer-ji-eng summer-ji-eng requested a review from chingor13 Jul 22, 2020
Copy link
Contributor

@chingor13 chingor13 left a comment

One more header to fix, then LGTM

…IdHidingCredentialsTest.java

Co-authored-by: Jeff Ching <chingor@google.com>
@summer-ji-eng summer-ji-eng merged commit 20bb200 into master Jul 24, 2020
9 checks passed
@summer-ji-eng summer-ji-eng deleted the quota_project branch Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants