[SPARK-42721][CONNECT] RPC logging interceptor #40342

rangadi · 2023-03-08T19:20:41Z

What changes were proposed in this pull request?

This adds an gRPC interceptor in spark-connect server. It logs all the incoming RPC requests and responses.

How to enable: Set interceptor config. e.g.

./sbin/start-connect-server.sh --conf spark.connect.grpc.interceptor.classes=org.apache.spark.sql.connect.service.LoggingInterceptor  --jars connector/connect/server/target/spark-connect_*-SNAPSHOT.jar

Sample output:

 23/03/08 10:54:37 INFO LoggingInterceptor: Received RPC Request spark.connect.SparkConnectService/ExecutePlan (id 1868663481):   
 {
   "client_id": "6844bc44-4411-4481-8109-a10e3a836f97",
   "user_context": {
     "user_id": "raghu"
   },
   "plan": {
     "root": {
       "common": {
         "plan_id": "37"
       },
       "show_string": {
         "input": {
           "common": {
             "plan_id": "36"
           },
           "read": {
             "data_source": {
               "format": "csv",
               "schema": "",
               "paths": ["file:///tmp/x-in"]
             }
           }
         },
         "num_rows": 20,
         "truncate": 20
       }
     }
   },
   "client_type": "_SPARK_CONNECT_PYTHON"
 }

Why are the changes needed?

This is useful in development. It might be useful to debug some problems in production as well.

Does this PR introduce any user-facing change?

no

How was this patch tested?

Manually in development
Unit test

LuciferYang · 2023-03-09T07:05:18Z

Need run ./build/mvn -Pscala-2.12 scalafmt:format -Dscalafmt.skip=false -Dscalafmt.validateOnly=false -Dscalafmt.changedOnly=false -pl connector/connect/common -pl connector/connect/server -pl connector/connect/client/jvm to format the code in connect modules

rangadi · 2023-03-09T07:15:47Z

@LuciferYang thanks. Fixed.

LuciferYang · 2023-03-09T07:54:11Z

connector/connect/server/pom.xml

@@ -155,6 +155,12 @@
      <version>${protobuf.version}</version>
      <scope>compile</scope>
    </dependency>
+    <dependency>
+      <groupId>com.google.protobuf</groupId>
+      <artifactId>protobuf-java-util</artifactId>


Without this pr, the server module depended on protobuf-java-util:3.19.2 . Should protobuf-java-util and protobuf-java always use the same version?

Oh, that should be fine. I didn't realize it already included protobuf-java-util. Removed this change.
Could you point me to where this dependency comes from?

I removed the dependency and mvn build fails. But SBT is ok. Not sure where SBT gets the dependency from.
What shall we do?

You can check it through

build/mvn dependency:tree -pl connector/connect/server

protobuf-java-util is the transitive dependency of grpc-services, but it is runtime scope.

[INFO] +- io.grpc:grpc-services:jar:1.47.0:compile [INFO] | \- com.google.protobuf:protobuf-java-util:jar:3.19.2:runtime

So I think we should explicit add this dependency for compilation

Thanks, done. Updated the PR.

LuciferYang · 2023-03-09T08:25:57Z

.../connect/server/src/main/scala/org/apache/spark/sql/connect/service/LoggingInterceptor.scala

+      headers: Metadata,
+      next: ServerCallHandler[ReqT, RespT]): ServerCall.Listener[ReqT] = {
+
+    val id = Random.nextInt(Int.MaxValue) // Assign a random id for this RPC.


Should we change to use UUID? I think it has less conflict probability than Random.nextInt

I don't think so. This is just for debugging. UUID looks very long and makes the logs harder to read. I intentionally even avoided negative numbers :).

This reverts commit 698741f.

LuciferYang · 2023-03-10T15:47:10Z

fine to me, cc @zhenlineo @amaliujia @hvanhovell FYI

zhenlineo

LGTM.

hvanhovell

LGTM

### What changes were proposed in this pull request? This adds an gRPC interceptor in spark-connect server. It logs all the incoming RPC requests and responses. - How to enable: Set interceptor config. e.g. ./sbin/start-connect-server.sh --conf spark.connect.grpc.interceptor.classes=org.apache.spark.sql.connect.service.LoggingInterceptor --jars connector/connect/server/target/spark-connect_*-SNAPSHOT.jar - Sample output: 23/03/08 10:54:37 INFO LoggingInterceptor: Received RPC Request spark.connect.SparkConnectService/ExecutePlan (id 1868663481): { "client_id": "6844bc44-4411-4481-8109-a10e3a836f97", "user_context": { "user_id": "raghu" }, "plan": { "root": { "common": { "plan_id": "37" }, "show_string": { "input": { "common": { "plan_id": "36" }, "read": { "data_source": { "format": "csv", "schema": "", "paths": ["file:///tmp/x-in"] } } }, "num_rows": 20, "truncate": 20 } } }, "client_type": "_SPARK_CONNECT_PYTHON" } ### Why are the changes needed? This is useful in development. It might be useful to debug some problems in production as well. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? - Manually in development - Unit test Closes #40342 from rangadi/logging-interceptor. Authored-by: Raghu Angadi <raghu.angadi@databricks.com> Signed-off-by: Herman van Hovell <herman@databricks.com> (cherry picked from commit 19cb8d7) Signed-off-by: Herman van Hovell <herman@databricks.com>

dongjoon-hyun · 2023-03-11T03:23:20Z

Hi, @rangadi and @hvanhovell . There is a scalafmt error. Here is a follow-up to recover CI

[SPARK-42721][CONNECT][FOLLOWUP] Apply scalafmt to LoggingInterceptor #40374

rangadi · 2023-03-11T16:59:04Z

@dongjoon-hyun thank you! I should have checked. missed it.

### What changes were proposed in this pull request? This adds an gRPC interceptor in spark-connect server. It logs all the incoming RPC requests and responses. - How to enable: Set interceptor config. e.g. ./sbin/start-connect-server.sh --conf spark.connect.grpc.interceptor.classes=org.apache.spark.sql.connect.service.LoggingInterceptor --jars connector/connect/server/target/spark-connect_*-SNAPSHOT.jar - Sample output: 23/03/08 10:54:37 INFO LoggingInterceptor: Received RPC Request spark.connect.SparkConnectService/ExecutePlan (id 1868663481): { "client_id": "6844bc44-4411-4481-8109-a10e3a836f97", "user_context": { "user_id": "raghu" }, "plan": { "root": { "common": { "plan_id": "37" }, "show_string": { "input": { "common": { "plan_id": "36" }, "read": { "data_source": { "format": "csv", "schema": "", "paths": ["file:///tmp/x-in"] } } }, "num_rows": 20, "truncate": 20 } } }, "client_type": "_SPARK_CONNECT_PYTHON" } ### Why are the changes needed? This is useful in development. It might be useful to debug some problems in production as well. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? - Manually in development - Unit test Closes apache#40342 from rangadi/logging-interceptor. Authored-by: Raghu Angadi <raghu.angadi@databricks.com> Signed-off-by: Herman van Hovell <herman@databricks.com> (cherry picked from commit 19cb8d7) Signed-off-by: Herman van Hovell <herman@databricks.com>

rangadi added 2 commits March 7, 2023 13:46

Add LoggingInterceptor

0d7e59c

Update LoggingInterceptor

30a5e4c

github-actions bot added BUILD CONNECT SQL labels Mar 8, 2023

minor

455bffb

scala formatr fix

b68e1ea

minor

e5149b9

LuciferYang reviewed Mar 9, 2023

View reviewed changes

Remove protobuf-java-util dependency

698741f

github-actions bot removed the BUILD label Mar 9, 2023

rangadi added 2 commits March 9, 2023 08:46

Revert "Remove protobuf-java-util dependency"

ab70956

This reverts commit 698741f.

minor

5f7caa8

rangadi requested a review from LuciferYang March 10, 2023 06:22

github-actions bot added the BUILD label Mar 10, 2023

zhenlineo approved these changes Mar 10, 2023

View reviewed changes

hvanhovell approved these changes Mar 10, 2023

View reviewed changes

hvanhovell closed this in 19cb8d7 Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-42721][CONNECT] RPC logging interceptor #40342

[SPARK-42721][CONNECT] RPC logging interceptor #40342

rangadi commented Mar 8, 2023

LuciferYang commented Mar 9, 2023

rangadi commented Mar 9, 2023

LuciferYang Mar 9, 2023

rangadi Mar 9, 2023

rangadi Mar 9, 2023

LuciferYang Mar 10, 2023

rangadi Mar 10, 2023

LuciferYang Mar 9, 2023

rangadi Mar 9, 2023

LuciferYang commented Mar 10, 2023

zhenlineo left a comment

hvanhovell left a comment

dongjoon-hyun commented Mar 11, 2023

rangadi commented Mar 11, 2023

[SPARK-42721][CONNECT] RPC logging interceptor #40342

[SPARK-42721][CONNECT] RPC logging interceptor #40342

Conversation

rangadi commented Mar 8, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

LuciferYang commented Mar 9, 2023

rangadi commented Mar 9, 2023

LuciferYang Mar 9, 2023

Choose a reason for hiding this comment

rangadi Mar 9, 2023

Choose a reason for hiding this comment

rangadi Mar 9, 2023

Choose a reason for hiding this comment

LuciferYang Mar 10, 2023

Choose a reason for hiding this comment

rangadi Mar 10, 2023

Choose a reason for hiding this comment

LuciferYang Mar 9, 2023

Choose a reason for hiding this comment

rangadi Mar 9, 2023

Choose a reason for hiding this comment

LuciferYang commented Mar 10, 2023

zhenlineo left a comment

Choose a reason for hiding this comment

hvanhovell left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Mar 11, 2023

rangadi commented Mar 11, 2023