FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(gax): append cred-type header for auth metrics by zhumin8 · Pull Request #3186 · googleapis/sdk-platform-java · GitHub

This repository was archived by the owner on May 14, 2026. It is now read-only.
/ sdk-platform-java Public archive

feat(gax): append cred-type header for auth metrics #3186

Merged
zhumin8 merged 14 commits into from
Oct 4, 2024
Conversation 15 Commits 14 Checks 0 Files changed
Merged

feat(gax): append cred-type header for auth metrics #3186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2dbd2d8
feat: expose credential type as part of x-goog-api-client header.
zhumin8 Sep 3, 2024
ee2624c
test: add test correctly appends cred-type to header.
zhumin8 Sep 10, 2024
fe3ed54
use enum for credential types.
zhumin8 Sep 13, 2024
0cd0ad8
update type.
zhumin8 Oct 1, 2024
b1dd5b0
fix naming and comment.
zhumin8 Oct 2, 2024
781f161
Merge branch 'main' into auth-metrics
zhumin8 Oct 2, 2024
01adbde
Merge branch 'main' into auth-metrics
zhumin8 Oct 3, 2024
32f8934
fix credential is null case.
zhumin8 Oct 3, 2024
51bed23
add test case.
zhumin8 Oct 3, 2024
0424fd2
update test, extract test setup steps.
zhumin8 Oct 3, 2024
0e69f72
Merge branch 'main' into auth-metrics
zhumin8 Oct 4, 2024
b50896a
address feedback: encapsulate credentials metrics logic inside getHea…
zhumin8 Oct 4, 2024
6e60633
address feedback.
zhumin8 Oct 4, 2024
33e24d9
update comment.
zhumin8 Oct 4, 2024
File filter

Filter by extension

Filter by extension .java  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
23 changes: 21 additions & 2 deletions gax-java/gax/src/main/java/com/google/api/gax/rpc/ClientContext.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import com.google.api.gax.tracing.ApiTracerFactory;
import com.google.api.gax.tracing.BaseApiTracerFactory;
import com.google.auth.ApiKeyCredentials;
import com.google.auth.CredentialTypeForMetrics;
import com.google.auth.Credentials;
import com.google.auth.oauth2.GdchCredentials;
import com.google.auto.value.AutoValue;
Expand Down Expand Up @@ -210,7 +211,8 @@ public static ClientContext create(StubSettings settings) throws IOException {
if (transportChannelProvider.needsExecutor() && settings.getExecutorProvider() != null) {
transportChannelProvider = transportChannelProvider.withExecutor(backgroundExecutor);
}
Map<String, String> headers = getHeadersFromSettings(settings);

Map<String, String> headers = getHeaders(settings, credentials);
if (transportChannelProvider.needsHeaders()) {
transportChannelProvider = transportChannelProvider.withHeaders(headers);
}
Expand Down Expand Up @@ -318,8 +320,11 @@ static GdchCredentials getGdchCredentials(
/**
* Getting a header map from HeaderProvider and InternalHeaderProvider from settings with Quota
* Project Id.
*
* <p>Then if credentials is present and its type for metrics is not {@code
* CredentialTypeForMetrics.DO_NOT_SEND}, append this type info to x-goog-api-client header.
*/
private static Map<String, String> getHeadersFromSettings(StubSettings settings) {
private static Map<String, String> getHeaders(StubSettings settings, Credentials credentials) {
// Resolve conflicts when merging headers from multiple sources
Map<String, String> userHeaders = settings.getHeaderProvider().getHeaders();
Map<String, String> internalHeaders = settings.getInternalHeaderProvider().getHeaders();
Expand All @@ -346,6 +351,20 @@ private static Map<String, String> getHeadersFromSettings(StubSettings settings)
effectiveHeaders.putAll(userHeaders);
effectiveHeaders.putAll(conflictResolution);

return appendCredentialTypeToHeaderIfPresent(effectiveHeaders, credentials);
}

private static Map<String, String> appendCredentialTypeToHeaderIfPresent(
Map<String, String> effectiveHeaders, Credentials credentials) {
CredentialTypeForMetrics credentialTypeForMetrics =

blakeli0 Oct 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit: extract this to a private method for better readability.
credentials == null
? CredentialTypeForMetrics.DO_NOT_SEND
: credentials.getMetricsCredentialType();
if (credentialTypeForMetrics != CredentialTypeForMetrics.DO_NOT_SEND) {
effectiveHeaders.computeIfPresent(
ApiClientHeaderProvider.getDefaultApiClientHeaderKey(),

blakeli0 Oct 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it decided to append cred-type/ to x-goog-api-client header? It seems all other header tokens in x-goog-api-client are static values from the client, not from user input. I wonder why we didn't choose to use a separate header key for this purpose?

zhumin8 Oct 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it decided to append cred-type/ to x-goog-api-client header?

yes, this design is already implemented for other some languages. See details in design go/googleapis-auth-metric-design section 2.3.2. I don't think I saw discussion on using a different header specifically though.

It seems all other header tokens in x-goog-api-client are static values from the client, not from user input.

IIUC, this header is eventually sent per request, at that time, I don't see credentials type much different than other client lib info? Do you have specific concerns about this?

blakeli0 Oct 4, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The concern is that x-goog-api-client was constructed in the generated layer only with static values, the name of the header api-client also seems to indicate that the header is supposed to contain only info for the client, not for requests.
In terms of the Java implementations, we now also have multiple places modifying the x-goog-api-client value, in both ClientContext and ApiClientHeaderProvider, which make this header harder to maintain in the future and also more error prone.
That being said, it is probably too late to change the overall design. We just need to be ware of the consequences and add more docs if possible.

zhumin8 Oct 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I see your point.
Let me know if you see better suited places for additional docs.
(key, value) -> value + " cred-type/" + credentialTypeForMetrics.getLabel());
}
return ImmutableMap.copyOf(effectiveHeaders);
}

Expand Down
82 changes: 82 additions & 0 deletions gax-java/gax/src/test/java/com/google/api/gax/rpc/ClientContextTest.java
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import com.google.api.core.ApiClock;
import com.google.api.gax.core.BackgroundResource;
Expand All @@ -53,6 +54,7 @@
import com.google.api.gax.rpc.testing.FakeStubSettings;
import com.google.api.gax.rpc.testing.FakeTransportChannel;
import com.google.auth.ApiKeyCredentials;
import com.google.auth.CredentialTypeForMetrics;
import com.google.auth.Credentials;
import com.google.auth.oauth2.ComputeEngineCredentials;
import com.google.auth.oauth2.GdchCredentials;
Expand Down Expand Up @@ -677,6 +679,86 @@ void testUserAgentConcat() throws Exception {
.containsEntry("user-agent", "user-supplied-agent internal-agent");
}

@Test
void testApiClientHeaderAppendsCredType() throws Exception {
GoogleCredentials googleCredentials = Mockito.mock(GoogleCredentials.class);
when(googleCredentials.getMetricsCredentialType())
.thenReturn(CredentialTypeForMetrics.USER_CREDENTIALS);

Map<String, String> headers =
setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
FixedCredentialsProvider.create(googleCredentials),
FixedHeaderProvider.create("x-goog-api-client", "internal-agent"));

assertThat(headers).containsEntry("x-goog-api-client", "internal-agent cred-type/u");
}

@Test
void testApiClientHeaderDoNotAppendsCredType_whenNoApiClientHeader() throws Exception {
GoogleCredentials googleCredentials = Mockito.mock(GoogleCredentials.class);
when(googleCredentials.getMetricsCredentialType())
.thenReturn(CredentialTypeForMetrics.USER_CREDENTIALS);

Map<String, String> headers =
setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
FixedCredentialsProvider.create(googleCredentials),
FixedHeaderProvider.create("some-other-header", "internal-agent"));

assertThat(headers).doesNotContainKey("x-goog-api-client");
assertThat(headers).containsEntry("some-other-header", "internal-agent");
}

@Test
void testApiClientHeaderDoNotAppendsCredType_whenNullCredentials() throws IOException {
Map<String, String> headers =
setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
NoCredentialsProvider.create(),
FixedHeaderProvider.create("x-goog-api-client", "internal-agent"));

assertThat(headers).containsKey("x-goog-api-client");
assertThat(headers.get("x-goog-api-client")).doesNotContain("cred-type/");
}

@Test
void testApiClientHeaderDoNotAppendsCredType_whenCredTypeDoNotSend() throws Exception {
GoogleCredentials googleCredentials = Mockito.mock(GoogleCredentials.class);
when(googleCredentials.getMetricsCredentialType())
.thenReturn(CredentialTypeForMetrics.DO_NOT_SEND);

Map<String, String> headers =
setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
FixedCredentialsProvider.create(googleCredentials),
FixedHeaderProvider.create("x-goog-api-client", "internal-agent"));

assertThat(headers).containsKey("x-goog-api-client");
assertThat(headers.get("x-goog-api-client")).doesNotContain("cred-type/");
}

private Map<String, String> setupTestForCredentialTokenUsageMetricsAndGetTransportChannelHeaders(
CredentialsProvider credentialsProvider, HeaderProvider headerProvider) throws IOException {
TransportChannelProvider transportChannelProvider =
new FakeTransportProvider(
FakeTransportChannel.create(new FakeChannel()),
null,
true,
null,
null,
DEFAULT_ENDPOINT);

ClientSettings.Builder builder =
new FakeClientSettings.Builder()
.setExecutorProvider(
FixedExecutorProvider.create(Mockito.mock(ScheduledExecutorService.class)))
.setTransportChannelProvider(transportChannelProvider)
.setCredentialsProvider(credentialsProvider)
.setInternalHeaderProvider(headerProvider);

ClientContext clientContext = ClientContext.create(builder.build());
FakeTransportChannel transportChannel =
(FakeTransportChannel) clientContext.getTransportChannel();
return transportChannel.getHeaders();
}

private static String endpoint = "https://foo.googleapis.com";
private static String mtlsEndpoint = "https://foo.mtls.googleapis.com";

Expand Down
Back | FazBrowse Home | New Git URL