Incorrect logging protobuf requests/responces

[juncevich]

Useless converting raw bytes from request(response) bodies broke protobuf model deserialization.

Description

Protobuf response body incorrect deserializing, because FilteredHttpRequest(FilteredHttpResponse).getBody() converts body to string and after that convert string to byte array. Which cause to brake model from body. For example, if i have raw body bytes array size 170, response.getBody() array of bytes size will 250, because it was converted to string.

Expected Behavior

FilteredHttpRequest.getBody() gives raw bytes, without converting to string.

Actual Behavior

Possible Fix

getBody must contains raw bytes from response

[juncevich]

Can fix it ASAP.

[skjolber]

@juncevich out of interest, which web server are you running?

[juncevich]

@skjolber Netty with Micronaut.

[skjolber]

I wonder if gRPC logging might be better served with a gRPC-centric module. At least thats how we're doing it internally (emulating the Logbook output format).

Also the HTTP 2 does not follow a strict request-response any more, with 1-n, n-1 or n-n combinations, not sure whether logbook will work as-is now @whiskeysierra?

[juncevich]

I've offered not to convert body to string in method byte[] getBody(). I suppose when when someone asked body in bytes - bytes must be raw. There is no need to convert body to string and after it convert to bytes. If i want body as string a can use String getBody(). I mean that problem not in gRPC, and fix in byte[] getBody() will fix the problem.

[whiskeysierra]

Actual Behavior

Is that the default implementation right now?

[juncevich]

Yep

logbook/logbook-core/src/main/java/org/zalando/logbook/FilteredHttpRequest.java

Line 78 in c54d0cb

public byte[] getBody() throws IOException {

[whiskeysierra]

Mmh, I see a bigger problem now.
The idea of filtering (the content or body) is done using a BodyFilter:

logbook/logbook-api/src/main/java/org/zalando/logbook/BodyFilter.java

Lines 11 to 13 in 39cc999

	public interface BodyFilter {
	String filter(@Nullable final String contentType, final String body);

As you can see, the interface that we defined here takes a String and returns a String.
This would need to change from String to byte[].

[juncevich]

@whiskeysierra Will we fix these problems? I can do it)

[whiskeysierra]

Will we fix these problems? I can do it)

I'd love to, but it sounds like a breaking change, tbh.

[whiskeysierra]

Useless converting raw bytes from request(response) bodies broke protobuf model deserialization.

@juncevich Just curious, do you just use protobuf over HTTP? I'm thinking how to integrate Logbook with gRPC and I'm looking for ways to do that cleanly.

[skjolber]

FYI we are using grpc-java and grpc-netty with subclasses of io.grpc.ServerInterceptor.

[whiskeysierra]

Can you share some of that code by any chance?

…

On Mon, 8 May 2023, 17:03 Thomas Skjølberg, ***@***.***> wrote: FYI we are using grpc-java and grpc-netty with subclasses of io.grpc.ServerInterceptor. — Reply to this email directly, view it on GitHub <#1330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADI7HJK5RQPSH6EXSGCRW3XFEDKLANCNFSM5YCZAW5A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[skjolber]

Yes. I've extracted the relevant subset of our internal logging project and published it here, it still needs some cleanups, but it is a fully working example (at least the unit tests run).

You'll notice that Logbook is not in use at all, however the output (JSON) format is essentially the same as in Logbook.

Also the gRPC stack typically lets us return a ton of responses for a single request, this turned out to be too noisy for our taste, so we added some "summary" request-response message types.

[juncevich]

Useless converting raw bytes from request(response) bodies broke protobuf model deserialization.

@juncevich Just curious, do you just use protobuf over HTTP? I'm thinking how to integrate Logbook with gRPC and I'm looking for ways to do that cleanly.

Yes.

[github-actions]

In order to prioritize the support for Logbook, we would like to check whether the old issues are still relevant.
This issue has not been updated for over six months.

• Please check if it is still relevant in latest version of the Logbook.
• If so, please add a descriptive comment to keep the issue open.
• Otherwise, the issue will automatically be closed after a week.