feat: add client logging with slf4j #1586

zhumin8 · 2024-12-05T15:41:16Z

This is a draft pr for preview changes for adding client logging capability to auth.
see go/java-client-logging-design

Very limited testing added at this point.

changes includes:

add slf4j dependency, introduce LoggingConfigs to enable logging only when GOOGLE_SDK_JAVA_LOGGING=true
logging implementation depends on users binding added. If no binding, default to no-op.
introduce LoggingUtils for helper method for logging. Add log for request and response made to auth endpoints for UserCredentials, ServiceAccountCredentials, ComputeEngineCredentials, ImpersonatedCredentials. For token values included, added hash in log.

Here is an example logging output for access token request from UserCredentials when DEBUG level is allowed.

{"@timestamp":"2024-12-04T21:10:48.596382834-05:00","@version":"1","message":"Sending auth request to refresh access token.","logger_name":"com.google.auth.oauth2.UserCredentials","thread_name":"main","severity":"DEBUG","level_value":10000,"request.method":"POST","request.headers":"{x-goog-api-client=gl-java/19.0.1 auth/${project.parent.version} cred-type/u, accept-encoding=[gzip]}","request.url":"https://foo.com/bar","request.payload":"{refresh_token=1/Tl6*****yGWY, grant_type=refresh_token, client_secret=jakua*****wZcu, client_id=ya29.1.AADtN_UtlxN3PuGAxrN2XQnZTVRvDyVWnYq4I6dws}"}
{"@timestamp":"2024-12-04T21:10:48.624914522-05:00","@version":"1","message":"Received auth respond for refresh access token.","logger_name":"com.google.auth.oauth2.UserCredentials","thread_name":"main","severity":"INFO","level_value":20000,"response.status":"200","response.headers":"{}"}
{"@timestamp":"2024-12-04T21:10:48.627483862-05:00","@version":"1","message":"Auth response payload.","logger_name":"com.google.auth.oauth2.UserCredentials","thread_name":"main","severity":"DEBUG","level_value":10000,"access_token":"1/MkS*****KPY2","refresh_token":"1/Tl6*****yGWY","token_type":"Bearer","expires_in":"3600"}

Update Dec 9: I added basic test coverage, not showcasing all the logging messages details. Will add that as followup task.

…esp to auth endpoints.

…e-plugin.

zhumin8 · 2024-12-05T21:39:45Z

@lqiu96 I still have some trouble with the test env, but wanted to put it out there so you can get a preview of the changes. Can you take a quick look and let me know if you have any concerns.

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

lqiu96 · 2024-12-06T01:42:08Z

oauth2_http/java/com/google/auth/oauth2/IamUtils.java

+    LoggingUtils.logRequest(request, LOGGER, "auth sending request to get signature.");
    HttpResponse response = request.execute();
+    LoggingUtils.logResponse(response, LOGGER, "auth received response for signature.");


nit: Could the message to be more descriptive? i.e. Signature to sign the blob or something that better describes this

oauth2_http/java/com/google/auth/oauth2/IamUtils.java

oauth2_http/java/com/google/auth/oauth2/LoggingConfigs.java

lqiu96 · 2024-12-06T01:59:29Z

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java

+        logger.info(message);
+        // Default to INFO level
+    }
+    MDC.clear();


why does this need to be cleared?

MDC carries contextual information in log messages. It is tied to thread, and is safer to clear it to avoid it be carried over to other log entries.

I see. Can you add that as a comment? Or a link to a guide that tells us clear it so future readers?

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java

lqiu96 · 2024-12-06T02:07:57Z

oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java

+  private GenericData parseResponseAs(HttpResponse response) throws IOException {
+    GenericData genericData = response.parseAs(GenericData.class);
+    LoggingUtils.logGenericData(genericData, LOGGER, "Auth response payload.");
+    return genericData;
+  }


Can we move this to LoggingUtils since this seems to be used in the other credentials?

They are slightly different, but I will try to extract any useful shared logics into LoggingUtils.

Can you highlight the difference? From what I see, the other Credentials also end up calling GenericData responseData = response.parseAs(GenericData.class); but this is moved to a function.

Might be missing something, but I don't see what is preventing this from going into LoggingUtils.

…pom.

zhumin8 · 2024-12-10T16:07:21Z

.github/workflows/ci.yaml

@@ -36,6 +36,7 @@ jobs:
    - run: .kokoro/build.sh
      env:
        JOB_TYPE: test
+        GOOGLE_SDK_JAVA_LOGGING: true


for next release: this env var is probably only needed for LoggingTest.java class.

zhumin8 · 2024-12-10T16:24:01Z

sonar has been failing since Dec 2. Filed
#1591

blakeli0 · 2024-12-10T17:44:59Z

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java

+    try {
+      if (logger.isInfoEnabled()) {
+        Map<String, String> loggingDataMap = new HashMap<>();
+        loggingDataMap.put("request.method", request.getRequestMethod());


I think it would be good to duplicate information like request.method and request.url in message instead of always relying on MDC, as we discussed yesterday. It would be great if we can make this change in this release. However, if not, it should not be a blocker either, as adding these info later is not a breaking change.

It's already done. (9e20be9)
It is verified in LoggingTest (here you can see the message is parsed then verified. The test is not extensive in verifying information. )

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

oauth2_http/java/com/google/auth/oauth2/LoggingConfigs.java

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java

lqiu96 · 2024-12-10T19:38:13Z

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java

+          // are payload always GenericData? If so, can parse and store in json
+          if (request.getContent() instanceof UrlEncodedContent) {
+
+            GenericData data = (GenericData) ((UrlEncodedContent) request.getContent()).getData();


Can you add a quick comment about why UrlEncodedContent is casted to GenericData? But JsonHttpContent (below) doesn't need to be?

lqiu96 · 2024-12-10T19:43:04Z

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java

+    }
+  }
+
+  static void logGenericData(GenericData genericData, Logger logger, String message) {


nit: Can we change this to be something like logResponsePayload and parseResponsePayload so it's clearer as to what it's doing?

lqiu96 · 2024-12-10T19:45:50Z

oauth2_http/java/com/google/auth/oauth2/ServiceAccountCredentials.java

+  private GenericData parseResponseAs(HttpResponse response) throws IOException {
+    GenericData genericData = response.parseAs(GenericData.class);
+    LoggingUtils.logGenericData(genericData, LOGGER, "Auth response payload");
+    return genericData;
+  }


qq, isn't this the same thing as LoggingUtils.logGenericData()? I'm missing why this method needs to exist

This response.parseAs(GenericData.class); can only be parsed once. So this is a wrapper to parse the data, log and continue with whatever is to do with the genericData.

Apologies, I might be missing something obvious but I'm not see where it's parsed more than once and might as be missing why it's different for ServiceAccountCredentials.

What I see is that it's changed to be:

GenericData responseData = parseResponseAs(response);

but I think it could just be

GenericData responseData = response.parseAs(GenericData.class); LoggingUtils.logGenericData(responseData, LOGGER, "Auth response payload"); ...

which is the same as it is for the other Credential types (i.e.

google-auth-library-java/oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java

Lines 300 to 302 in 6485526

GenericData responseData = response.parseAs(GenericData.class);

LoggingUtils.logGenericData(

responseData, SLF4JLOGGER, "Response from refresh access token payload");

)

sonarqubecloud · 2024-12-11T15:51:40Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
92.1% Coverage on New Code
0.5% Duplication on New Code

See analysis details on SonarQube Cloud

blakeli0 · 2024-12-11T23:19:45Z

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java

+    }
+  }
+
+  static void logResponse(HttpResponse response, Logger logger, String message) {


In general, I think we should not try to call these methods if logging is not enabled due to performance concerns, similar to the comment in sdk-platform-java.

blakeli0 · 2024-12-11T23:21:56Z

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java

+        responseLogDataMap.put("response.headers", headers.toString());
+        logWithMDC(logger, org.slf4j.event.Level.INFO, responseLogDataMap, message);
+      }
+    } catch (Exception e) {


Similar to the comment in sdk-platform-java, I think we should try to avoid doing these try catches or fail more gracefully.

zhumin8 added 2 commits December 4, 2024 21:14

add slf4j and other test dep. add logging utils. add logging to req/r…

c38282b

…esp to auth endpoints.

remove JUL wrapper as fallback condition. Other test setups.

3f30267

product-auto-label bot added the size: l Pull request size is large. label Dec 5, 2024

zhumin8 added 7 commits December 5, 2024 10:42

remove unused LoggingInterceptor.

35d9a75

revert unintential change to test. add comments.

a63d6fe

remove public from LoggingUtils.

83329e7

downgrade logback and remove unused.

6a5d3d3

put logging inside try-catch block to avoid crashing for logging errors.

930d0aa

simplify logic for getLogger. remove unused argLine for maven-surefir…

44f3de8

…e-plugin.

logging util class refactor.

598d5b9

zhumin8 requested a review from lqiu96 December 5, 2024 20:41

fix test setup timing issue.

445fe08

lqiu96 reviewed Dec 5, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java Show resolved Hide resolved

lqiu96 reviewed Dec 5, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 5, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 6, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/IamUtils.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 6, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/LoggingConfigs.java Show resolved Hide resolved

lqiu96 reviewed Dec 6, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/LoggingConfigs.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 6, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 6, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java Show resolved Hide resolved

lqiu96 reviewed Dec 6, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 6, 2024

View reviewed changes

zhumin8 added 5 commits December 6, 2024 22:47

add tests. feedback. minor cleanups.

a7f5a5e

TestAppender change to per test. add LoggingTest.

8bc1ae7

update messages.

bde6ede

test setup - try use env for all test in ci.

7c89c3f

add env set to sonar test.

6da450d

duplicate fields to log in message. add tests.

9e20be9

product-auto-label bot added size: xl Pull request size is extra large. and removed size: l Pull request size is large. labels Dec 10, 2024

zhumin8 added 6 commits December 9, 2024 22:19

add tests.

db99e70

minor cleanup and comment.

68307fc

fix add env to sonar ci.

4202902

add log env to integration in build.sh

cfbe873

deps: add gson explicitly. move gson and slf4j-api version to parent …

858b725

…pom.

deps: explicitly declare org.hamcrest:hamcrest-core:test

fb8889a

zhumin8 marked this pull request as ready for review December 10, 2024 16:05

zhumin8 requested review from a team as code owners December 10, 2024 16:05

zhumin8 changed the title ~~[draft] feat: add client logging~~ feat: add client logging with slf4j Dec 10, 2024

zhumin8 commented Dec 10, 2024

View reviewed changes

blakeli0 reviewed Dec 10, 2024

View reviewed changes

lqiu96 reviewed Dec 10, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 10, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 10, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/ComputeEngineCredentials.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 10, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/LoggingConfigs.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 10, 2024

View reviewed changes

oauth2_http/java/com/google/auth/oauth2/LoggingUtils.java Outdated Show resolved Hide resolved

lqiu96 reviewed Dec 10, 2024

View reviewed changes

zhumin8 added 3 commits December 10, 2024 15:48

update logging messages to rm 'auth'.

15d3016

make sensitive key comparison case insensitive.

c2a8d76

extract env var name.

6485526

blakeli0 reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add client logging with slf4j #1586

feat: add client logging with slf4j #1586

zhumin8 commented Dec 5, 2024 •

edited

Loading

zhumin8 commented Dec 5, 2024

lqiu96 Dec 6, 2024

lqiu96 Dec 6, 2024

zhumin8 Dec 6, 2024

lqiu96 Dec 9, 2024

lqiu96 Dec 6, 2024

zhumin8 Dec 6, 2024

lqiu96 Dec 9, 2024

zhumin8 Dec 10, 2024

zhumin8 commented Dec 10, 2024

blakeli0 Dec 10, 2024

zhumin8 Dec 10, 2024

lqiu96 Dec 10, 2024

zhumin8 Dec 11, 2024

lqiu96 Dec 10, 2024

lqiu96 Dec 10, 2024

zhumin8 Dec 10, 2024

lqiu96 Dec 11, 2024

sonarqubecloud bot commented Dec 11, 2024

blakeli0 Dec 11, 2024

blakeli0 Dec 11, 2024

	GenericData responseData = response.parseAs(GenericData.class);
	LoggingUtils.logGenericData(
	responseData, SLF4JLOGGER, "Response from refresh access token payload");

feat: add client logging with slf4j #1586

Are you sure you want to change the base?

feat: add client logging with slf4j #1586

Conversation

zhumin8 commented Dec 5, 2024 • edited Loading

zhumin8 commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhumin8 commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 11, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhumin8 commented Dec 5, 2024 •

edited

Loading