line/armeria

Provide a way to customize gRPC content logging

0x1306e6d opened this issue · 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.

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.

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.

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

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.