feat: add client side logging with slf4j #3403

zhumin8 · 2024-11-27T22:08:22Z

WIP - test failures still expected.
Will update description when ready

…g needs more work.

…not enabled.

gax-java/gax/src/main/java/com/google/api/gax/logging/LoggingUtils.java

blakeli0 · 2024-12-03T19:01:07Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

+      @Override
+      public void start(Listener<RespT> responseListener, Metadata headers) {
+        if (logger.isInfoEnabled()) {
+          String requestId = UUID.randomUUID().toString();


I think requestId is for debugging purpose for now and would be removed later?

Yes for now. Later when we also log in tracer, we may need some requestID to track (not this one).

gax-java/gax/src/main/java/com/google/api/gax/logging/JsonContextMapHandler.java

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

…ngUtils.

…g entries, add request id to track.

… not done yet.)

…ing interceptors.

…sts.

jskeet · 2024-12-11T10:47:10Z

gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonLoggingInterceptor.java

+            .getHeaders()
+            .forEach((key, value) -> requestHeaders.addProperty(key, value.toString()));
+        logDataBuilder.requestHeaders(gson.toJson(requestHeaders));
+        logDataBuilder.requestHeaders(gson.toJson(requestHeaders));


Why are we calling this twice?

That's a mistake. Thanks for catching this!

zhumin8 · 2024-12-11T15:38:48Z

ci intergration are failing because cannot find slf4j classes.

external/com_google_api_gax_java/gax/src/main/java/com/google/api/gax/logging/LoggingUtils.java:39: error: symbol not found org.slf4j.Logger
import org.slf4j.Logger;

I added deps to dependencies.properties file: 172701d
Is any where else I need to add these dependencies to? Or are the names malformed? @blakeli0

blakeli0 · 2024-12-11T19:10:41Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

+import org.slf4j.event.Level;
+
+@InternalApi
+public class GrpcLoggingInterceptor implements ClientInterceptor {


This can be package private. Same thing for HttpJsonLoggingInterceptor.

With the current test setup, these needs to be public for testing purposes. I'll look into the test setup again

blakeli0 · 2024-12-11T21:15:17Z

gax-java/dependencies.properties

@@ -76,6 +76,7 @@ maven.com_google_http_client_google_http_client_gson=com.google.http-client:goog
 maven.org_codehaus_mojo_animal_sniffer_annotations=org.codehaus.mojo:animal-sniffer-annotations:1.24
 maven.javax_annotation_javax_annotation_api=javax.annotation:javax.annotation-api:1.3.2
 maven.org_graalvm_sdk=org.graalvm.sdk:graal-sdk:22.3.5
+maven_org_slf4j_slf4j_api=org.slf4j:slf4j-api:2.0.16


Suggested change

maven_org_slf4j_slf4j_api=org.slf4j:slf4j-api:2.0.16

maven.org_slf4j_slf4j_api=org.slf4j:slf4j-api:2.0.16

blakeli0 · 2024-12-11T22:44:48Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

+        }
+      }
+    } catch (Exception e) {
+      logger.error("Error logging request info (and headers)", e);


What are the possible errors? I think we need to make sure there are no errors. Even if there are possible errors due to misconfiguration, we should fail more gracefully. This would result in one error log entry for every request which would flood customer's application log.
Same thing for all other try catch blocks.

These are catch-all safe guard to ensure logging logic never cause user application to crash. Especially due to original timeline, I was concerned that I would miss anything. I will definitely take another look at these logging logic.

That said, I still lean to keep these try-catch blocks as safety net. If we are concerned about flooding customers's log, maybe we can do nothing in the catch and let logging silently fail, which I think is acceptable.

blakeli0 · 2024-12-11T22:54:06Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

+        .keys()
+        .forEach(
+            key -> {
+              Metadata.Key<String> metadataKey =


gRPC metadata could be binary, see https://grpc.io/docs/guides/metadata/. I'm not sure if this is going to fail or not if the value is binary, can you test it out? Either way, I think we should exclude binary headers as they don't provide much value.

blakeli0 · 2024-12-11T23:00:58Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

+public class GrpcLoggingInterceptor implements ClientInterceptor {
+
+  private static final Logger LOGGER = LoggingUtils.getLogger(GrpcLoggingInterceptor.class);
+  private static final Gson GSON = new Gson();


I don't think gax-grpc depends on gson as of now, we need to be careful introducing new dependencies. Can we try to not use gson if possible? If we have to use it, can we extract the common logic to gax?

I was under impression that gax-grpc already depends on gson. I will try to extract to gax.

blakeli0 · 2024-12-11T23:02:57Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

+  <RespT> void recordResponsePayload(RespT message, LogData.Builder logDataBuilder) {
+    try {
+      if (LOGGER.isDebugEnabled()) {
+        logDataBuilder.responsePayload(GSON.toJsonTree(message));


message is a Protobuf message, so we should use Protobuf utils to serialize it to Json if we have to. See how we serialized it in ProtoRestSerializer. Same comment for the request payload logging below.

Thanks for the pointer. Will look into.

blakeli0 · 2024-12-11T23:06:47Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/internal/SystemEnvironmentProvider.java

+
+/** Represents the default system environment provider. */
+@InternalApi
+public class SystemEnvironmentProvider implements EnvironmentProvider {


Is this class needed? Can we do System::getEnv?

This class is needed because of the current test setup, again was something of lower priority because it is not public and only for test setup.
I will look into migrate tests to only use ci setups to control env vars, in that case, I will be able to get rid of this.

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

blakeli0 · 2024-12-11T23:12:37Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

+    };
+  }
+
+  // Helper methods for logging


Since we are exposing public util classes in gax already, I think it's better to move these to gax to prevent duplication. This can also help us remove the dependency of gson in gax-grpc, and LogData does not have to exposed publicly.

Yes. I was inclined to do this as refactoring later, but I agree these are better of as utils methods from gax.

…t set. also some cleanups.

sonarqubecloud · 2024-12-12T22:56:22Z

Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
66.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

sonarqubecloud · 2024-12-12T23:00:03Z

Quality Gate failed for 'java_showcase_integration_tests'

Failed conditions
39.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

blakeli0 · 2024-12-13T19:34:19Z

gax-java/pom.xml

@@ -205,6 +210,20 @@
      </exclusions>
      <scope>test</scope>
    </dependency>
+    <!--    Logback  for testing -->
+    <dependency>


It would be great if we don't have to add logback in unit tests. Maybe create our own SLF4J test logger, similar to the JUL TestLogger

blakeli0 · 2024-12-13T19:35:41Z

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java

+
+      @Override
+      public void start(Listener<RespT> responseListener, Metadata headers) {
+        logRequestInfo(method, logDataBuilder, LOGGER);


There is a chance that a call is started but failed before sending messages, so it would be better if we only record the log data (headers) here, and do the actual logging in sendMessage.

zhumin8 added 9 commits November 19, 2024 11:16

poc: add log in grpc interceptor.

59b5d73

try out basics with httpjson interceptor.

d34ec88

implement conditional logging, use JUL when no binding. log formattin…

b6417ae

…g needs more work.

temp manual testing with showcase.

0c169ad

simplify logic for conditional logger.

9ced175

implement logging for basic case.

681f7d5

add a JsonContextMapHandler to format in JUL case.

5c01ef2

fix response payload for streaming. Add if guards to bypass log when …

6605ef3

…not enabled.

when logging disabled, assign NOPLogger directly.

ebbae15

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

some temp settings for testing with showcase.

543298d

blakeli0 reviewed Dec 3, 2024

View reviewed changes

gax-java/gax/src/main/java/com/google/api/gax/logging/LoggingUtils.java Outdated Show resolved Hide resolved

blakeli0 reviewed Dec 3, 2024

View reviewed changes

gax-java/gax/src/main/java/com/google/api/gax/logging/JsonContextMapHandler.java Outdated Show resolved Hide resolved

blakeli0 reviewed Dec 3, 2024

View reviewed changes

gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcLoggingInterceptor.java Outdated Show resolved Hide resolved

zhumin8 added 11 commits December 5, 2024 16:25

remove JUL wrapper for now. refactor getLogger(). Add tests for Loggi…

aac66f2

…ngUtils.

minor clean up.:

487614c

fix test after adding interceptor for logging for http.

cc3465b

cleanup and fix response header.

211a3d1

lint

2970210

lint: license.

32f8b7c

refactor GrpcLoggingInterceptor for readability, intro LogData.

245c14d

minor cleanups.

8358497

GrpcLoggingInterceptor fix for thread safe, separate out debug loggin…

97087b2

…g entries, add request id to track.

minor changes + add showcase it test.(need to run with env var, setup…

727a3e9

… not done yet.)

refactor HttpJsonLoggingInterceptor. remove public modifier from logg…

23bc111

…ing interceptors.

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

zhumin8 added 2 commits December 7, 2024 21:41

add GrpcLoggingInterceptorTest and test interceptor structure.

bbe0700

fix status code '0'.

3d7c7f9

zhumin8 added 12 commits December 10, 2024 17:38

add showcase tests. expose logging interceptors as public to setup te…

d85c848

…sts.

add internal api annotation.

1919809

lint

613b6c8

fix typo

1169eaa

remove unused test class.

a6b5433

try to exclude test from showcase-native.

fb0966e

fix logback version.

77939fe

revert accidental change.

04ef774

fix typo in ci.

439e071

cleanup gax pom. add tests.

673c9fe

add test.

c2b607b

Merge branch 'main' into client-log

3df39fa

jskeet reviewed Dec 11, 2024

View reviewed changes

zhumin8 added 5 commits December 11, 2024 09:38

rm redundant assignment.

4190dc7

add logging test setup in httpjson.

fefb436

add log response test to interceptors.

7901d8a

remove redundant code.

20f9f5a

add slf4j and test dep to dependencies.properties.

172701d

zhumin8 changed the title ~~[POC] add client side logging~~ feat: add client side logging with slf4j Dec 11, 2024

Merge branch 'main' into client-log

d25e742

zhumin8 marked this pull request as ready for review December 11, 2024 15:49

make requestPayload also as JsonElement in LogData.

a8d2f20

blakeli0 reviewed Dec 11, 2024

View reviewed changes

zhumin8 added 2 commits December 12, 2024 17:38

fix typo in dependencies.properties.

df97834

add test assertion to verify logger not enabled at levels when env no…

32dbd0e

…t set. also some cleanups.

blakeli0 reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add client side logging with slf4j #3403

feat: add client side logging with slf4j #3403

zhumin8 commented Nov 27, 2024 •

edited

Loading

blakeli0 Dec 3, 2024

zhumin8 Dec 3, 2024

jskeet Dec 11, 2024

zhumin8 Dec 11, 2024

zhumin8 commented Dec 11, 2024

blakeli0 Dec 11, 2024

zhumin8 Dec 12, 2024

blakeli0 Dec 11, 2024

blakeli0 Dec 11, 2024

zhumin8 Dec 12, 2024

blakeli0 Dec 11, 2024

blakeli0 Dec 11, 2024

zhumin8 Dec 12, 2024

blakeli0 Dec 11, 2024

zhumin8 Dec 12, 2024

blakeli0 Dec 11, 2024

zhumin8 Dec 12, 2024

blakeli0 Dec 11, 2024

zhumin8 Dec 12, 2024

sonarqubecloud bot commented Dec 12, 2024

sonarqubecloud bot commented Dec 12, 2024

blakeli0 Dec 13, 2024

blakeli0 Dec 13, 2024

	maven_org_slf4j_slf4j_api=org.slf4j:slf4j-api:2.0.16
	maven.org_slf4j_slf4j_api=org.slf4j:slf4j-api:2.0.16

feat: add client side logging with slf4j #3403

Are you sure you want to change the base?

feat: add client side logging with slf4j #3403

Conversation

zhumin8 commented Nov 27, 2024 • edited Loading

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 11, 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

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 12, 2024

Quality Gate failed for 'gapic-generator-java-root'

sonarqubecloud bot commented Dec 12, 2024

Quality Gate failed for 'java_showcase_integration_tests'

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhumin8 commented Nov 27, 2024 •

edited

Loading