armeria icon indicating copy to clipboard operation
armeria copied to clipboard

Provide a way to customize gRPC content logging

Open 0x1306e6d opened this issue 1 year ago • 4 comments

Hello,

When using LoggingService with a gRPC service, an escaped content is logged when non-ASCII string is included in its message:

content=CompletableRpcResponse{foo {
  id: "J9CfU0LU"
  name: "\354\225\210\353\205\225\355\225\230\354\204\270\354\232\224"
}
}

I guess that this is the default behavior of protobuf-java's toString(). There's an option not to escape, so I could address by adding contentSanitizer to use the custom TextFormat only when the content type is protobuf Message:

LogFormatter.builderForText()
            .contentSanitizer(
                    (requestContext, o) -> {
                        if (o instanceof CompletableRpcResponse res) {
                            final Object result = res.getNow(null);
                            if (result instanceof Message message) {
                                return TextFormat.printer()
                                                 .escapingNonAscii(false)
                                                 .printToString(message);
                            }
                        }
                        return o.toString();
                    })
            .build();

For gRPC (Thrift also I guess), Logging{Client|Service} simply calls toString() of a CompletableRpcResponse and there's a weak support for handling its content for logging. It'd be useful if we provide a way to handle the content.

0x1306e6d avatar Jun 20 '24 11:06 0x1306e6d

Would it be a good idea for us to disable the escape option by default? Is there any risk of doing so? I think it's perhaps a good idea to escape the control characters though.

trustin avatar Jun 20 '24 11:06 trustin

I agree that disabling by default has a risk. I think that keeping the default behavior and providing an option to control would be better so that users can take the risk.

0x1306e6d avatar Jun 20 '24 11:06 0x1306e6d

I'm just trying to assess the risk. What risk would it have?

trustin avatar Jun 20 '24 11:06 trustin

I see, I misunderstood the intention. I took a conservative approach: there might be a risk that I could not imagine. I couldn't imagine any risk.

0x1306e6d avatar Jun 20 '24 12:06 0x1306e6d