line / armeria

Your go-to microservice framework for any situation, from the creator of Netty et al. You can build any type of microservice leveraging your favorite technologies, including gRPC, Thrift, Kotlin, Retrofit, Reactive Streams, Spring Boot and Dropwizard.
https://armeria.dev
Apache License 2.0
4.73k stars 899 forks source link

Provide a way to customize gRPC content logging #5777

Open 0x1306e6d opened 1 week ago

0x1306e6d commented 1 week ago

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.

trustin commented 1 week ago

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.

0x1306e6d commented 1 week ago

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.

trustin commented 1 week ago

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

0x1306e6d commented 1 week ago

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.