Skip to content

Commit 33c3917

Browse files
committed
core,opentelemetry: Fix early-close metric labels
Resolve generated-method classification before serverCallStarted() so close metrics do not fall back to "other" when streamClosed() happens first. Keep fallback registry lookup on the existing async path and avoid tracer-side HandlerRegistry access to match the maintainer constraints from issue #12117. Add regressions for primary generated, primary non-generated, and fallback-generated server paths, plus the tracer-level early-resolution contract.
1 parent b38df6c commit 33c3917

5 files changed

Lines changed: 323 additions & 18 deletions

File tree

core/src/main/java/io/grpc/internal/ServerImpl.java

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public final class ServerImpl extends io.grpc.Server implements InternalInstrume
9999
private final ObjectPool<? extends Executor> executorPool;
100100
/** Executor for application processing. Safe to read after {@link #start()}. */
101101
private Executor executor;
102-
private final HandlerRegistry registry;
102+
private final InternalHandlerRegistry registry;
103103
private final HandlerRegistry fallbackRegistry;
104104
private final List<ServerTransportFilter> transportFilters;
105105
// This is iterated on a per-call basis. Use an array instead of a Collection to avoid iterator
@@ -498,8 +498,12 @@ private void streamCreatedInternal(
498498

499499
final StatsTraceContext statsTraceCtx = Preconditions.checkNotNull(
500500
stream.statsTraceContext(), "statsTraceCtx not present from stream");
501+
final ServerMethodDefinition<?, ?> primaryMethod = registry.lookupMethod(methodName, null);
501502

502503
final Context.CancellableContext context = createContext(headers, statsTraceCtx);
504+
if (primaryMethod != null) {
505+
statsTraceCtx.serverCallMethodResolved(primaryMethod.getMethodDescriptor());
506+
}
503507

504508
final Link link = PerfMark.linkOut();
505509

@@ -536,7 +540,7 @@ private void runInternal() {
536540
ServerMethodDefinition<?, ?> wrapMethod;
537541
ServerCallParameters<?, ?> callParams;
538542
try {
539-
ServerMethodDefinition<?, ?> method = registry.lookupMethod(methodName);
543+
ServerMethodDefinition<?, ?> method = primaryMethod;
540544
if (method == null) {
541545
method = fallbackRegistry.lookupMethod(methodName, stream.getAuthority());
542546
}
@@ -554,7 +558,12 @@ private void runInternal() {
554558
future.cancel(false);
555559
return;
556560
}
557-
wrapMethod = wrapMethod(stream, method, statsTraceCtx);
561+
statsTraceCtx.serverCallStarted(
562+
new ServerCallInfoImpl<>(
563+
method.getMethodDescriptor(), // notify with original method descriptor
564+
stream.getAttributes(),
565+
stream.getAuthority()));
566+
wrapMethod = wrapMethod(method);
558567
callParams = maySwitchExecutor(wrapMethod, stream, headers, context, tag);
559568
future.set(callParams);
560569
} catch (Throwable t) {
@@ -653,14 +662,8 @@ private Context.CancellableContext createContext(
653662
}
654663

655664
/** Never returns {@code null}. */
656-
private <ReqT, RespT> ServerMethodDefinition<?,?> wrapMethod(ServerStream stream,
657-
ServerMethodDefinition<ReqT, RespT> methodDef, StatsTraceContext statsTraceCtx) {
658-
// TODO(ejona86): should we update fullMethodName to have the canonical path of the method?
659-
statsTraceCtx.serverCallStarted(
660-
new ServerCallInfoImpl<>(
661-
methodDef.getMethodDescriptor(), // notify with original method descriptor
662-
stream.getAttributes(),
663-
stream.getAuthority()));
665+
private <ReqT, RespT> ServerMethodDefinition<?,?> wrapMethod(
666+
ServerMethodDefinition<ReqT, RespT> methodDef) {
664667
ServerCallHandler<ReqT, RespT> handler = methodDef.getServerCallHandler();
665668
for (ServerInterceptor interceptor : interceptors) {
666669
handler = InternalServerInterceptors.interceptCallHandlerCreate(interceptor, handler);

core/src/main/java/io/grpc/internal/StatsTraceContext.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import io.grpc.ClientStreamTracer;
2424
import io.grpc.Context;
2525
import io.grpc.Metadata;
26+
import io.grpc.MethodDescriptor;
2627
import io.grpc.ServerStreamTracer;
2728
import io.grpc.ServerStreamTracer.ServerCallInfo;
2829
import io.grpc.Status;
@@ -38,6 +39,14 @@
3839
*/
3940
@ThreadSafe
4041
public final class StatsTraceContext {
42+
/**
43+
* Internal hook for server tracers that can use the resolved method descriptor before
44+
* {@link ServerStreamTracer#serverCallStarted(ServerCallInfo)} runs.
45+
*/
46+
public interface ServerCallMethodListener {
47+
void serverCallMethodResolved(MethodDescriptor<?, ?> method);
48+
}
49+
4150
public static final StatsTraceContext NOOP = new StatsTraceContext(new StreamTracer[0]);
4251

4352
private final StreamTracer[] tracers;
@@ -144,6 +153,20 @@ public void serverCallStarted(ServerCallInfo<?, ?> callInfo) {
144153
}
145154
}
146155

156+
/**
157+
* Notifies server tracers that a primary-registry method descriptor was resolved before
158+
* {@link ServerStreamTracer#serverCallStarted(ServerCallInfo)}.
159+
*
160+
* <p>Called from {@link io.grpc.internal.ServerImpl}.
161+
*/
162+
public void serverCallMethodResolved(MethodDescriptor<?, ?> method) {
163+
for (StreamTracer tracer : tracers) {
164+
if (tracer instanceof ServerCallMethodListener) {
165+
((ServerCallMethodListener) tracer).serverCallMethodResolved(method);
166+
}
167+
}
168+
}
169+
147170
/**
148171
* See {@link StreamTracer#streamClosed}. This may be called multiple times, and only the first
149172
* value will be taken.

core/src/test/java/io/grpc/internal/ServerImplTest.java

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,10 @@ public class ServerImplTest {
129129
.setRequestMarshaller(STRING_MARSHALLER)
130130
.setResponseMarshaller(INTEGER_MARSHALLER)
131131
.build();
132+
private static final MethodDescriptor<String, Integer> GENERATED_METHOD =
133+
METHOD.toBuilder()
134+
.setSampledToLocalTracing(true)
135+
.build();
132136
private static final Context.Key<String> SERVER_ONLY = Context.key("serverOnly");
133137
private static final Context.Key<String> SERVER_TRACER_ADDED_KEY = Context.key("tracer-added");
134138
private static final Context.CancellableContext SERVER_CONTEXT =
@@ -142,6 +146,60 @@ public boolean shouldAccept(Runnable runnable) {
142146
};
143147
private static final String AUTHORITY = "some_authority";
144148

149+
private static final class MethodNameCapturingTracer extends ServerStreamTracer
150+
implements StatsTraceContext.ServerCallMethodListener {
151+
@Nullable private ServerCallInfo<?, ?> serverCallInfo;
152+
@Nullable private String recordedMethodName;
153+
@Nullable private String resolvedMethodName;
154+
private boolean streamClosed;
155+
156+
@Override
157+
public synchronized void serverCallMethodResolved(MethodDescriptor<?, ?> method) {
158+
resolvedMethodName =
159+
recordMethodName(method.isSampledToLocalTracing(), method.getFullMethodName());
160+
}
161+
162+
@Override
163+
public synchronized void streamClosed(Status status) {
164+
streamClosed = true;
165+
if (serverCallInfo != null) {
166+
recordedMethodName =
167+
recordMethodName(
168+
serverCallInfo.getMethodDescriptor().isSampledToLocalTracing(),
169+
serverCallInfo.getMethodDescriptor().getFullMethodName());
170+
} else if (resolvedMethodName != null) {
171+
recordedMethodName = resolvedMethodName;
172+
} else {
173+
recordedMethodName = "other";
174+
}
175+
}
176+
177+
@Override
178+
public synchronized void serverCallStarted(ServerCallInfo<?, ?> callInfo) {
179+
serverCallInfo = callInfo;
180+
if (streamClosed) {
181+
recordedMethodName =
182+
recordMethodName(
183+
callInfo.getMethodDescriptor().isSampledToLocalTracing(),
184+
callInfo.getMethodDescriptor().getFullMethodName());
185+
}
186+
}
187+
188+
@Nullable
189+
synchronized ServerCallInfo<?, ?> getServerCallInfo() {
190+
return serverCallInfo;
191+
}
192+
193+
@Nullable
194+
synchronized String getRecordedMethodName() {
195+
return recordedMethodName;
196+
}
197+
198+
private static String recordMethodName(boolean generatedMethod, String fullMethodName) {
199+
return generatedMethod ? fullMethodName : "other";
200+
}
201+
}
202+
145203
@Rule public final MockitoRule mocks = MockitoJUnit.rule();
146204

147205
@BeforeClass
@@ -462,6 +520,172 @@ public void methodNotFound() throws Exception {
462520
assertEquals(Status.Code.UNIMPLEMENTED, statusCaptor.getValue().getCode());
463521
}
464522

523+
@Test
524+
public void primaryRegistryGeneratedMethod_streamClosedBeforeStart_preservesMethodName()
525+
throws Exception {
526+
MethodNameCapturingTracer methodNameTracer = new MethodNameCapturingTracer();
527+
streamTracerFactories =
528+
Collections.singletonList(
529+
new ServerStreamTracer.Factory() {
530+
@Override
531+
public ServerStreamTracer newServerStreamTracer(
532+
String fullMethodName, Metadata headers) {
533+
return methodNameTracer;
534+
}
535+
});
536+
builder.addService(
537+
ServerServiceDefinition.builder(new ServiceDescriptor("Waiter", GENERATED_METHOD))
538+
.addMethod(
539+
GENERATED_METHOD,
540+
new ServerCallHandler<String, Integer>() {
541+
@Override
542+
public ServerCall.Listener<String> startCall(
543+
ServerCall<String, Integer> call, Metadata headers) {
544+
return callListener;
545+
}
546+
})
547+
.build());
548+
549+
createAndStartServer();
550+
ServerTransportListener transportListener
551+
= transportServer.registerNewServerTransport(new SimpleServerTransport());
552+
transportListener.transportReady(Attributes.EMPTY);
553+
Metadata requestHeaders = new Metadata();
554+
StatsTraceContext statsTraceCtx =
555+
StatsTraceContext.newServerContext(
556+
streamTracerFactories, GENERATED_METHOD.getFullMethodName(), requestHeaders);
557+
when(stream.getAttributes()).thenReturn(Attributes.EMPTY);
558+
when(stream.statsTraceContext()).thenReturn(statsTraceCtx);
559+
560+
transportListener.streamCreated(stream, GENERATED_METHOD.getFullMethodName(), requestHeaders);
561+
verify(stream).setListener(isA(ServerStreamListener.class));
562+
verify(stream, atLeast(1)).statsTraceContext();
563+
564+
statsTraceCtx.streamClosed(Status.CANCELLED);
565+
assertNull(methodNameTracer.getServerCallInfo());
566+
assertEquals(
567+
GENERATED_METHOD.getFullMethodName(),
568+
methodNameTracer.getRecordedMethodName());
569+
570+
assertEquals(1, executor.runDueTasks());
571+
572+
assertNotNull(methodNameTracer.getServerCallInfo());
573+
assertSame(GENERATED_METHOD, methodNameTracer.getServerCallInfo().getMethodDescriptor());
574+
assertEquals(
575+
GENERATED_METHOD.getFullMethodName(),
576+
methodNameTracer.getRecordedMethodName());
577+
verify(fallbackRegistry, never()).lookupMethod(anyString(), any());
578+
}
579+
580+
@Test
581+
public void primaryRegistryNonGeneratedMethod_streamClosedBeforeStart_recordsOther()
582+
throws Exception {
583+
MethodNameCapturingTracer methodNameTracer = new MethodNameCapturingTracer();
584+
streamTracerFactories =
585+
Collections.singletonList(
586+
new ServerStreamTracer.Factory() {
587+
@Override
588+
public ServerStreamTracer newServerStreamTracer(
589+
String fullMethodName, Metadata headers) {
590+
return methodNameTracer;
591+
}
592+
});
593+
builder.addService(
594+
ServerServiceDefinition.builder(new ServiceDescriptor("Waiter", METHOD))
595+
.addMethod(
596+
METHOD,
597+
new ServerCallHandler<String, Integer>() {
598+
@Override
599+
public ServerCall.Listener<String> startCall(
600+
ServerCall<String, Integer> call, Metadata headers) {
601+
return callListener;
602+
}
603+
})
604+
.build());
605+
606+
createAndStartServer();
607+
ServerTransportListener transportListener
608+
= transportServer.registerNewServerTransport(new SimpleServerTransport());
609+
transportListener.transportReady(Attributes.EMPTY);
610+
Metadata requestHeaders = new Metadata();
611+
StatsTraceContext statsTraceCtx =
612+
StatsTraceContext.newServerContext(
613+
streamTracerFactories, METHOD.getFullMethodName(), requestHeaders);
614+
when(stream.getAttributes()).thenReturn(Attributes.EMPTY);
615+
when(stream.statsTraceContext()).thenReturn(statsTraceCtx);
616+
617+
transportListener.streamCreated(stream, METHOD.getFullMethodName(), requestHeaders);
618+
verify(stream).setListener(isA(ServerStreamListener.class));
619+
verify(stream, atLeast(1)).statsTraceContext();
620+
621+
statsTraceCtx.streamClosed(Status.CANCELLED);
622+
assertNull(methodNameTracer.getServerCallInfo());
623+
assertEquals("other", methodNameTracer.getRecordedMethodName());
624+
625+
assertEquals(1, executor.runDueTasks());
626+
627+
assertNotNull(methodNameTracer.getServerCallInfo());
628+
assertSame(METHOD, methodNameTracer.getServerCallInfo().getMethodDescriptor());
629+
assertEquals("other", methodNameTracer.getRecordedMethodName());
630+
verify(fallbackRegistry, never()).lookupMethod(anyString(), any());
631+
}
632+
633+
@Test
634+
public void fallbackRegistryGeneratedMethod_streamClosedBeforeStart_resolvesOnAsyncLookup()
635+
throws Exception {
636+
MethodNameCapturingTracer methodNameTracer = new MethodNameCapturingTracer();
637+
streamTracerFactories =
638+
Collections.singletonList(
639+
new ServerStreamTracer.Factory() {
640+
@Override
641+
public ServerStreamTracer newServerStreamTracer(
642+
String fullMethodName, Metadata headers) {
643+
return methodNameTracer;
644+
}
645+
});
646+
mutableFallbackRegistry.addService(
647+
ServerServiceDefinition.builder(new ServiceDescriptor("Waiter", GENERATED_METHOD))
648+
.addMethod(
649+
GENERATED_METHOD,
650+
new ServerCallHandler<String, Integer>() {
651+
@Override
652+
public ServerCall.Listener<String> startCall(
653+
ServerCall<String, Integer> call, Metadata headers) {
654+
return callListener;
655+
}
656+
})
657+
.build());
658+
659+
createAndStartServer();
660+
ServerTransportListener transportListener
661+
= transportServer.registerNewServerTransport(new SimpleServerTransport());
662+
transportListener.transportReady(Attributes.EMPTY);
663+
Metadata requestHeaders = new Metadata();
664+
StatsTraceContext statsTraceCtx =
665+
StatsTraceContext.newServerContext(
666+
streamTracerFactories, GENERATED_METHOD.getFullMethodName(), requestHeaders);
667+
when(stream.getAttributes()).thenReturn(Attributes.EMPTY);
668+
when(stream.statsTraceContext()).thenReturn(statsTraceCtx);
669+
670+
transportListener.streamCreated(stream, GENERATED_METHOD.getFullMethodName(), requestHeaders);
671+
verify(stream).setListener(isA(ServerStreamListener.class));
672+
verify(stream, atLeast(1)).statsTraceContext();
673+
674+
statsTraceCtx.streamClosed(Status.CANCELLED);
675+
assertNull(methodNameTracer.getServerCallInfo());
676+
assertEquals("other", methodNameTracer.getRecordedMethodName());
677+
verify(fallbackRegistry, never()).lookupMethod(anyString(), any());
678+
679+
assertEquals(1, executor.runDueTasks());
680+
681+
assertNotNull(methodNameTracer.getServerCallInfo());
682+
assertSame(GENERATED_METHOD, methodNameTracer.getServerCallInfo().getMethodDescriptor());
683+
assertEquals(
684+
GENERATED_METHOD.getFullMethodName(),
685+
methodNameTracer.getRecordedMethodName());
686+
verify(fallbackRegistry).lookupMethod(GENERATED_METHOD.getFullMethodName(), AUTHORITY);
687+
}
688+
465689

466690
@Test
467691
public void executorSupplierSameExecutorBasic() throws Exception {

0 commit comments

Comments
 (0)