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: add metric headers #1503

Merged
merged 27 commits into from
Oct 2, 2024
Merged

feat: add metric headers #1503

merged 27 commits into from
Oct 2, 2024

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented Sep 13, 2024

context: b/339259830 and go/send-auth-metrics-java

Changes include:

  • expose Credentials type via getMetricsCredentialType(). Override this method for UserCredentials, ServiceAccountCredentials, ImpersonatedCredentials, and ComputeEngineCredentials. This is used in both token request and token usage flows.
  • add metric headers for each of the in-scope token requests. Below are examples of each request flow with added metrics:
    • User credentials request (at/id): “gl-java/19.0.1 auth/1.24.3 cred-type/u”
    • SA credentials, VM credentials or Impersonated credentials requests (at/id): “gl-java/19.0.1 auth/1.24.3 auth-request-type/at cred-type/sa”
    • MDS ping (This is used in ADC during the credential detection): “gl-java/19.0.1 auth/1.24.3 auth-request-type/mds”
  • What is not tracked: ComputeEngineCredentials getUniverseDomain and getAccount does not send metrics header; TPC flows does not send metrics headers.

Related pr: adding for cred_type for token usage requests
googleapis/sdk-platform-java#3186

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Sep 13, 2024
@zhumin8 zhumin8 marked this pull request as ready for review September 25, 2024 18:08
@zhumin8 zhumin8 requested review from a team as code owners September 25, 2024 18:08

package com.google.auth;

public enum CredentialType {

Choose a reason for hiding this comment

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

Since these are strictly defined by our metrics implementation, can we make that more clear everywhere?

For instance we might call this "CredentialNameForMetrics" (or something better than that).

That way we know that we can't add enum items without a definition that is established elsewhere, and there won't be Credentials that have a CredentialType of Unknown (when we actually do know the type, we just don't want to score it for metrics.)

@zhumin8 zhumin8 requested a review from lqiu96 October 1, 2024 17:22
assertTrue(headers.containsKey(MetricsUtils.API_CLIENT_HEADER));
String actualMetricsValue = headers.get(MetricsUtils.API_CLIENT_HEADER).get(0);
String expectedMetricsValue;
if (requestType.equals("untracked")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Can we use the enum value here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

note requestType and credentialType here are String values. They represent part of the expected value in this utils method. I am leaning towards checking against plain text here

MetricsUtils.getLanguageAndAuthLibraryVersions(),
MetricsUtils.CRED_TYPE,
credentialType);
} else if (credentialType.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isEmpty() and not check for DO NOT SEND?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updating to be consistent with requestType above.

Copy link
Contributor

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

LGTM. Added two questions in the tests. Thanks for addressing the comments and questions!

Copy link

sonarcloud bot commented Oct 2, 2024

@zhumin8 zhumin8 merged commit 7f0c1d3 into main Oct 2, 2024
15 of 18 checks passed
@zhumin8 zhumin8 deleted the metrics branch October 2, 2024 15:04
zhumin8 added a commit to googleapis/sdk-platform-java that referenced this pull request Oct 4, 2024
This is POC change in gax-java for auth metrics requirements on token
usage. See go/googleapis-auth-metric-design for context.


[Credentials](https://github.com/googleapis/google-auth-library-java/blob/main/credentials/java/com/google/auth/Credentials.java)
will expose `getMetricsCredentialType()` method, this change appends it
to existing `x-goog-api-client` header

Note: Currently implement in gax at client level. There are 2 edge cases
not covered and will create followups for: if handwritten library
overrides credentials at rpc level; If handwritten library does not
build on gax. (ref: b/370039645, b/370038458)

related change in
`google-auth-library`googleapis/google-auth-library-java#1503
included in 1.28.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants