feat: add client debug logging support for unary-stream gRPC/REST calls #794

parthea · 2025-02-14T22:34:51Z

Towards b/383403001

ohmayr · 2025-04-03T15:10:03Z

google/api_core/_rest_streaming_base.py

@@ -97,19 +100,38 @@ def _process_chunk(self, chunk: str):
                self._obj += char
            self._escape_next = not self._escape_next if char == "\\" else False

+    def _log_response_payload(self, response_payload: str):  # pragma: NO COVER


Why do we need to skip coverage here?

ohmayr · 2025-04-03T15:11:11Z

google/api_core/_rest_streaming_base.py

+        _LOGGER.debug(
+            "Received response via REST stream",
+            extra={
+                "response": rest_response,


should we be using httpResponse here instead for structured logs?

ohmayr · 2025-04-03T15:13:39Z

google/api_core/_rest_streaming_base.py

                )

            return grab
        elif issubclass(self._response_message_cls, google.protobuf.message.Message):

            def grab(this):
-                return Parse(this._ready_objs.popleft(), this._response_message_cls())
+                response_payload = this._ready_objs.popleft()
+                if logging_enabled:  # pragma: NO COVER


Can we add coverage for this?

ohmayr · 2025-04-03T15:18:23Z

google/api_core/grpc_helpers.py

+            else:
+                result = next(self._wrapped)
+            logging_enabled = _LOGGER.isEnabledFor(logging.DEBUG)
+            if logging_enabled:  # pragma: NO COVER
+                if isinstance(result, proto.Message):
+                    response_payload = type(result).to_json(result)
+                elif isinstance(result, google.protobuf.message.Message):
+                    response_payload = google.protobuf.json_format.MessageToJson(result)
+                else:
+                    response_payload = (
+                        f"{type(result).__name__}: {str(pickle.dumps(result))}"
+                    )
+                grpc_response = {
+                    "payload": response_payload,
+                    "status": "OK",
+                }
+                _LOGGER.debug(
+                    f"Received response of type {type(result)} via gRPC stream",
+                    extra={
+                        "response": grpc_response,
+                    },


Can this be added as a helper function and re-used in grpc_helpers_async?

ohmayr · 2025-04-03T15:32:03Z

google/api_core/grpc_helpers_async.py

+            result = await self._call.read()
+            logging_enabled = _LOGGER.isEnabledFor(logging.DEBUG)
+            if logging_enabled:  # pragma: NO COVER
+                if isinstance(result, proto.Message):


Can you link

{# TODO(https://github.com/googleapis/gapic-generator-python/issues/2293): Investigate if we can improve this logic or wait for next gen protobuf. #}

Also, can the parsing logic be added as a helper function and re-used in the gapic? We can file a TODO for the latter.

+1

This ties into the issue I still want to get back to b/382299158. Maybe we reference that issue instead of creating a new one, and in that issue we reference these changes so we have easy access.

vchudnov-g · 2025-04-03T16:46:40Z

google/api_core/_rest_streaming_base.py

+        rest_response = {
+            "payload": response_payload,
+            "status": "OK",
+        }
+        _LOGGER.debug(
+            "Received response via REST stream",
+            extra={
+                "response": rest_response,


I'm not sure we should be using/reference any sort of HTTP response here at all. This helper is being called from grab, and grab simply passes already received messages reconstituted from chunks. So what we should log in grab is something like passing next message to stream: <message>.

We should log http responses where we actually receive the HTTP chunks.

vchudnov-g · 2025-04-03T16:55:06Z

google/api_core/grpc_helpers.py

+                grpc_response = {
+                    "payload": response_payload,
+                    "status": "OK",


As in the REST case, I don't think we should be logging things here as though we're receiving a response from the server. We should log that we're passing the next item in the stream. (Because, for example, we might be getting _stored_first_result that was received and stored earlier.)

So let's log that we're returning the next item here, but wherever we do receive the item from the server (is it next(self.wrapped)?), we should log that we received X message from the server: <msg> <msg> ....

IOW, let's not conflate for ourselves or our users receiving the streaming data from the server vs passing eached streamed message to the GAPIC user. This is particularly important for async streaming.

vchudnov-g · 2025-04-03T17:00:42Z

google/api_core/grpc_helpers_async.py

+            result = await self._call.read()
+            logging_enabled = _LOGGER.isEnabledFor(logging.DEBUG)
+            if logging_enabled:  # pragma: NO COVER
+                if isinstance(result, proto.Message):


+1

This ties into the issue I still want to get back to b/382299158. Maybe we reference that issue instead of creating a new one, and in that issue we reference these changes so we have easy access.

vchudnov-g · 2025-04-03T17:02:45Z

google/api_core/grpc_helpers_async.py

@@ -94,7 +100,28 @@ def __init__(self):

    async def read(self) -> P:
        try:
-            return await self._call.read()
+            result = await self._call.read()


IIUC, this gRPC doing an actual read from the network, so logging it as such below makes sense.

(just pointing this out to contrast with my other comments about logging returning previously streamed messages to the user)

vchudnov-g · 2025-04-03T17:30:18Z

Also, could you reference googleapis/gapic-generator-python#2289 rather than the mirrored internal issue in this PR's description?

product-auto-label bot added the size: m Pull request size is medium. label Feb 14, 2025

parthea mentioned this pull request Feb 14, 2025

feat: add client debug logging support for unary-stream gRPC calls googleapis/gapic-generator-python#2336

Open

parthea force-pushed the add-unary-stream-logging branch from ae07484 to 3b29113 Compare February 14, 2025 22:37

parthea self-assigned this Feb 14, 2025

parthea force-pushed the add-unary-stream-logging branch from 739a255 to fdf6bd6 Compare February 14, 2025 23:02

parthea marked this pull request as ready for review February 19, 2025 14:34

parthea requested review from a team as code owners February 19, 2025 14:34

parthea assigned ohmayr and vchudnov-g and unassigned parthea Feb 19, 2025

parthea changed the title ~~feat: add client debug logging support for unary-stream gRPC calls~~ feat: add client debug logging support for unary-stream gRPC/REST calls Feb 19, 2025

parthea mentioned this pull request Feb 19, 2025

feat: add client debug logging support for unary-stream REST calls googleapis/gapic-generator-python#2340

Open

parthea added 7 commits March 19, 2025 15:13

feat: add client debug logging support for unary-stream gRPC calls

2123298

mypy

5510056

coverage

3fb362f

include debug log for first result in grpc unary->stream calls

d73d18c

add debug logs for rest streaming

826c79b

result->response_payload

230fe9a

cover

9fac30c

parthea force-pushed the add-unary-stream-logging branch from 8908765 to 9fac30c Compare March 19, 2025 15:13

Merge branch 'main' into add-unary-stream-logging

56810ca

ohmayr reviewed Apr 3, 2025

View reviewed changes

vchudnov-g reviewed Apr 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add client debug logging support for unary-stream gRPC/REST calls #794

feat: add client debug logging support for unary-stream gRPC/REST calls #794

parthea commented Feb 14, 2025 •

edited

Loading

ohmayr Apr 3, 2025

ohmayr Apr 3, 2025

ohmayr Apr 3, 2025

ohmayr Apr 3, 2025

ohmayr Apr 3, 2025

ohmayr Apr 3, 2025

vchudnov-g Apr 3, 2025

vchudnov-g Apr 3, 2025

vchudnov-g Apr 3, 2025

vchudnov-g Apr 3, 2025

vchudnov-g Apr 3, 2025

vchudnov-g commented Apr 3, 2025

feat: add client debug logging support for unary-stream gRPC/REST calls #794

Are you sure you want to change the base?

feat: add client debug logging support for unary-stream gRPC/REST calls #794

Conversation

parthea commented Feb 14, 2025 • 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

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

vchudnov-g commented Apr 3, 2025

parthea commented Feb 14, 2025 •

edited

Loading