Skip to content

Commit a16e2c0

Browse files
ikhoonikhoon
authored
Fix a bug where gRPC and Thrift service name is not overriden by defa… (#2994)
…ultServcieName Motivation: The service name and method of gRPC and Thrift are set automatically to `RequestLog.*name()` by gRPC and Thrift service. However, the `defaultServiceName`, which was specified when building a service, should have a higher priority. By setting gRPC and Thrift service when a request side is ended, all service types will have a consistent behavior with regards to `RequestLog.serviceName()` and `ReqeustLog.name()`. Modifications: - Set `RequestLog.serviceName()` that was derived from a service stub when `defaultServiceName` is not configured. - Set `RequestLog.name()` that was derived from a method of service stub when `defaultLogName` is not configured. Result: gRPC and Thrift service now respect a user defined `defaultServiceName` and `defaultLogName`.
1 parent 4c52e50 commit a16e2c0

7 files changed

Lines changed: 99 additions & 26 deletions

File tree

‎core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java‎

Lines changed: 30 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1050,25 +1050,46 @@ private void setNamesIfAbsent() {
10501050
if (name == null) {
10511051
String newServiceName = null;
10521052
String newName = null;
1053+
ServiceConfig config = null;
1054+
1055+
// Set the default names from ServiceConfig
1056+
if (ctx instanceof ServiceRequestContext) {
1057+
config = ((ServiceRequestContext) ctx).config();
1058+
newServiceName = config.defaultServiceName();
1059+
newName = config.defaultLogName();
1060+
}
1061+
10531062
RpcRequest rpcReq = ctx.rpcRequest();
10541063
if (rpcReq == null && requestContent instanceof RpcRequest) {
10551064
rpcReq = (RpcRequest) requestContent;
10561065
}
10571066

1058-
if (rpcReq != null) {
1059-
newServiceName = rpcReq.serviceType().getName();
1060-
newName = rpcReq.method();
1061-
} else if (ctx instanceof ServiceRequestContext) {
1062-
final ServiceConfig config = ((ServiceRequestContext) ctx).config();
1063-
newServiceName = config.defaultServiceName();
1064-
if (newServiceName == null) {
1067+
// Set serviceName from ServiceType or innermost class name
1068+
if (newServiceName == null) {
1069+
if (rpcReq != null) {
1070+
final String serviceType = rpcReq.serviceType().getName();
1071+
if ("com.linecorp.armeria.internal.common.grpc.GrpcLogUtil".equals(serviceType)) {
1072+
// Parse gRPC serviceName and methodName
1073+
final String fullMethodName = rpcReq.method();
1074+
final int methodIndex = fullMethodName.lastIndexOf('/');
1075+
newServiceName = fullMethodName.substring(0, methodIndex);
1076+
if (newName == null) {
1077+
newName = fullMethodName.substring(methodIndex + 1);
1078+
}
1079+
} else {
1080+
newServiceName = serviceType;
1081+
}
1082+
} else if (config != null) {
10651083
newServiceName = getInnermostServiceName(config.service());
10661084
}
1067-
newName = config.defaultLogName();
10681085
}
10691086

10701087
if (newName == null) {
1071-
newName = ctx.method().name();
1088+
if (rpcReq != null) {
1089+
newName = rpcReq.method();
1090+
} else {
1091+
newName = ctx.method().name();
1092+
}
10721093
}
10731094

10741095
serviceName = newServiceName;

‎grpc/src/main/java/com/linecorp/armeria/internal/client/grpc/ArmeriaChannel.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,6 @@ public <I, O> ClientCall<I, O> newCall(
9292
HttpHeaderNames.TE, HttpHeaderValues.TRAILERS));
9393
final DefaultClientRequestContext ctx = newContext(HttpMethod.POST, req);
9494

95-
final String fullMethodName = method.getFullMethodName();
96-
final int methodIndex = fullMethodName.lastIndexOf('/') + 1;
97-
ctx.logBuilder().name(method.getServiceName(), fullMethodName.substring(methodIndex));
9895
ctx.logBuilder().serializationFormat(serializationFormat);
9996
ctx.logBuilder().defer(RequestLogProperty.REQUEST_CONTENT,
10097
RequestLogProperty.RESPONSE_CONTENT);

‎grpc/src/main/java/com/linecorp/armeria/server/grpc/FramedGrpcService.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -197,8 +197,6 @@ protected HttpResponse doPost(ServiceRequestContext ctx, HttpRequest req) throws
197197
}
198198
}
199199

200-
final int methodIndex = methodName.lastIndexOf('/') + 1;
201-
ctx.logBuilder().name(method.getMethodDescriptor().getServiceName(), methodName.substring(methodIndex));
202200
ctx.logBuilder().defer(RequestLogProperty.REQUEST_CONTENT,
203201
RequestLogProperty.RESPONSE_CONTENT);
204202

‎grpc/src/test/java/com/linecorp/armeria/server/grpc/GrpcServiceLogNameTest.java‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,18 @@ class GrpcServiceLogNameTest {
6161
static ServerExtension server = new ServerExtension() {
6262
@Override
6363
protected void configure(ServerBuilder sb) throws Exception {
64+
final GrpcService grpcService =
65+
GrpcService.builder()
66+
.addService(new TestServiceImpl(Executors.newSingleThreadScheduledExecutor()))
67+
.build();
68+
6469
sb.accessLogWriter(AccessLogWriter.combined(), true);
65-
sb.service(GrpcService.builder()
66-
.addService(new TestServiceImpl(Executors.newSingleThreadScheduledExecutor()))
67-
.build());
70+
sb.serviceUnder("/grpc/", grpcService);
71+
sb.route()
72+
.pathPrefix("/default-names")
73+
.defaultServiceName("DefaultServiceName")
74+
.defaultLogName("DefaultName")
75+
.build(grpcService);
6876
sb.decorator((delegate, ctx, req) -> {
6977
capturedCtx = ctx;
7078
return delegate.serve(ctx, req);
@@ -89,8 +97,9 @@ void cleanupLogger() {
8997

9098
@Test
9199
void logName() {
92-
final TestServiceBlockingStub client = Clients.builder(server.httpUri(GrpcSerializationFormats.PROTO))
93-
.build(TestServiceBlockingStub.class);
100+
final TestServiceBlockingStub client =
101+
Clients.builder(server.httpUri(GrpcSerializationFormats.PROTO).resolve("/grpc/"))
102+
.build(TestServiceBlockingStub.class);
94103
client.emptyCall(Empty.newBuilder().build());
95104

96105
final RequestLog log = capturedCtx.log().partial();
@@ -99,24 +108,39 @@ void logName() {
99108
assertThat(log.fullName()).isEqualTo(TestServiceGrpc.getEmptyCallMethod().getFullMethodName());
100109
}
101110

111+
@Test
112+
void defaultNames() {
113+
final TestServiceBlockingStub client =
114+
Clients.builder(server.httpUri(GrpcSerializationFormats.PROTO).resolve("/default-names/"))
115+
.build(TestServiceBlockingStub.class);
116+
client.emptyCall(Empty.newBuilder().build());
117+
118+
final RequestLog log = capturedCtx.log().partial();
119+
assertThat(log.serviceName()).isEqualTo("DefaultServiceName");
120+
assertThat(log.name()).isEqualTo("DefaultName");
121+
assertThat(log.fullName()).isEqualTo("DefaultServiceName/DefaultName");
122+
}
123+
102124
@Test
103125
void logNameInAccessLog() {
104-
final TestServiceBlockingStub client = Clients.builder(server.httpUri(GrpcSerializationFormats.PROTO))
105-
.build(TestServiceBlockingStub.class);
126+
final TestServiceBlockingStub client =
127+
Clients.builder(server.httpUri(GrpcSerializationFormats.PROTO).resolve("/grpc/"))
128+
.build(TestServiceBlockingStub.class);
106129
client.emptyCall(Empty.newBuilder().build());
107130

108131
await().untilAsserted(() -> {
109132
verify(appender, atLeast(0)).doAppend(eventCaptor.capture());
110133
assertThat(eventCaptor.getAllValues()).anyMatch(evt -> {
111-
return evt.getMessage().contains("POST /armeria.grpc.testing.TestService/EmptyCall h2c");
134+
return evt.getMessage().contains("POST /grpc/armeria.grpc.testing.TestService/EmptyCall h2c");
112135
});
113136
});
114137
}
115138

116139
@Test
117140
void logNameInClientSide() {
118-
final TestServiceBlockingStub client = Clients.builder(server.httpUri(GrpcSerializationFormats.PROTO))
119-
.build(TestServiceBlockingStub.class);
141+
final TestServiceBlockingStub client =
142+
Clients.builder(server.httpUri(GrpcSerializationFormats.PROTO).resolve("/grpc/"))
143+
.build(TestServiceBlockingStub.class);
120144
try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) {
121145
client.emptyCall(Empty.newBuilder().build());
122146
final ClientRequestContext ctx = captor.get();

‎thrift0.13/src/main/java/com/linecorp/armeria/internal/client/thrift/THttpClientDelegate.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,6 @@ public RpcResponse execute(ClientRequestContext ctx, RpcRequest call) {
9393
final List<Object> args = call.params();
9494
final CompletableRpcResponse reply = new CompletableRpcResponse();
9595

96-
ctx.logBuilder().name(call.serviceType().getName(), call.method());
9796
ctx.logBuilder().serializationFormat(serializationFormat);
9897

9998
final ThriftFunction func;

‎thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,6 @@ private void decodeAndInvoke(
482482
handlePreDecodeException(ctx, httpRes, cause, serializationFormat, seqId, methodName);
483483
return;
484484
}
485-
ctx.logBuilder().name(f.serviceType().getName(), methodName);
486485

487486
// Decode the invocation parameters.
488487
try {

‎thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/ThriftServiceLogNameTest.java‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,13 @@ protected void configure(ServerBuilder sb) throws Exception {
6767
sb.service("/thrift", THttpService.builder()
6868
.addService(HELLO_SERVICE_HANDLER)
6969
.build());
70+
sb.route()
71+
.path("/default-names")
72+
.defaultServiceName("HelloService")
73+
.defaultLogName("defaultName")
74+
.build(THttpService.builder()
75+
.addService(HELLO_SERVICE_HANDLER)
76+
.build());
7077
}
7178
};
7279

@@ -98,6 +105,19 @@ void logName() throws TException {
98105
assertThat(log.fullName()).isEqualTo(HelloService.AsyncIface.class.getName() + "/hello");
99106
}
100107

108+
@Test
109+
void defaultNames() throws TException {
110+
final HelloService.Iface client =
111+
Clients.builder(server.httpUri(ThriftSerializationFormats.BINARY).resolve("/default-names"))
112+
.build(HelloService.Iface.class);
113+
client.hello("hello");
114+
115+
final RequestLog log = capturedCtx.log().partial();
116+
assertThat(log.serviceName()).isEqualTo("HelloService");
117+
assertThat(log.name()).isEqualTo("defaultName");
118+
assertThat(log.fullName()).isEqualTo("HelloService/defaultName");
119+
}
120+
101121
@Test
102122
void logNameInAccessLog() throws TException {
103123
final HelloService.Iface client =
@@ -113,6 +133,21 @@ void logNameInAccessLog() throws TException {
113133
});
114134
}
115135

136+
@Test
137+
void defaultNamesInAccessLog() throws TException {
138+
final HelloService.Iface client =
139+
Clients.builder(server.httpUri(ThriftSerializationFormats.BINARY).resolve("/default-names"))
140+
.build(HelloService.Iface.class);
141+
client.hello("hello");
142+
143+
await().untilAsserted(() -> {
144+
verify(appender, atLeast(0)).doAppend(eventCaptor.capture());
145+
assertThat(eventCaptor.getAllValues()).anyMatch(evt -> {
146+
return evt.getMessage().contains("POST /default-names#HelloService/defaultName h2c");
147+
});
148+
});
149+
}
150+
116151
@Test
117152
void logNameOfClientSide() throws TException {
118153
final HelloService.Iface client =

0 commit comments

Comments
 (0)