From 4082b2d0d70998968a858ba8146e7ce4ab543cbc Mon Sep 17 00:00:00 2001 From: agravator Date: Fri, 6 Mar 2026 16:12:07 +0530 Subject: [PATCH 01/11] feat: Implement TCP metrics collection and pipeline MetricRecorder (Address PR comments) --- .../main/java/io/grpc/InternalTcpMetrics.java | 102 ++++ api/src/main/java/io/grpc/NameResolver.java | 3 +- .../grpc/internal/ClientTransportFactory.java | 12 + .../java/io/grpc/internal/InternalServer.java | 6 +- .../io/grpc/internal/InternalSubchannel.java | 3 + .../java/io/grpc/internal/ServerImpl.java | 1 + .../io/grpc/internal/ServerImplBuilder.java | 20 +- .../grpc/internal/ServerImplBuilderTest.java | 9 +- .../java/io/grpc/internal/ServerImplTest.java | 3 +- .../inprocess/InProcessServerBuilder.java | 3 +- .../io/grpc/netty/NettyChannelBuilder.java | 1 + .../io/grpc/netty/NettyClientHandler.java | 26 +- .../io/grpc/netty/NettyClientTransport.java | 7 +- .../main/java/io/grpc/netty/NettyServer.java | 18 +- .../io/grpc/netty/NettyServerBuilder.java | 12 +- .../io/grpc/netty/NettyServerHandler.java | 19 +- .../io/grpc/netty/NettyServerTransport.java | 9 +- .../main/java/io/grpc/netty/TcpMetrics.java | 271 +++++++++ .../io/grpc/netty/NettyClientHandlerTest.java | 3 +- .../grpc/netty/NettyClientTransportTest.java | 6 +- .../io/grpc/netty/NettyServerBuilderTest.java | 16 +- .../io/grpc/netty/NettyServerHandlerTest.java | 3 +- .../java/io/grpc/netty/NettyServerTest.java | 14 +- .../io/grpc/netty/NettyTransportTest.java | 6 +- .../grpc/netty/ProtocolNegotiatorsTest.java | 5 +- .../java/io/grpc/netty/TcpMetricsTest.java | 552 ++++++++++++++++++ .../okhttp/InternalOkHttpServerBuilder.java | 5 +- .../io/grpc/okhttp/OkHttpServerBuilder.java | 3 +- .../io/grpc/okhttp/OkHttpTransportTest.java | 8 +- .../io/grpc/servlet/JettyTransportTest.java | 4 +- .../io/grpc/servlet/ServletServerBuilder.java | 3 +- .../io/grpc/servlet/TomcatTransportTest.java | 5 +- .../grpc/servlet/UndertowTransportTest.java | 4 +- 33 files changed, 1106 insertions(+), 56 deletions(-) create mode 100644 api/src/main/java/io/grpc/InternalTcpMetrics.java create mode 100644 netty/src/main/java/io/grpc/netty/TcpMetrics.java create mode 100644 netty/src/test/java/io/grpc/netty/TcpMetricsTest.java diff --git a/api/src/main/java/io/grpc/InternalTcpMetrics.java b/api/src/main/java/io/grpc/InternalTcpMetrics.java new file mode 100644 index 00000000000..6ddc06b6946 --- /dev/null +++ b/api/src/main/java/io/grpc/InternalTcpMetrics.java @@ -0,0 +1,102 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +/** + * TCP Metrics defined to be shared across transport implementations. + */ +@Internal +public final class InternalTcpMetrics { + + private InternalTcpMetrics() { + } + + private static final List OPTIONAL_LABELS = Arrays.asList( + "network.local.address", + "network.local.port", + "network.peer.address", + "network.peer.port"); + + public static final DoubleHistogramMetricInstrument MIN_RTT_INSTRUMENT = + MetricInstrumentRegistry.getDefaultRegistry() + .registerDoubleHistogram( + "grpc.tcp.min_rtt", + "Minimum round-trip time of a TCP connection", + "s", + getMinRttBuckets(), + Collections.emptyList(), + OPTIONAL_LABELS, + false); + + public static final LongCounterMetricInstrument CONNECTIONS_CREATED_INSTRUMENT = + MetricInstrumentRegistry + .getDefaultRegistry() + .registerLongCounter( + "grpc.tcp.connections_created", + "The total number of TCP connections established.", + "{connection}", + Collections.emptyList(), + OPTIONAL_LABELS, + false); + + public static final LongUpDownCounterMetricInstrument CONNECTION_COUNT_INSTRUMENT = + MetricInstrumentRegistry + .getDefaultRegistry() + .registerLongUpDownCounter( + "grpc.tcp.connection_count", + "The current number of active TCP connections.", + "{connection}", + Collections.emptyList(), + OPTIONAL_LABELS, + false); + + public static final LongCounterMetricInstrument PACKETS_RETRANSMITTED_INSTRUMENT = + MetricInstrumentRegistry + .getDefaultRegistry() + .registerLongCounter( + "grpc.tcp.packets_retransmitted", + "The total number of packets retransmitted for all TCP connections.", + "{packet}", + Collections.emptyList(), + OPTIONAL_LABELS, + false); + + public static final LongCounterMetricInstrument RECURRING_RETRANSMITS_INSTRUMENT = + MetricInstrumentRegistry + .getDefaultRegistry() + .registerLongCounter( + "grpc.tcp.recurring_retransmits", + "The total number of times the retransmit timer popped for all TCP" + + " connections.", + "{timeout}", + Collections.emptyList(), + OPTIONAL_LABELS, + false); + + private static List getMinRttBuckets() { + List buckets = new ArrayList<>(100); + for (int i = 1; i <= 100; i++) { + buckets.add(1e-6 * Math.pow(2.0, i * 0.24)); + } + return Collections.unmodifiableList(buckets); + } +} diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 53dbc5d6888..3494eab93d0 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -355,7 +355,7 @@ public static final class Args { @Nullable private final ChannelLogger channelLogger; @Nullable private final Executor executor; @Nullable private final String overrideAuthority; - @Nullable private final MetricRecorder metricRecorder; + private final MetricRecorder metricRecorder; @Nullable private final NameResolverRegistry nameResolverRegistry; @Nullable private final IdentityHashMap, Object> customArgs; @@ -497,7 +497,6 @@ public String getOverrideAuthority() { /** * Returns the {@link MetricRecorder} that the channel uses to record metrics. */ - @Nullable public MetricRecorder getMetricRecorder() { return metricRecorder; } diff --git a/core/src/main/java/io/grpc/internal/ClientTransportFactory.java b/core/src/main/java/io/grpc/internal/ClientTransportFactory.java index 6c10ced4652..dffd18ae5c7 100644 --- a/core/src/main/java/io/grpc/internal/ClientTransportFactory.java +++ b/core/src/main/java/io/grpc/internal/ClientTransportFactory.java @@ -24,6 +24,7 @@ import io.grpc.ChannelCredentials; import io.grpc.ChannelLogger; import io.grpc.HttpConnectProxiedSocketAddress; +import io.grpc.MetricRecorder; import java.io.Closeable; import java.net.SocketAddress; import java.util.Collection; @@ -91,6 +92,7 @@ final class ClientTransportOptions { private Attributes eagAttributes = Attributes.EMPTY; @Nullable private String userAgent; @Nullable private HttpConnectProxiedSocketAddress connectProxiedSocketAddr; + @Nullable private MetricRecorder metricRecorder; public ChannelLogger getChannelLogger() { return channelLogger; @@ -101,6 +103,16 @@ public ClientTransportOptions setChannelLogger(ChannelLogger channelLogger) { return this; } + @Nullable + public MetricRecorder getMetricRecorder() { + return metricRecorder; + } + + public ClientTransportOptions setMetricRecorder(@Nullable MetricRecorder metricRecorder) { + this.metricRecorder = metricRecorder; + return this; + } + public String getAuthority() { return authority; } diff --git a/core/src/main/java/io/grpc/internal/InternalServer.java b/core/src/main/java/io/grpc/internal/InternalServer.java index a6079081233..fdd4f32d0d8 100644 --- a/core/src/main/java/io/grpc/internal/InternalServer.java +++ b/core/src/main/java/io/grpc/internal/InternalServer.java @@ -58,7 +58,8 @@ public interface InternalServer { /** * Returns the first listen socket stats of this server. May return {@code null}. */ - @Nullable InternalInstrumented getListenSocketStats(); + @Nullable + InternalInstrumented getListenSocketStats(); /** * Returns a list of listening socket addresses. May change after {@link #start(ServerListener)} @@ -69,6 +70,7 @@ public interface InternalServer { /** * Returns a list of listen socket stats of this server. May return {@code null}. */ - @Nullable List> getListenSocketStatsList(); + @Nullable + List> getListenSocketStatsList(); } diff --git a/core/src/main/java/io/grpc/internal/InternalSubchannel.java b/core/src/main/java/io/grpc/internal/InternalSubchannel.java index 7a48bf642fe..ce31921e316 100644 --- a/core/src/main/java/io/grpc/internal/InternalSubchannel.java +++ b/core/src/main/java/io/grpc/internal/InternalSubchannel.java @@ -80,6 +80,7 @@ final class InternalSubchannel implements InternalInstrumented, Tr private final InternalChannelz channelz; private final CallTracer callsTracer; private final ChannelTracer channelTracer; + private final MetricRecorder metricRecorder; private final ChannelLogger channelLogger; private final boolean reconnectDisabled; @@ -191,6 +192,7 @@ protected void handleNotInUse() { this.scheduledExecutor = scheduledExecutor; this.connectingTimer = stopwatchSupplier.get(); this.syncContext = syncContext; + this.metricRecorder = metricRecorder; this.callback = callback; this.channelz = channelz; this.callsTracer = callsTracer; @@ -265,6 +267,7 @@ private void startNewTransport() { .setAuthority(eagChannelAuthority != null ? eagChannelAuthority : authority) .setEagAttributes(currentEagAttributes) .setUserAgent(userAgent) + .setMetricRecorder(metricRecorder) .setHttpConnectProxiedSocketAddress(proxiedAddr); TransportLogger transportLogger = new TransportLogger(); // In case the transport logs in the constructor, use the subchannel logId diff --git a/core/src/main/java/io/grpc/internal/ServerImpl.java b/core/src/main/java/io/grpc/internal/ServerImpl.java index dc0709e1fb8..20e4a06ac38 100644 --- a/core/src/main/java/io/grpc/internal/ServerImpl.java +++ b/core/src/main/java/io/grpc/internal/ServerImpl.java @@ -143,6 +143,7 @@ public final class ServerImpl extends io.grpc.Server implements InternalInstrume InternalServer transportServer, Context rootContext) { this.executorPool = Preconditions.checkNotNull(builder.executorPool, "executorPool"); + this.registry = Preconditions.checkNotNull(builder.registryBuilder.build(), "registryBuilder"); this.fallbackRegistry = Preconditions.checkNotNull(builder.fallbackRegistry, "fallbackRegistry"); diff --git a/core/src/main/java/io/grpc/internal/ServerImplBuilder.java b/core/src/main/java/io/grpc/internal/ServerImplBuilder.java index f6566e067db..c61fc53a3db 100644 --- a/core/src/main/java/io/grpc/internal/ServerImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ServerImplBuilder.java @@ -31,6 +31,9 @@ import io.grpc.HandlerRegistry; import io.grpc.InternalChannelz; import io.grpc.InternalConfiguratorRegistry; +import io.grpc.MetricInstrumentRegistry; +import io.grpc.MetricRecorder; +import io.grpc.MetricSink; import io.grpc.Server; import io.grpc.ServerBuilder; import io.grpc.ServerCallExecutorSupplier; @@ -80,6 +83,7 @@ public static ServerBuilder forPort(int port) { final List transportFilters = new ArrayList<>(); final List interceptors = new ArrayList<>(); private final List streamTracerFactories = new ArrayList<>(); + final List metricSinks = new ArrayList<>(); private final ClientTransportServersBuilder clientTransportServersBuilder; HandlerRegistry fallbackRegistry = DEFAULT_FALLBACK_REGISTRY; ObjectPool executorPool = DEFAULT_EXECUTOR_POOL; @@ -104,7 +108,8 @@ public static ServerBuilder forPort(int port) { */ public interface ClientTransportServersBuilder { InternalServer buildClientTransportServers( - List streamTracerFactories); + List streamTracerFactories, + MetricRecorder metricRecorder); } /** @@ -157,6 +162,14 @@ public ServerImplBuilder intercept(ServerInterceptor interceptor) { return this; } + /** + * Adds a MetricSink to the server. + */ + public ServerImplBuilder addMetricSink(MetricSink metricSink) { + metricSinks.add(checkNotNull(metricSink, "metricSink")); + return this; + } + @Override public ServerImplBuilder addStreamTracerFactory(ServerStreamTracer.Factory factory) { streamTracerFactories.add(checkNotNull(factory, "factory")); @@ -241,8 +254,11 @@ public void setDeadlineTicker(Deadline.Ticker ticker) { @Override public Server build() { + MetricRecorder metricRecorder = new MetricRecorderImpl(metricSinks, + MetricInstrumentRegistry.getDefaultRegistry()); return new ServerImpl(this, - clientTransportServersBuilder.buildClientTransportServers(getTracerFactories()), + clientTransportServersBuilder.buildClientTransportServers( + getTracerFactories(), metricRecorder), Context.ROOT); } diff --git a/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java index 7ad7f15f358..7263c0bb69d 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java @@ -73,7 +73,8 @@ public void setUp() throws Exception { new ClientTransportServersBuilder() { @Override public InternalServer buildClientTransportServers( - List streamTracerFactories) { + List streamTracerFactories, + io.grpc.MetricRecorder metricRecorder) { throw new UnsupportedOperationException(); } }); @@ -139,7 +140,7 @@ public static final class StaticTestingClassLoaderCallsGet implements Runnable { public void run() { ServerImplBuilder builder = new ServerImplBuilder( - streamTracerFactories -> { + (streamTracerFactories, metricRecorder) -> { throw new UnsupportedOperationException(); }); assertThat(builder.getTracerFactories()).hasSize(2); @@ -169,7 +170,7 @@ public void configureServerBuilder(ServerBuilder builder) { })); ServerImplBuilder builder = new ServerImplBuilder( - streamTracerFactories -> { + (streamTracerFactories, metricRecorder) -> { throw new UnsupportedOperationException(); }); assertThat(builder.getTracerFactories()).containsExactly(DUMMY_USER_TRACER); @@ -192,7 +193,7 @@ public void run() { InternalConfiguratorRegistry.setConfigurators(Collections.emptyList()); ServerImplBuilder builder = new ServerImplBuilder( - streamTracerFactories -> { + (streamTracerFactories, metricRecorder) -> { throw new UnsupportedOperationException(); }); assertThat(builder.getTracerFactories()).isEmpty(); diff --git a/core/src/test/java/io/grpc/internal/ServerImplTest.java b/core/src/test/java/io/grpc/internal/ServerImplTest.java index 0f18efe078c..73f1b9bf91b 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplTest.java @@ -206,7 +206,8 @@ public void startUp() throws IOException { new ClientTransportServersBuilder() { @Override public InternalServer buildClientTransportServers( - List streamTracerFactories) { + List streamTracerFactories, + io.grpc.MetricRecorder metricRecorder) { throw new UnsupportedOperationException(); } }); diff --git a/inprocess/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java b/inprocess/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java index 190f67603c3..288ff219b06 100644 --- a/inprocess/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java +++ b/inprocess/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java @@ -120,7 +120,8 @@ private InProcessServerBuilder(SocketAddress listenAddress) { final class InProcessClientTransportServersBuilder implements ClientTransportServersBuilder { @Override public InternalServer buildClientTransportServers( - List streamTracerFactories) { + List streamTracerFactories, + io.grpc.MetricRecorder metricRecorder) { return buildTransportServers(streamTracerFactories); } } diff --git a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java index 258aa15b005..e64f1065681 100644 --- a/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java @@ -856,6 +856,7 @@ public void run() { localSocketPicker, channelLogger, useGetForSafeMethods, + options.getMetricRecorder(), Ticker.systemTicker()); return transport; } diff --git a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java index 8ebf89842ad..5615f5ed75a 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientHandler.java @@ -30,6 +30,7 @@ import io.grpc.InternalChannelz; import io.grpc.InternalStatus; import io.grpc.Metadata; +import io.grpc.MetricRecorder; import io.grpc.Status; import io.grpc.StatusException; import io.grpc.internal.ClientStreamListener.RpcProgress; @@ -123,6 +124,7 @@ class NettyClientHandler extends AbstractNettyHandler { private final Supplier stopwatchFactory; private final TransportTracer transportTracer; private final Attributes eagAttributes; + private final TcpMetrics.Tracker tcpMetrics; private final String authority; private final InUseStateAggregator inUseState = new InUseStateAggregator() { @@ -164,7 +166,8 @@ static NettyClientHandler newHandler( Attributes eagAttributes, String authority, ChannelLogger negotiationLogger, - Ticker ticker) { + Ticker ticker, + MetricRecorder metricRecorder) { Preconditions.checkArgument(maxHeaderListSize > 0, "maxHeaderListSize must be positive"); Http2HeadersDecoder headersDecoder = new GrpcHttp2ClientHeadersDecoder(maxHeaderListSize); Http2FrameReader frameReader = new DefaultHttp2FrameReader(headersDecoder); @@ -194,7 +197,8 @@ static NettyClientHandler newHandler( eagAttributes, authority, negotiationLogger, - ticker); + ticker, + metricRecorder); } @VisibleForTesting @@ -214,7 +218,8 @@ static NettyClientHandler newHandler( Attributes eagAttributes, String authority, ChannelLogger negotiationLogger, - Ticker ticker) { + Ticker ticker, + MetricRecorder metricRecorder) { Preconditions.checkNotNull(connection, "connection"); Preconditions.checkNotNull(frameReader, "frameReader"); Preconditions.checkNotNull(lifecycleManager, "lifecycleManager"); @@ -269,7 +274,8 @@ static NettyClientHandler newHandler( pingCounter, ticker, maxHeaderListSize, - softLimitHeaderListSize); + softLimitHeaderListSize, + metricRecorder); } private NettyClientHandler( @@ -288,7 +294,8 @@ private NettyClientHandler( PingLimiter pingLimiter, Ticker ticker, int maxHeaderListSize, - int softLimitHeaderListSize) { + int softLimitHeaderListSize, + MetricRecorder metricRecorder) { super( /* channelUnused= */ null, decoder, @@ -350,6 +357,7 @@ public void onStreamClosed(Http2Stream stream) { } } }); + this.tcpMetrics = new TcpMetrics.Tracker(metricRecorder); } /** @@ -478,6 +486,7 @@ private void onRstStreamRead(int streamId, long errorCode) { @Override public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exception { + tcpMetrics.recordTcpInfo(ctx.channel()); logger.fine("Network channel being closed by the application."); if (ctx.channel().isActive()) { // Ignore notification that the socket was closed lifecycleManager.notifyShutdown( @@ -490,10 +499,17 @@ public void close(ChannelHandlerContext ctx, ChannelPromise promise) throws Exce /** * Handler for the Channel shutting down. */ + @Override + public void channelActive(ChannelHandlerContext ctx) throws Exception { + tcpMetrics.channelActive(ctx.channel()); + super.channelActive(ctx); + } + @Override public void channelInactive(ChannelHandlerContext ctx) throws Exception { try { logger.fine("Network channel is closed"); + tcpMetrics.channelInactive(ctx.channel()); Status status = Status.UNAVAILABLE.withDescription("Network closed for unknown reason"); lifecycleManager.notifyShutdown(status, SimpleDisconnectError.UNKNOWN); final Status streamStatus; diff --git a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java index 53914b3c877..6585df42df3 100644 --- a/netty/src/main/java/io/grpc/netty/NettyClientTransport.java +++ b/netty/src/main/java/io/grpc/netty/NettyClientTransport.java @@ -34,6 +34,7 @@ import io.grpc.InternalLogId; import io.grpc.Metadata; import io.grpc.MethodDescriptor; +import io.grpc.MetricRecorder; import io.grpc.Status; import io.grpc.internal.ClientStream; import io.grpc.internal.ConnectionClientTransport; @@ -108,6 +109,7 @@ class NettyClientTransport implements ConnectionClientTransport, private final ChannelLogger channelLogger; private final boolean useGetForSafeMethods; private final Ticker ticker; + private final MetricRecorder metricRecorder; NettyClientTransport( @@ -132,6 +134,7 @@ class NettyClientTransport implements ConnectionClientTransport, LocalSocketPicker localSocketPicker, ChannelLogger channelLogger, boolean useGetForSafeMethods, + MetricRecorder metricRecorder, Ticker ticker) { this.negotiator = Preconditions.checkNotNull(negotiator, "negotiator"); @@ -159,6 +162,7 @@ class NettyClientTransport implements ConnectionClientTransport, this.logId = InternalLogId.allocate(getClass(), remoteAddress.toString()); this.channelLogger = Preconditions.checkNotNull(channelLogger, "channelLogger"); this.useGetForSafeMethods = useGetForSafeMethods; + this.metricRecorder = metricRecorder; this.ticker = Preconditions.checkNotNull(ticker, "ticker"); } @@ -251,7 +255,8 @@ public Runnable start(Listener transportListener) { eagAttributes, authorityString, channelLogger, - ticker); + ticker, + metricRecorder); ChannelHandler negotiationHandler = negotiator.newHandler(handler); diff --git a/netty/src/main/java/io/grpc/netty/NettyServer.java b/netty/src/main/java/io/grpc/netty/NettyServer.java index 1cf67ea25ca..80c45b490f8 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServer.java +++ b/netty/src/main/java/io/grpc/netty/NettyServer.java @@ -21,6 +21,7 @@ import static io.netty.channel.ChannelOption.ALLOCATOR; import static io.netty.channel.ChannelOption.SO_KEEPALIVE; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.ListenableFuture; @@ -31,6 +32,7 @@ import io.grpc.InternalInstrumented; import io.grpc.InternalLogId; import io.grpc.InternalWithLogId; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.InternalServer; import io.grpc.internal.ObjectPool; @@ -67,6 +69,7 @@ import java.util.concurrent.Callable; import java.util.logging.Level; import java.util.logging.Logger; +import javax.annotation.Nullable; /** * Netty-based server implementation. @@ -93,6 +96,7 @@ class NettyServer implements InternalServer, InternalWithLogId { private final int maxMessageSize; private final int maxHeaderListSize; private final int softLimitHeaderListSize; + private MetricRecorder metricRecorder; private final long keepAliveTimeInNanos; private final long keepAliveTimeoutInNanos; private final long maxConnectionIdleInNanos; @@ -136,8 +140,10 @@ class NettyServer implements InternalServer, InternalWithLogId { long maxConnectionAgeInNanos, long maxConnectionAgeGraceInNanos, boolean permitKeepAliveWithoutCalls, long permitKeepAliveTimeInNanos, int maxRstCount, long maxRstPeriodNanos, - Attributes eagAttributes, InternalChannelz channelz) { + Attributes eagAttributes, InternalChannelz channelz, + MetricRecorder metricRecorder) { this.addresses = checkNotNull(addresses, "addresses"); + this.metricRecorder = metricRecorder; this.channelFactory = checkNotNull(channelFactory, "channelFactory"); checkNotNull(channelOptions, "channelOptions"); this.channelOptions = new HashMap, Object>(channelOptions); @@ -172,6 +178,13 @@ class NettyServer implements InternalServer, InternalWithLogId { this.channelz = Preconditions.checkNotNull(channelz); this.logId = InternalLogId.allocate(getClass(), addresses.isEmpty() ? "No address" : String.valueOf(addresses)); + this.metricRecorder = metricRecorder; + } + + @VisibleForTesting + @Nullable + MetricRecorder getMetricRecorder() { + return metricRecorder; } @Override @@ -272,7 +285,8 @@ public void initChannel(Channel ch) { permitKeepAliveTimeInNanos, maxRstCount, maxRstPeriodNanos, - eagAttributes); + eagAttributes, + metricRecorder); ServerTransportListener transportListener; // This is to order callbacks on the listener, not to guard access to channel. synchronized (NettyServer.this) { diff --git a/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java b/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java index eb3a6d9b538..4f8500e0ca8 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java @@ -164,8 +164,9 @@ public static NettyServerBuilder forAddress(SocketAddress address, ServerCredent private final class NettyClientTransportServersBuilder implements ClientTransportServersBuilder { @Override public InternalServer buildClientTransportServers( - List streamTracerFactories) { - return buildTransportServers(streamTracerFactories); + List streamTracerFactories, + io.grpc.MetricRecorder metricRecorder) { + return buildTransportServers(streamTracerFactories, metricRecorder); } } @@ -703,8 +704,10 @@ void eagAttributes(Attributes eagAttributes) { this.eagAttributes = checkNotNull(eagAttributes, "eagAttributes"); } + @VisibleForTesting NettyServer buildTransportServers( - List streamTracerFactories) { + List streamTracerFactories, + io.grpc.MetricRecorder metricRecorder) { assertEventLoopsAndChannelType(); ProtocolNegotiator negotiator = protocolNegotiatorFactory.newNegotiator( @@ -737,7 +740,8 @@ NettyServer buildTransportServers( maxRstCount, maxRstPeriodNanos, eagAttributes, - this.serverImplBuilder.getChannelz()); + this.serverImplBuilder.getChannelz(), + metricRecorder); } @VisibleForTesting diff --git a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java index 036fde55e2c..a44fa3ad10d 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java @@ -42,6 +42,7 @@ import io.grpc.InternalMetadata; import io.grpc.InternalStatus; import io.grpc.Metadata; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.Status; import io.grpc.internal.GrpcUtil; @@ -127,6 +128,7 @@ class NettyServerHandler extends AbstractNettyHandler { private final Http2Connection.PropertyKey streamKey; private final ServerTransportListener transportListener; private final int maxMessageSize; + private final TcpMetrics.Tracker tcpMetrics; private final long keepAliveTimeInNanos; private final long keepAliveTimeoutInNanos; private final long maxConnectionAgeInNanos; @@ -174,7 +176,8 @@ static NettyServerHandler newHandler( long permitKeepAliveTimeInNanos, int maxRstCount, long maxRstPeriodNanos, - Attributes eagAttributes) { + Attributes eagAttributes, + MetricRecorder metricRecorder) { Preconditions.checkArgument(maxHeaderListSize > 0, "maxHeaderListSize must be positive: %s", maxHeaderListSize); Http2FrameLogger frameLogger = new Http2FrameLogger(LogLevel.DEBUG, NettyServerHandler.class); @@ -208,7 +211,8 @@ static NettyServerHandler newHandler( maxRstCount, maxRstPeriodNanos, eagAttributes, - Ticker.systemTicker()); + Ticker.systemTicker(), + metricRecorder); } static NettyServerHandler newHandler( @@ -234,7 +238,8 @@ static NettyServerHandler newHandler( int maxRstCount, long maxRstPeriodNanos, Attributes eagAttributes, - Ticker ticker) { + Ticker ticker, + MetricRecorder metricRecorder) { Preconditions.checkArgument(maxStreams > 0, "maxStreams must be positive: %s", maxStreams); Preconditions.checkArgument(flowControlWindow > 0, "flowControlWindow must be positive: %s", flowControlWindow); @@ -294,7 +299,8 @@ static NettyServerHandler newHandler( keepAliveEnforcer, autoFlowControl, rstStreamCounter, - eagAttributes, ticker); + eagAttributes, ticker, + metricRecorder); } private NettyServerHandler( @@ -318,7 +324,8 @@ private NettyServerHandler( boolean autoFlowControl, RstStreamCounter rstStreamCounter, Attributes eagAttributes, - Ticker ticker) { + Ticker ticker, + MetricRecorder metricRecorder) { super( channelUnused, decoder, @@ -362,6 +369,7 @@ public void onStreamClosed(Http2Stream stream) { checkArgument(maxMessageSize >= 0, "maxMessageSize must be non-negative: %s", maxMessageSize); this.maxMessageSize = maxMessageSize; + this.tcpMetrics = new TcpMetrics.Tracker(metricRecorder); this.keepAliveTimeInNanos = keepAliveTimeInNanos; this.keepAliveTimeoutInNanos = keepAliveTimeoutInNanos; this.maxConnectionIdleManager = maxConnectionIdleManager; @@ -663,6 +671,7 @@ void setKeepAliveManagerForTest(KeepAliveManager keepAliveManager) { */ @Override public void channelInactive(ChannelHandlerContext ctx) throws Exception { + tcpMetrics.channelInactive(ctx.channel()); try { if (keepAliveManager != null) { keepAliveManager.onTransportTermination(); diff --git a/netty/src/main/java/io/grpc/netty/NettyServerTransport.java b/netty/src/main/java/io/grpc/netty/NettyServerTransport.java index 758ffeee5b1..c0e52b75876 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerTransport.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerTransport.java @@ -25,6 +25,7 @@ import io.grpc.Attributes; import io.grpc.InternalChannelz.SocketStats; import io.grpc.InternalLogId; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.Status; import io.grpc.internal.ServerTransport; @@ -81,6 +82,7 @@ class NettyServerTransport implements ServerTransport { private final int maxRstCount; private final long maxRstPeriodNanos; private final Attributes eagAttributes; + private final MetricRecorder metricRecorder; private final List streamTracerFactories; private final TransportTracer transportTracer; @@ -105,7 +107,8 @@ class NettyServerTransport implements ServerTransport { long permitKeepAliveTimeInNanos, int maxRstCount, long maxRstPeriodNanos, - Attributes eagAttributes) { + Attributes eagAttributes, + MetricRecorder metricRecorder) { this.channel = Preconditions.checkNotNull(channel, "channel"); this.channelUnused = channelUnused; this.protocolNegotiator = Preconditions.checkNotNull(protocolNegotiator, "protocolNegotiator"); @@ -128,6 +131,7 @@ class NettyServerTransport implements ServerTransport { this.maxRstCount = maxRstCount; this.maxRstPeriodNanos = maxRstPeriodNanos; this.eagAttributes = Preconditions.checkNotNull(eagAttributes, "eagAttributes"); + this.metricRecorder = metricRecorder; SocketAddress remote = channel.remoteAddress(); this.logId = InternalLogId.allocate(getClass(), remote != null ? remote.toString() : null); } @@ -289,6 +293,7 @@ private NettyServerHandler createHandler( permitKeepAliveTimeInNanos, maxRstCount, maxRstPeriodNanos, - eagAttributes); + eagAttributes, + metricRecorder); } } diff --git a/netty/src/main/java/io/grpc/netty/TcpMetrics.java b/netty/src/main/java/io/grpc/netty/TcpMetrics.java new file mode 100644 index 00000000000..d7f8d080d11 --- /dev/null +++ b/netty/src/main/java/io/grpc/netty/TcpMetrics.java @@ -0,0 +1,271 @@ +/* + * Copyright 2026 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.netty; + +import io.grpc.DoubleHistogramMetricInstrument; +import io.grpc.InternalTcpMetrics; +import io.grpc.LongCounterMetricInstrument; +import io.grpc.LongUpDownCounterMetricInstrument; +import io.grpc.MetricRecorder; +import io.netty.channel.Channel; +import io.netty.util.concurrent.ScheduledFuture; +import java.lang.reflect.Method; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.RejectedExecutionException; +import java.util.concurrent.ThreadLocalRandom; +import java.util.concurrent.TimeUnit; +import java.util.logging.Level; +import java.util.logging.Logger; + +/** + * Utility for collecting TCP metrics from Netty channels. + */ +final class TcpMetrics { + private static final Logger log = Logger.getLogger(TcpMetrics.class.getName()); + + private static final Metrics DEFAULT_METRICS; + + static { + boolean epollAvailable = false; + try { + Class epollClass = Class.forName("io.netty.channel.epoll.Epoll"); + Method isAvailableMethod = epollClass.getDeclaredMethod("isAvailable"); + epollAvailable = (Boolean) isAvailableMethod.invoke(null); + } catch (ClassNotFoundException e) { + log.log(Level.FINE, "Epoll is not available", e); + } catch (Exception e) { + log.log(Level.FINE, "Failed to determine Epoll availability", e); + } catch (Error e) { + log.log(Level.FINE, "Failed to load native Epoll library", e); + } + DEFAULT_METRICS = new Metrics(epollAvailable); + } + + static Metrics getDefaultMetrics() { + return DEFAULT_METRICS; + } + + static final class Metrics { + final LongCounterMetricInstrument connectionsCreated; + final LongUpDownCounterMetricInstrument connectionCount; + final LongCounterMetricInstrument packetsRetransmitted; + final LongCounterMetricInstrument recurringRetransmits; + final DoubleHistogramMetricInstrument minRtt; + + Metrics(boolean epollAvailable) { + connectionsCreated = InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT; + connectionCount = InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT; + + if (epollAvailable) { + packetsRetransmitted = InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT; + recurringRetransmits = InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT; + minRtt = InternalTcpMetrics.MIN_RTT_INSTRUMENT; + } else { + packetsRetransmitted = null; + recurringRetransmits = null; + minRtt = null; + } + } + } + + static final class Tracker { + private final MetricRecorder metricRecorder; + private final Metrics metrics; + private final Class epollSocketChannelClass; + private final Method tcpInfoMethod; + private final Method totalRetransMethod; + private final Method retransmitsMethod; + private final Method rttMethod; + private final Object tcpInfo; + + private long lastTotalRetrans = 0; + + Tracker(MetricRecorder metricRecorder) { + this(metricRecorder, DEFAULT_METRICS); + } + + Tracker(MetricRecorder metricRecorder, Metrics metrics) { + this( + metricRecorder, metrics, + "io.netty.channel.epoll.EpollSocketChannel", + "io.netty.channel.epoll.EpollTcpInfo"); + } + + Tracker(MetricRecorder metricRecorder, Metrics metrics, + String epollSocketChannelClassName, String epollTcpInfoClassName) { + this.metricRecorder = metricRecorder; + this.metrics = metrics; + + Class epollSocketChannelClass; + Method tcpInfoMethod; + Object tcpInfo; + Method totalRetransMethod; + Method retransMethod; + Method rttMethod; + + try { + epollSocketChannelClass = Class.forName(epollSocketChannelClassName); + Class epollTcpInfoClass = Class.forName(epollTcpInfoClassName); + tcpInfo = epollTcpInfoClass.getDeclaredConstructor().newInstance(); + tcpInfoMethod = epollSocketChannelClass.getMethod("tcpInfo", epollTcpInfoClass); + totalRetransMethod = epollTcpInfoClass.getMethod("totalRetrans"); + retransMethod = epollTcpInfoClass.getMethod("retrans"); + rttMethod = epollTcpInfoClass.getMethod("rtt"); + } catch (Exception | Error t) { + // Epoll not available or error getting tcp_info, features disabled + log.log(Level.FINE, "Failed to initialize Epoll tcp_info reflection", t); + epollSocketChannelClass = null; + tcpInfoMethod = null; + tcpInfo = null; + totalRetransMethod = null; + retransMethod = null; + rttMethod = null; + } + this.epollSocketChannelClass = epollSocketChannelClass; + this.tcpInfoMethod = tcpInfoMethod; + this.tcpInfo = tcpInfo; + this.totalRetransMethod = totalRetransMethod; + this.retransmitsMethod = retransMethod; + this.rttMethod = rttMethod; + } + + private static final long RECORD_INTERVAL_MILLIS = TimeUnit.MINUTES.toMillis(5); + private ScheduledFuture reportTimer; + + void channelActive(Channel channel) { + if (metricRecorder != null) { + List labelValues = getLabelValues(channel); + metricRecorder.addLongCounter(metrics.connectionsCreated, 1, + Collections.emptyList(), labelValues); + metricRecorder.addLongUpDownCounter(metrics.connectionCount, 1, + Collections.emptyList(), labelValues); + scheduleNextReport(channel, true); + } + } + + private void scheduleNextReport(final Channel channel, boolean isInitial) { + if (!channel.isActive()) { + return; + } + + double jitter = isInitial + ? 0.1 + ThreadLocalRandom.current().nextDouble() // 10% to 110% + : 0.9 + ThreadLocalRandom.current().nextDouble() * 0.2; // 90% to 110% + long rearmingDelay = (long) (RECORD_INTERVAL_MILLIS * jitter); + + try { + reportTimer = channel.eventLoop().schedule(() -> { + if (channel.isActive()) { + Tracker.this.recordTcpInfo(channel, false); + scheduleNextReport(channel, false); // Re-arm + } + }, rearmingDelay, TimeUnit.MILLISECONDS); + } catch (RejectedExecutionException e) { + log.log(Level.FINE, "Failed to schedule next TCP metrics report", e); + // The event loop is likely shutting down. We can safely ignore this. + } + } + + void channelInactive(Channel channel) { + if (reportTimer != null) { + reportTimer.cancel(false); + } + if (metricRecorder != null) { + List labelValues = getLabelValues(channel); + metricRecorder.addLongUpDownCounter(metrics.connectionCount, -1, + Collections.emptyList(), labelValues); + // Final collection on close + recordTcpInfo(channel, true); + } + } + + void recordTcpInfo(Channel channel) { + recordTcpInfo(channel, false); + } + + private void recordTcpInfo(Channel channel, boolean isClose) { + if (metricRecorder == null || epollSocketChannelClass == null + || !epollSocketChannelClass.isInstance(channel)) { + return; + } + List labelValues = getLabelValues(channel); + long totalRetrans; + long retransmits; + long rtt; + try { + tcpInfoMethod.invoke(channel, tcpInfo); + + totalRetrans = (Long) totalRetransMethod.invoke(tcpInfo); + retransmits = (Long) retransmitsMethod.invoke(tcpInfo); + rtt = (Long) rttMethod.invoke(tcpInfo); + } catch (Exception e) { + log.log(Level.FINE, "Error computing TCP metrics", e); + return; + } + + if (metrics.packetsRetransmitted != null) { + long deltaTotal = totalRetrans - lastTotalRetrans; + if (deltaTotal > 0) { + metricRecorder.addLongCounter(metrics.packetsRetransmitted, deltaTotal, + Collections.emptyList(), labelValues); + lastTotalRetrans = totalRetrans; + } + } + if (metrics.recurringRetransmits != null && isClose) { + if (retransmits > 0) { + metricRecorder.addLongCounter(metrics.recurringRetransmits, retransmits, + Collections.emptyList(), labelValues); + } + } + if (metrics.minRtt != null) { + metricRecorder.recordDoubleHistogram(metrics.minRtt, + rtt / 1000000.0, // Convert microseconds to seconds + Collections.emptyList(), labelValues); + } + } + } + + private static List getLabelValues(Channel channel) { + String localAddress = ""; + String localPort = ""; + String peerAddress = ""; + String peerPort = ""; + + SocketAddress local = channel.localAddress(); + if (local instanceof InetSocketAddress) { + InetSocketAddress inetLocal = (InetSocketAddress) local; + localAddress = inetLocal.getAddress().getHostAddress(); + localPort = String.valueOf(inetLocal.getPort()); + } + + SocketAddress remote = channel.remoteAddress(); + if (remote instanceof InetSocketAddress) { + InetSocketAddress inetRemote = (InetSocketAddress) remote; + peerAddress = inetRemote.getAddress().getHostAddress(); + peerPort = String.valueOf(inetRemote.getPort()); + } + + return Arrays.asList(localAddress, localPort, peerAddress, peerPort); + } + + + private TcpMetrics() {} +} diff --git a/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java index 53598727efd..599aecf80b2 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java @@ -1165,7 +1165,8 @@ public Stopwatch get() { Attributes.EMPTY, "someauthority", null, - fakeClock().getTicker()); + fakeClock().getTicker(), + new io.grpc.MetricRecorder() {}); } @Override diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index db44c8f50fd..ea6fef81b06 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -251,6 +251,7 @@ public void setSoLingerChannelOption() throws IOException, GeneralSecurityExcept new SocketPicker(), new FakeChannelLogger(), false, + new io.grpc.MetricRecorder() {} , Ticker.systemTicker()); transports.add(transport); callMeMaybe(transport.start(clientTransportListener)); @@ -526,6 +527,7 @@ public void failingToConstructChannelShouldFailGracefully() throws Exception { new SocketPicker(), new FakeChannelLogger(), false, + new io.grpc.MetricRecorder() {} , Ticker.systemTicker()); transports.add(transport); @@ -1148,6 +1150,7 @@ private NettyClientTransport newTransport(ProtocolNegotiator negotiator, int max new SocketPicker(), new FakeChannelLogger(), false, + new io.grpc.MetricRecorder() {} , Ticker.systemTicker()); transports.add(transport); return transport; @@ -1195,7 +1198,8 @@ private void startServer(int maxStreamsPerConnection, int maxHeaderListSize, MAX_RST_COUNT_DISABLED, 0, Attributes.EMPTY, - channelz); + channelz, + org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); server.start(serverListener); address = TestUtils.testServerAddress((InetSocketAddress) server.getListenSocketAddress()); authority = GrpcUtil.authorityFromHostAndPort(address.getHostString(), address.getPort()); diff --git a/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java index 797cfa95c0e..7999f73dbfd 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java @@ -43,8 +43,10 @@ public class NettyServerBuilderTest { @Test public void addMultipleListenAddresses() { builder.addListenAddress(new InetSocketAddress(8081)); - NettyServer server = - builder.buildTransportServers(ImmutableList.of()); + NettyServer server = builder.buildTransportServers( + ImmutableList.of(), + new io.grpc.MetricRecorder() { + }); assertThat(server.getListenSocketAddresses()).hasSize(2); } @@ -189,4 +191,14 @@ public void useNioTransport_shouldNotThrow() { builder.assertEventLoopsAndChannelType(); } + + @Test + public void metricRecorder_propagatedToServer() throws Exception { + io.grpc.MetricRecorder recorder = mock(io.grpc.MetricRecorder.class); + + NettyServer server = builder.buildTransportServers( + ImmutableList.of(), recorder); + + assertThat(server.getMetricRecorder()).isSameInstanceAs(recorder); + } } diff --git a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java index 0d5a9bab176..a5ab42d15a6 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java @@ -1416,7 +1416,8 @@ protected NettyServerHandler newHandler() { maxRstCount, maxRstPeriodNanos, Attributes.EMPTY, - fakeClock().getTicker()); + fakeClock().getTicker(), + new io.grpc.MetricRecorder() {}); } @Override diff --git a/netty/src/test/java/io/grpc/netty/NettyServerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerTest.java index f9bda4c5af1..65f2fe4d82c 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerTest.java @@ -161,7 +161,7 @@ class NoHandlerProtocolNegotiator implements ProtocolNegotiator { 0, 0, // ignore Attributes.EMPTY, - channelz); + channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); final SettableFuture serverShutdownCalled = SettableFuture.create(); ns.start(new ServerListener() { @Override @@ -218,7 +218,7 @@ public void multiPortStartStopGet() throws Exception { 0, 0, // ignore Attributes.EMPTY, - channelz); + channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); final SettableFuture shutdownCompleted = SettableFuture.create(); ns.start(new ServerListener() { @Override @@ -298,7 +298,7 @@ public void multiPortConnections() throws Exception { 0, 0, // ignore Attributes.EMPTY, - channelz); + channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); final SettableFuture shutdownCompleted = SettableFuture.create(); ns.start(new ServerListener() { @Override @@ -366,7 +366,7 @@ public void getPort_notStarted() { 0, 0, // ignore Attributes.EMPTY, - channelz); + channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); assertThat(ns.getListenSocketAddress()).isEqualTo(addr); assertThat(ns.getListenSocketAddresses()).isEqualTo(addresses); @@ -447,7 +447,7 @@ class TestProtocolNegotiator implements ProtocolNegotiator { 0, 0, // ignore eagAttributes, - channelz); + channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); ns.start(new ServerListener() { @Override public ServerTransportListener transportCreated(ServerTransport transport) { @@ -501,7 +501,7 @@ public void channelzListenSocket() throws Exception { 0, 0, // ignore Attributes.EMPTY, - channelz); + channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); final SettableFuture shutdownCompleted = SettableFuture.create(); ns.start(new ServerListener() { @Override @@ -649,7 +649,7 @@ private NettyServer getServer(List addr, EventLoopGroup ev) { 0, 0, // ignore Attributes.EMPTY, - channelz); + channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); } private static class NoopServerTransportListener implements ServerTransportListener { diff --git a/netty/src/test/java/io/grpc/netty/NettyTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyTransportTest.java index b779dfbe980..d4dcc69bf12 100644 --- a/netty/src/test/java/io/grpc/netty/NettyTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyTransportTest.java @@ -71,7 +71,8 @@ protected InternalServer newServer( .forAddress(new InetSocketAddress("localhost", 0)) .flowControlWindow(AbstractTransportTest.TEST_FLOW_CONTROL_WINDOW) .setTransportTracerFactory(fakeClockTransportTracer) - .buildTransportServers(streamTracerFactories); + .buildTransportServers(streamTracerFactories, new io.grpc.MetricRecorder() { + }); } @Override @@ -81,7 +82,8 @@ protected InternalServer newServer( .forAddress(new InetSocketAddress("localhost", port)) .flowControlWindow(AbstractTransportTest.TEST_FLOW_CONTROL_WINDOW) .setTransportTracerFactory(fakeClockTransportTracer) - .buildTransportServers(streamTracerFactories); + .buildTransportServers(streamTracerFactories, new io.grpc.MetricRecorder() { + }); } @Override diff --git a/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java b/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java index 80438532172..86667c807c5 100644 --- a/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java +++ b/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java @@ -389,7 +389,10 @@ private Object expectHandshake( .buildTransportFactory(); InternalServer server = NettyServerBuilder .forPort(0, serverCreds) - .buildTransportServers(Collections.emptyList()); + .buildTransportServers( + Collections.emptyList(), + new io.grpc.MetricRecorder() { + }); server.start(serverListener); ManagedClientTransport.Listener clientTransportListener = diff --git a/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java b/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java new file mode 100644 index 00000000000..b6d9ca05bde --- /dev/null +++ b/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java @@ -0,0 +1,552 @@ +/* + * Copyright 2026 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.netty; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; + +import io.grpc.MetricRecorder; +import io.netty.channel.Channel; +import io.netty.channel.EventLoop; +import io.netty.util.concurrent.ScheduledFuture; +import java.net.InetAddress; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.concurrent.TimeUnit; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +@RunWith(JUnit4.class) +public class TcpMetricsTest { + + @Rule public final MockitoRule mocks = MockitoJUnit.rule(); + + @Mock private MetricRecorder metricRecorder; + @Mock private Channel channel; + @Mock + private EventLoop eventLoop; + @Mock + private ScheduledFuture scheduledFuture; + + private TcpMetrics.Tracker metrics; + + @Before + public void setUp() { + when(channel.eventLoop()).thenReturn(eventLoop); + when(eventLoop.schedule(any(Runnable.class), anyLong(), any(TimeUnit.class))) + .thenAnswer(invocation -> scheduledFuture); + metrics = new TcpMetrics.Tracker(metricRecorder); + } + + @Test + public void metricsInitialization_epollUnavailable() { + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(false); + + org.junit.Assert.assertNotNull(metrics.connectionsCreated); + org.junit.Assert.assertNotNull(metrics.connectionCount); + org.junit.Assert.assertNull(metrics.packetsRetransmitted); + org.junit.Assert.assertNull(metrics.recurringRetransmits); + org.junit.Assert.assertNull(metrics.minRtt); + } + + @Test + public void metricsInitialization_epollAvailable() { + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + + org.junit.Assert.assertNotNull(metrics.connectionsCreated); + org.junit.Assert.assertNotNull(metrics.connectionCount); + org.junit.Assert.assertNotNull(metrics.packetsRetransmitted); + org.junit.Assert.assertNotNull(metrics.recurringRetransmits); + org.junit.Assert.assertNotNull(metrics.minRtt); + } + + public static class FakeEpollTcpInfo { + long totalRetrans; + long retransmits; + long rtt; + + public void setValues(long totalRetrans, long retransmits, long rtt) { + this.totalRetrans = totalRetrans; + this.retransmits = retransmits; + this.rtt = rtt; + } + + @SuppressWarnings("unused") + public long totalRetrans() { + return totalRetrans; + } + + @SuppressWarnings("unused") + public long retrans() { + return retransmits; + } + + @SuppressWarnings("unused") + public long rtt() { + return rtt; + } + } + + @Test + public void tracker_recordTcpInfo_reflectionSuccess() { + MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, + ConfigurableFakeWithTcpInfo.class.getName(), + FakeEpollTcpInfo.class.getName()); + + FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); + infoSource.setValues(123, 4, 5000); + ConfigurableFakeWithTcpInfo channel = new ConfigurableFakeWithTcpInfo(infoSource); + channel.writeInbound("dummy"); + + tracker.channelInactive(channel); + + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(123L), any(), any()); + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.recurringRetransmits)), + eq(4L), any(), any()); + verify(recorder).recordDoubleHistogram(eq(Objects.requireNonNull(metrics.minRtt)), + eq(0.005), any(), any()); + } + + @Test + public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() { + MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, + ConfigurableFakeWithTcpInfo.class.getName(), + FakeEpollTcpInfo.class.getName()); + + FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); + infoSource.setValues(123, 4, 5000); + ConfigurableFakeWithTcpInfo channel = + org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource)); + when(channel.eventLoop()).thenReturn(eventLoop); + when(channel.isActive()).thenReturn(true); + + tracker.channelActive(channel); + + ArgumentCaptor runnableCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(eventLoop).schedule(runnableCaptor.capture(), anyLong(), eq(TimeUnit.MILLISECONDS)); + Runnable periodicTask = runnableCaptor.getValue(); + + org.mockito.Mockito.clearInvocations(recorder); + periodicTask.run(); + + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(123L), any(), any()); + verify(recorder).recordDoubleHistogram(eq(Objects.requireNonNull(metrics.minRtt)), + eq(0.005), any(), any()); + // Should NOT record recurring retransmits during periodic polling + verify(recorder, org.mockito.Mockito.never()) + .addLongCounter(eq(Objects.requireNonNull(metrics.recurringRetransmits)), + anyLong(), any(), any()); + } + + @Test + public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() { + MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, + ConfigurableFakeWithTcpInfo.class.getName(), + FakeEpollTcpInfo.class.getName()); + + FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); + infoSource.setValues(123, 4, 5000); + ConfigurableFakeWithTcpInfo channel = + org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource)); + when(channel.eventLoop()).thenReturn(eventLoop); + when(channel.isActive()).thenReturn(true); + + // Mimic the periodic schedule invocation + tracker.channelActive(channel); + ArgumentCaptor runnableCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(eventLoop).schedule(runnableCaptor.capture(), anyLong(), eq(TimeUnit.MILLISECONDS)); + + // Fire periodic task once. TotalRetrans=123, retransmits=4. + runnableCaptor.getValue().run(); + + org.mockito.Mockito.clearInvocations(recorder); + + // Let's just create a new channel instance where tcpInfo sets retrans=5. + FakeEpollTcpInfo infoSource2 = new FakeEpollTcpInfo(); + infoSource2.setValues(130, 5, 5000); + ConfigurableFakeWithTcpInfo channel2 = + org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource2)); + when(channel2.eventLoop()).thenReturn(eventLoop); + + tracker.channelInactive(channel2); + + // It should record delta for totalRetrans (130 - 123 = 7) + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(7L), any(), any()); + // But for recurringRetransmits it MUST record the raw value 5, not the delta! + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.recurringRetransmits)), + eq(5L), any(), any()); + } + + @Test + public void tracker_periodicRecord_reportsDeltaForTotalRetrans() { + MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, + ConfigurableFakeWithTcpInfo.class.getName(), + FakeEpollTcpInfo.class.getName()); + + FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); + infoSource.setValues(123, 4, 5000); + ConfigurableFakeWithTcpInfo channel = + org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource)); + when(channel.eventLoop()).thenReturn(eventLoop); + when(channel.isActive()).thenReturn(true); + + // Initial Active Trigger + tracker.channelActive(channel); + ArgumentCaptor runnableCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(eventLoop).schedule(runnableCaptor.capture(), anyLong(), eq(TimeUnit.MILLISECONDS)); + Runnable periodicTask = runnableCaptor.getValue(); + + // First periodic record + org.mockito.Mockito.clearInvocations(recorder); + periodicTask.run(); + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(123L), any(), any()); + + // Change tcpInfo for second periodic record + org.mockito.Mockito.doAnswer(invocation -> { + FakeEpollTcpInfo info = invocation.getArgument(0); + info.totalRetrans = 150; + info.retransmits = 2; // Should not be recorded + info.rtt = 6000; + return null; + }).when(channel).tcpInfo(any(FakeEpollTcpInfo.class)); + + org.mockito.Mockito.clearInvocations(recorder); + periodicTask.run(); + + // Only the delta (150 - 123 = 27) should be recorded + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(27L), any(), any()); + verify(recorder).recordDoubleHistogram(eq(Objects.requireNonNull(metrics.minRtt)), + eq(0.006), any(), any()); + verify(recorder, org.mockito.Mockito.never()) + .addLongCounter(eq(Objects.requireNonNull(metrics.recurringRetransmits)), + anyLong(), any(), any()); + } + + @Test + public void tracker_periodicRecord_doesNotReportZeroDeltaForTotalRetrans() { + MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, + ConfigurableFakeWithTcpInfo.class.getName(), + FakeEpollTcpInfo.class.getName()); + + FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); + infoSource.setValues(123, 4, 5000); + ConfigurableFakeWithTcpInfo channel = + org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource)); + when(channel.eventLoop()).thenReturn(eventLoop); + when(channel.isActive()).thenReturn(true); + + // Initial Active Trigger + tracker.channelActive(channel); + ArgumentCaptor runnableCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(eventLoop).schedule(runnableCaptor.capture(), anyLong(), eq(TimeUnit.MILLISECONDS)); + Runnable periodicTask = runnableCaptor.getValue(); + + // First periodic record + periodicTask.run(); + org.mockito.Mockito.clearInvocations(recorder); + + // Keep tcpInfo the same for second periodic record + periodicTask.run(); + + // NO delta (123 - 123 = 0), so it should not be recorded + verify(recorder, org.mockito.Mockito.never()) + .addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + anyLong(), any(), any()); + verify(recorder).recordDoubleHistogram(eq(Objects.requireNonNull(metrics.minRtt)), + eq(0.005), any(), any()); + } + + public static class ConfigurableFakeWithTcpInfo extends + io.netty.channel.embedded.EmbeddedChannel { + private final FakeEpollTcpInfo infoToCopy; + + public ConfigurableFakeWithTcpInfo(FakeEpollTcpInfo infoToCopy) { + this.infoToCopy = infoToCopy; + } + + public void tcpInfo(FakeEpollTcpInfo info) { + info.totalRetrans = infoToCopy.totalRetrans; + info.retransmits = infoToCopy.retransmits; + info.rtt = infoToCopy.rtt; + } + } + + @Test + public void tracker_reportsDeltas_correctly() { + MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + + String fakeChannelName = ConfigurableFakeWithTcpInfo.class.getName(); + String fakeInfoName = FakeEpollTcpInfo.class.getName(); + + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, + fakeChannelName, fakeInfoName); + + FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); + ConfigurableFakeWithTcpInfo channel = new ConfigurableFakeWithTcpInfo(infoSource); + + // 10 retransmits total + infoSource.setValues(10, 2, 1000); + tracker.recordTcpInfo(channel); + + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(10L), any(), any()); + + // 15 retransmits total (delta 5) + infoSource.setValues(15, 0, 1000); + tracker.recordTcpInfo(channel); + + verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(5L), any(), any()); + + // 15 retransmits total (delta 0) - should NOT report + tracker.recordTcpInfo(channel); + // Verify no new interactions with this specific metric and value + // We can't easily verify "no interaction" for specific value without capturing. + verify(recorder, org.mockito.Mockito.times(1)).addLongCounter( + eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(10L), any(), any()); + verify(recorder, org.mockito.Mockito.times(1)).addLongCounter( + eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + eq(5L), any(), any()); + // Total interactions for packetsRetransmitted should be 2 + verify(recorder, org.mockito.Mockito.times(2)).addLongCounter( + eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + anyLong(), any(), any()); + + // recurringRetransmits should NOT have been reported yet (periodic calls) + verify(recorder, org.mockito.Mockito.times(0)).addLongCounter( + eq(Objects.requireNonNull(metrics.recurringRetransmits)), + anyLong(), any(), any()); + + // Close channel - should report recurringRetransmits + tracker.channelInactive(channel); + verify(recorder, org.mockito.Mockito.times(1)).addLongCounter( + eq(Objects.requireNonNull(metrics.recurringRetransmits)), + eq(0L), // From last infoSource setValues(15, 0, 1000) + any(), any()); + } + + @Test + public void tracker_recordTcpInfo_reflectionFailure() { + MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, + "non.existent.Class", "non.existent.Info"); + + Channel channel = org.mockito.Mockito.mock(Channel.class); + when(channel.isActive()).thenReturn(true); + + // Should catch exception and ignore + tracker.channelInactive(channel); + } + + @Test + public void registeredMetrics_haveCorrectOptionalLabels() { + List expectedOptionalLabels = Arrays.asList( + "network.local.address", + "network.local.port", + "network.peer.address", + "network.peer.port" + ); + + org.junit.Assert.assertEquals( + expectedOptionalLabels, + TcpMetrics.getDefaultMetrics().connectionsCreated.getOptionalLabelKeys()); + org.junit.Assert.assertEquals( + expectedOptionalLabels, + TcpMetrics.getDefaultMetrics().connectionCount.getOptionalLabelKeys()); + + if (TcpMetrics.getDefaultMetrics().packetsRetransmitted != null) { + org.junit.Assert.assertEquals( + expectedOptionalLabels, + Objects.requireNonNull(TcpMetrics.getDefaultMetrics().packetsRetransmitted) + .getOptionalLabelKeys()); + org.junit.Assert.assertEquals( + expectedOptionalLabels, + Objects.requireNonNull(TcpMetrics.getDefaultMetrics().recurringRetransmits) + .getOptionalLabelKeys()); + org.junit.Assert.assertEquals( + expectedOptionalLabels, + Objects.requireNonNull(TcpMetrics.getDefaultMetrics().minRtt).getOptionalLabelKeys()); + } + } + + @Test + public void channelActive_extractsLabels_ipv4() throws Exception { + + InetAddress localInet = InetAddress.getByAddress(new byte[] { 127, 0, 0, 1 }); + InetAddress remoteInet = InetAddress.getByAddress(new byte[] { 10, 0, 0, 1 }); + when(channel.localAddress()).thenReturn(new InetSocketAddress(localInet, 8080)); + when(channel.remoteAddress()).thenReturn(new InetSocketAddress(remoteInet, 443)); + + metrics.channelActive(channel); + + verify(metricRecorder).addLongCounter( + eq(TcpMetrics.getDefaultMetrics().connectionsCreated), eq(1L), + eq(Collections.emptyList()), + eq(Arrays.asList( + localInet.getHostAddress(), "8080", remoteInet.getHostAddress(), "443"))); + verify(metricRecorder).addLongUpDownCounter( + eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(1L), + eq(Collections.emptyList()), + eq(Arrays.asList( + localInet.getHostAddress(), "8080", remoteInet.getHostAddress(), "443"))); + verifyNoMoreInteractions(metricRecorder); + } + + @Test + public void channelInactive_extractsLabels_ipv6() throws Exception { + + InetAddress localInet = InetAddress.getByAddress( + new byte[] { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1 }); + InetAddress remoteInet = InetAddress.getByAddress( + new byte[] { 32, 1, 13, -72, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1 }); + when(channel.localAddress()).thenReturn(new InetSocketAddress(localInet, 8080)); + when(channel.remoteAddress()).thenReturn(new InetSocketAddress(remoteInet, 443)); + + metrics.channelInactive(channel); + + verify(metricRecorder).addLongUpDownCounter( + eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(-1L), + eq(Collections.emptyList()), + eq(Arrays.asList( + localInet.getHostAddress(), "8080", remoteInet.getHostAddress(), "443"))); + verifyNoMoreInteractions(metricRecorder); + } + + @Test + public void channelActive_extractsLabels_nonInetAddress() { + SocketAddress dummyAddress = new SocketAddress() {}; + when(channel.localAddress()).thenReturn(dummyAddress); + when(channel.remoteAddress()).thenReturn(dummyAddress); + + metrics.channelActive(channel); + + verify(metricRecorder).addLongCounter( + eq(TcpMetrics.getDefaultMetrics().connectionsCreated), eq(1L), + eq(Collections.emptyList()), + eq(Arrays.asList("", "", "", ""))); + verify(metricRecorder).addLongUpDownCounter( + eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(1L), + eq(Collections.emptyList()), + eq(Arrays.asList("", "", "", ""))); + verifyNoMoreInteractions(metricRecorder); + } + + @Test + public void channelActive_incrementsCounts() { + metrics.channelActive(channel); + verify(metricRecorder).addLongCounter( + eq(TcpMetrics.getDefaultMetrics().connectionsCreated), eq(1L), + eq(Collections.emptyList()), + eq(Arrays.asList("", "", "", ""))); + verify(metricRecorder).addLongUpDownCounter( + eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(1L), + eq(Collections.emptyList()), + eq(Arrays.asList("", "", "", ""))); + verifyNoMoreInteractions(metricRecorder); + } + + @Test + public void channelInactive_decrementsCount_noEpoll_noError() { + metrics.channelInactive(channel); + verify(metricRecorder).addLongUpDownCounter( + eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(-1L), + eq(Collections.emptyList()), + eq(Arrays.asList("", "", "", ""))); + verifyNoMoreInteractions(metricRecorder); + } + + @Test + public void channelActive_schedulesReportTimer() { + when(channel.isActive()).thenReturn(true); + metrics.channelActive(channel); + + ArgumentCaptor runnableCaptor = ArgumentCaptor.forClass(Runnable.class); + ArgumentCaptor delayCaptor = ArgumentCaptor.forClass(Long.class); + verify(eventLoop).schedule( + runnableCaptor.capture(), delayCaptor.capture(), eq(TimeUnit.MILLISECONDS)); + + Runnable task = runnableCaptor.getValue(); + long delay = delayCaptor.getValue(); + + // Default RECORD_INTERVAL_MILLIS is 5 minutes (300,000 ms) + // Initial jitter is 10% to 110%, so 30,000 ms to 330,000 ms + org.junit.Assert.assertTrue("Delay should be >= 30000 but was " + delay, delay >= 30_000); + org.junit.Assert.assertTrue("Delay should be <= 330000 but was " + delay, delay <= 330_000); + + // Run the task to verify rescheduling + task.run(); + + verify(eventLoop, org.mockito.Mockito.times(2)) + .schedule(any(Runnable.class), delayCaptor.capture(), eq(TimeUnit.MILLISECONDS)); + + // Re-arming jitter is 90% to 110%, so 270,000 ms to 330,000 ms + long rearmDelay = delayCaptor.getValue(); + org.junit.Assert.assertTrue( + "Delay should be >= 270000 but was " + rearmDelay, rearmDelay >= 270_000); + org.junit.Assert.assertTrue( + "Delay should be <= 330000 but was " + rearmDelay, rearmDelay <= 330_000); + } + + @Test + public void channelInactive_cancelsReportTimer() { + when(channel.isActive()).thenReturn(true); + metrics.channelActive(channel); + + metrics.channelInactive(channel); + + verify(scheduledFuture).cancel(false); + } +} diff --git a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java index 78a409a3f85..b89a0487ae8 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java @@ -29,8 +29,9 @@ @Internal public final class InternalOkHttpServerBuilder { public static InternalServer buildTransportServers(OkHttpServerBuilder builder, - List streamTracerFactories) { - return builder.buildTransportServers(streamTracerFactories); + List streamTracerFactories, + io.grpc.MetricRecorder metricRecorder) { + return builder.buildTransportServers(streamTracerFactories, metricRecorder); } public static void setTransportTracerFactory(OkHttpServerBuilder builder, diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java index 8daeed42a8c..c6507fcbc86 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java @@ -387,7 +387,8 @@ void setStatsEnabled(boolean value) { } InternalServer buildTransportServers( - List streamTracerFactories) { + List streamTracerFactories, + io.grpc.MetricRecorder metricRecorder) { return new OkHttpServer(this, streamTracerFactories, serverImplBuilder.getChannelz()); } diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java index 076eea3349a..2cd6370bcc7 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java @@ -58,11 +58,13 @@ protected InternalServer newServer( @Override protected InternalServer newServer( int port, List streamTracerFactories) { - return OkHttpServerBuilder + OkHttpServerBuilder builder = OkHttpServerBuilder .forPort(port, InsecureServerCredentials.create()) .flowControlWindow(AbstractTransportTest.TEST_FLOW_CONTROL_WINDOW) - .setTransportTracerFactory(fakeClockTransportTracer) - .buildTransportServers(streamTracerFactories); + .setTransportTracerFactory(fakeClockTransportTracer); + return InternalOkHttpServerBuilder + .buildTransportServers(builder, streamTracerFactories, new io.grpc.MetricRecorder() { + }); } @Override diff --git a/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java b/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java index 58143a8516c..f3e2d6247ac 100644 --- a/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java +++ b/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java @@ -59,7 +59,9 @@ public class JettyTransportTest extends AbstractTransportTest { protected InternalServer newServer(List streamTracerFactories) { return new InternalServer() { final InternalServer delegate = - new ServletServerBuilder().buildTransportServers(streamTracerFactories); + new ServletServerBuilder().buildTransportServers( + streamTracerFactories, new io.grpc.MetricRecorder() { + }); @Override public void start(ServerListener listener) throws IOException { diff --git a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java index aee25de01ad..1fc007f969e 100644 --- a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java +++ b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java @@ -159,7 +159,8 @@ public void transportTerminated() { @VisibleForTesting InternalServer buildTransportServers( - List streamTracerFactories) { + List streamTracerFactories, + io.grpc.MetricRecorder metricRecorder) { checkNotNull(streamTracerFactories, "streamTracerFactories"); this.streamTracerFactories = streamTracerFactories; internalServer = new InternalServerImpl(); diff --git a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java index cd73b096ccb..20b4fc83833 100644 --- a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java +++ b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java @@ -71,8 +71,11 @@ public void tearDown() throws InterruptedException { @Override protected InternalServer newServer(List streamTracerFactories) { return new InternalServer() { + final ServletServerBuilder builder = new ServletServerBuilder(); final InternalServer delegate = - new ServletServerBuilder().buildTransportServers(streamTracerFactories); + builder.buildTransportServers( + streamTracerFactories, new io.grpc.MetricRecorder() { + }); @Override public void start(ServerListener listener) throws IOException { diff --git a/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java b/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java index ef897c87d70..85f70dd987e 100644 --- a/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java +++ b/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java @@ -91,7 +91,9 @@ protected InternalServer newServer(List streamTracerFactories) { return new InternalServer() { final InternalServer delegate = - new ServletServerBuilder().buildTransportServers(streamTracerFactories); + new ServletServerBuilder().buildTransportServers( + streamTracerFactories, new io.grpc.MetricRecorder() { + }); @Override public void start(ServerListener listener) throws IOException { From a8b66f44c91daa6eedfaf103c81943b5b4b091dc Mon Sep 17 00:00:00 2001 From: agravator Date: Sat, 7 Mar 2026 00:32:40 +0530 Subject: [PATCH 02/11] suggested changes --- .../java/io/grpc/ForwardingServerBuilder.java | 6 +++++ .../main/java/io/grpc/InternalTcpMetrics.java | 14 ++++++----- api/src/main/java/io/grpc/ServerBuilder.java | 10 ++++++++ .../grpc/internal/ClientTransportFactory.java | 8 +++--- .../io/grpc/internal/ServerImplBuilder.java | 1 + .../grpc/internal/ServerImplBuilderTest.java | 12 ++++++++- .../java/io/grpc/internal/ServerImplTest.java | 3 ++- .../inprocess/InProcessServerBuilder.java | 3 ++- .../io/grpc/netty/NettyServerBuilder.java | 8 ++++++ .../io/grpc/netty/NettyServerHandler.java | 6 +++++ .../io/grpc/netty/NettyClientHandlerTest.java | 3 ++- .../grpc/netty/NettyClientTransportTest.java | 13 +++++++--- .../io/grpc/netty/NettyServerBuilderTest.java | 13 +++++----- .../io/grpc/netty/NettyServerHandlerTest.java | 3 ++- .../java/io/grpc/netty/NettyServerTest.java | 15 +++++------ .../io/grpc/netty/NettyTransportTest.java | 7 +++--- .../grpc/netty/ProtocolNegotiatorsTest.java | 4 +-- .../java/io/grpc/netty/TcpMetricsTest.java | 25 +++++++++++-------- .../okhttp/InternalOkHttpServerBuilder.java | 3 ++- .../io/grpc/okhttp/OkHttpServerBuilder.java | 3 ++- .../io/grpc/okhttp/OkHttpTransportTest.java | 4 +-- .../grpc/opentelemetry/GrpcOpenTelemetry.java | 2 +- .../io/grpc/servlet/JettyTransportTest.java | 3 ++- .../io/grpc/servlet/ServletServerBuilder.java | 3 ++- .../io/grpc/servlet/TomcatTransportTest.java | 3 ++- .../grpc/servlet/UndertowTransportTest.java | 3 ++- 26 files changed, 120 insertions(+), 58 deletions(-) diff --git a/api/src/main/java/io/grpc/ForwardingServerBuilder.java b/api/src/main/java/io/grpc/ForwardingServerBuilder.java index 9cef7cfa331..d1f183dd824 100644 --- a/api/src/main/java/io/grpc/ForwardingServerBuilder.java +++ b/api/src/main/java/io/grpc/ForwardingServerBuilder.java @@ -201,6 +201,12 @@ public Server build() { return delegate().build(); } + @Override + public T addMetricSink(MetricSink metricSink) { + delegate().addMetricSink(metricSink); + return thisT(); + } + @Override public String toString() { return MoreObjects.toStringHelper(this).add("delegate", delegate()).toString(); diff --git a/api/src/main/java/io/grpc/InternalTcpMetrics.java b/api/src/main/java/io/grpc/InternalTcpMetrics.java index 6ddc06b6946..dd1048ff765 100644 --- a/api/src/main/java/io/grpc/InternalTcpMetrics.java +++ b/api/src/main/java/io/grpc/InternalTcpMetrics.java @@ -1,5 +1,5 @@ /* - * Copyright 2024 The gRPC Authors + * Copyright 2026 The gRPC Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -27,8 +27,7 @@ @Internal public final class InternalTcpMetrics { - private InternalTcpMetrics() { - } + private InternalTcpMetrics() {} private static final List OPTIONAL_LABELS = Arrays.asList( "network.local.address", @@ -67,7 +66,8 @@ private InternalTcpMetrics() { "{connection}", Collections.emptyList(), OPTIONAL_LABELS, - false); + false + ); public static final LongCounterMetricInstrument PACKETS_RETRANSMITTED_INSTRUMENT = MetricInstrumentRegistry @@ -78,7 +78,8 @@ private InternalTcpMetrics() { "{packet}", Collections.emptyList(), OPTIONAL_LABELS, - false); + false + ); public static final LongCounterMetricInstrument RECURRING_RETRANSMITS_INSTRUMENT = MetricInstrumentRegistry @@ -90,7 +91,8 @@ private InternalTcpMetrics() { "{timeout}", Collections.emptyList(), OPTIONAL_LABELS, - false); + false + ); private static List getMinRttBuckets() { List buckets = new ArrayList<>(100); diff --git a/api/src/main/java/io/grpc/ServerBuilder.java b/api/src/main/java/io/grpc/ServerBuilder.java index cd1cddbb93f..7dbc7bf6077 100644 --- a/api/src/main/java/io/grpc/ServerBuilder.java +++ b/api/src/main/java/io/grpc/ServerBuilder.java @@ -435,6 +435,16 @@ public T setBinaryLog(BinaryLog binaryLog) { */ public abstract Server build(); + /** + * Adds a metric sink to the server. + * + * @param metricSink the metric sink to add. + * @return this + */ + public T addMetricSink(MetricSink metricSink) { + throw new UnsupportedOperationException(); + } + /** * Returns the correctly typed version of the builder. */ diff --git a/core/src/main/java/io/grpc/internal/ClientTransportFactory.java b/core/src/main/java/io/grpc/internal/ClientTransportFactory.java index dffd18ae5c7..6023fb14aa9 100644 --- a/core/src/main/java/io/grpc/internal/ClientTransportFactory.java +++ b/core/src/main/java/io/grpc/internal/ClientTransportFactory.java @@ -92,7 +92,8 @@ final class ClientTransportOptions { private Attributes eagAttributes = Attributes.EMPTY; @Nullable private String userAgent; @Nullable private HttpConnectProxiedSocketAddress connectProxiedSocketAddr; - @Nullable private MetricRecorder metricRecorder; + private MetricRecorder metricRecorder = new MetricRecorder() { + }; public ChannelLogger getChannelLogger() { return channelLogger; @@ -103,13 +104,12 @@ public ClientTransportOptions setChannelLogger(ChannelLogger channelLogger) { return this; } - @Nullable public MetricRecorder getMetricRecorder() { return metricRecorder; } - public ClientTransportOptions setMetricRecorder(@Nullable MetricRecorder metricRecorder) { - this.metricRecorder = metricRecorder; + public ClientTransportOptions setMetricRecorder(MetricRecorder metricRecorder) { + this.metricRecorder = Preconditions.checkNotNull(metricRecorder, "metricRecorder"); return this; } diff --git a/core/src/main/java/io/grpc/internal/ServerImplBuilder.java b/core/src/main/java/io/grpc/internal/ServerImplBuilder.java index c61fc53a3db..62a0e66f314 100644 --- a/core/src/main/java/io/grpc/internal/ServerImplBuilder.java +++ b/core/src/main/java/io/grpc/internal/ServerImplBuilder.java @@ -165,6 +165,7 @@ public ServerImplBuilder intercept(ServerInterceptor interceptor) { /** * Adds a MetricSink to the server. */ + @Override public ServerImplBuilder addMetricSink(MetricSink metricSink) { metricSinks.add(checkNotNull(metricSink, "metricSink")); return this; diff --git a/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java b/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java index 7263c0bb69d..54c2d6ef8b1 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplBuilderTest.java @@ -18,10 +18,13 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; +import static org.mockito.Mockito.mock; import io.grpc.InternalConfigurator; import io.grpc.InternalConfiguratorRegistry; import io.grpc.Metadata; +import io.grpc.MetricRecorder; +import io.grpc.MetricSink; import io.grpc.ServerBuilder; import io.grpc.ServerCall; import io.grpc.ServerCallHandler; @@ -74,7 +77,7 @@ public void setUp() throws Exception { @Override public InternalServer buildClientTransportServers( List streamTracerFactories, - io.grpc.MetricRecorder metricRecorder) { + MetricRecorder metricRecorder) { throw new UnsupportedOperationException(); } }); @@ -129,6 +132,13 @@ public void getTracerFactories_disableBoth() { assertThat(factories).containsExactly(DUMMY_USER_TRACER); } + @Test + public void addMetricSink_addsToSinks() { + MetricSink mockSink = mock(MetricSink.class); + builder.addMetricSink(mockSink); + assertThat(builder.metricSinks).containsExactly(mockSink); + } + @Test public void getTracerFactories_callsGet() throws Exception { Class runnable = classLoader.loadClass(StaticTestingClassLoaderCallsGet.class.getName()); diff --git a/core/src/test/java/io/grpc/internal/ServerImplTest.java b/core/src/test/java/io/grpc/internal/ServerImplTest.java index 73f1b9bf91b..3405cb9bb0c 100644 --- a/core/src/test/java/io/grpc/internal/ServerImplTest.java +++ b/core/src/test/java/io/grpc/internal/ServerImplTest.java @@ -65,6 +65,7 @@ import io.grpc.InternalServerInterceptors; import io.grpc.Metadata; import io.grpc.MethodDescriptor; +import io.grpc.MetricRecorder; import io.grpc.ServerCall; import io.grpc.ServerCall.Listener; import io.grpc.ServerCallExecutorSupplier; @@ -207,7 +208,7 @@ public void startUp() throws IOException { @Override public InternalServer buildClientTransportServers( List streamTracerFactories, - io.grpc.MetricRecorder metricRecorder) { + MetricRecorder metricRecorder) { throw new UnsupportedOperationException(); } }); diff --git a/inprocess/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java b/inprocess/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java index 288ff219b06..b2004426aae 100644 --- a/inprocess/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java +++ b/inprocess/src/main/java/io/grpc/inprocess/InProcessServerBuilder.java @@ -24,6 +24,7 @@ import io.grpc.ExperimentalApi; import io.grpc.ForwardingServerBuilder; import io.grpc.Internal; +import io.grpc.MetricRecorder; import io.grpc.ServerBuilder; import io.grpc.ServerStreamTracer; import io.grpc.internal.FixedObjectPool; @@ -121,7 +122,7 @@ final class InProcessClientTransportServersBuilder implements ClientTransportSer @Override public InternalServer buildClientTransportServers( List streamTracerFactories, - io.grpc.MetricRecorder metricRecorder) { + MetricRecorder metricRecorder) { return buildTransportServers(streamTracerFactories); } } diff --git a/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java b/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java index 4f8500e0ca8..21e1a063700 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerBuilder.java @@ -32,6 +32,7 @@ import io.grpc.ExperimentalApi; import io.grpc.ForwardingServerBuilder; import io.grpc.Internal; +import io.grpc.MetricSink; import io.grpc.ServerBuilder; import io.grpc.ServerCredentials; import io.grpc.ServerStreamTracer; @@ -764,6 +765,13 @@ NettyServerBuilder setTransportTracerFactory(TransportTracer.Factory transportTr return this; } + @CanIgnoreReturnValue + @Override + public NettyServerBuilder addMetricSink(MetricSink metricSink) { + serverImplBuilder.addMetricSink(metricSink); + return this; + } + @CanIgnoreReturnValue @Override public NettyServerBuilder useTransportSecurity(File certChain, File privateKey) { diff --git a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java index a44fa3ad10d..53b0f3e0dfd 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java +++ b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java @@ -669,6 +669,12 @@ void setKeepAliveManagerForTest(KeepAliveManager keepAliveManager) { /** * Handler for the Channel shutting down. */ + @Override + public void channelActive(ChannelHandlerContext ctx) throws Exception { + tcpMetrics.channelActive(ctx.channel()); + super.channelActive(ctx); + } + @Override public void channelInactive(ChannelHandlerContext ctx) throws Exception { tcpMetrics.channelInactive(ctx.channel()); diff --git a/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java index 599aecf80b2..9f6be9a2f3e 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientHandlerTest.java @@ -57,6 +57,7 @@ import io.grpc.Attributes; import io.grpc.CallOptions; import io.grpc.Metadata; +import io.grpc.MetricRecorder; import io.grpc.Status; import io.grpc.internal.AbstractStream; import io.grpc.internal.ClientStreamListener; @@ -1166,7 +1167,7 @@ public Stopwatch get() { "someauthority", null, fakeClock().getTicker(), - new io.grpc.MetricRecorder() {}); + new MetricRecorder() {}); } @Override diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index ea6fef81b06..08ec1e83b84 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -37,6 +37,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -56,6 +57,7 @@ import io.grpc.Metadata; import io.grpc.MethodDescriptor; import io.grpc.MethodDescriptor.Marshaller; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.Status; import io.grpc.Status.Code; @@ -251,7 +253,8 @@ public void setSoLingerChannelOption() throws IOException, GeneralSecurityExcept new SocketPicker(), new FakeChannelLogger(), false, - new io.grpc.MetricRecorder() {} , + new MetricRecorder() { + }, Ticker.systemTicker()); transports.add(transport); callMeMaybe(transport.start(clientTransportListener)); @@ -527,7 +530,8 @@ public void failingToConstructChannelShouldFailGracefully() throws Exception { new SocketPicker(), new FakeChannelLogger(), false, - new io.grpc.MetricRecorder() {} , + new MetricRecorder() { + }, Ticker.systemTicker()); transports.add(transport); @@ -1150,7 +1154,8 @@ private NettyClientTransport newTransport(ProtocolNegotiator negotiator, int max new SocketPicker(), new FakeChannelLogger(), false, - new io.grpc.MetricRecorder() {} , + new MetricRecorder() { + }, Ticker.systemTicker()); transports.add(transport); return transport; @@ -1199,7 +1204,7 @@ private void startServer(int maxStreamsPerConnection, int maxHeaderListSize, 0, Attributes.EMPTY, channelz, - org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); + mock(MetricRecorder.class)); server.start(serverListener); address = TestUtils.testServerAddress((InetSocketAddress) server.getListenSocketAddress()); authority = GrpcUtil.authorityFromHostAndPort(address.getHostString(), address.getPort()); diff --git a/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java index 7999f73dbfd..2fef700970e 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java @@ -22,7 +22,7 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; -import io.grpc.ServerStreamTracer; +import io.grpc.MetricRecorder; import io.netty.channel.EventLoopGroup; import io.netty.channel.local.LocalServerChannel; import io.netty.handler.ssl.SslContext; @@ -44,9 +44,8 @@ public class NettyServerBuilderTest { public void addMultipleListenAddresses() { builder.addListenAddress(new InetSocketAddress(8081)); NettyServer server = builder.buildTransportServers( - ImmutableList.of(), - new io.grpc.MetricRecorder() { - }); + ImmutableList.of(), + new MetricRecorder() {}); assertThat(server.getListenSocketAddresses()).hasSize(2); } @@ -193,11 +192,11 @@ public void useNioTransport_shouldNotThrow() { } @Test - public void metricRecorder_propagatedToServer() throws Exception { - io.grpc.MetricRecorder recorder = mock(io.grpc.MetricRecorder.class); + public void metricRecorder_propagatedToServer() { + MetricRecorder recorder = mock(MetricRecorder.class); NettyServer server = builder.buildTransportServers( - ImmutableList.of(), recorder); + ImmutableList.of(), recorder); assertThat(server.getMetricRecorder()).isSameInstanceAs(recorder); } diff --git a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java index a5ab42d15a6..5c010793e08 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java @@ -59,6 +59,7 @@ import io.grpc.Attributes; import io.grpc.InternalStatus; import io.grpc.Metadata; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.Status; import io.grpc.Status.Code; @@ -1417,7 +1418,7 @@ protected NettyServerHandler newHandler() { maxRstPeriodNanos, Attributes.EMPTY, fakeClock().getTicker(), - new io.grpc.MetricRecorder() {}); + new MetricRecorder() {}); } @Override diff --git a/netty/src/test/java/io/grpc/netty/NettyServerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerTest.java index 65f2fe4d82c..61c3f9e219e 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerTest.java @@ -37,6 +37,7 @@ import io.grpc.InternalChannelz.SocketStats; import io.grpc.InternalInstrumented; import io.grpc.Metadata; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.FixedObjectPool; import io.grpc.internal.ServerListener; @@ -161,7 +162,7 @@ class NoHandlerProtocolNegotiator implements ProtocolNegotiator { 0, 0, // ignore Attributes.EMPTY, - channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); + channelz, mock(MetricRecorder.class)); final SettableFuture serverShutdownCalled = SettableFuture.create(); ns.start(new ServerListener() { @Override @@ -218,7 +219,7 @@ public void multiPortStartStopGet() throws Exception { 0, 0, // ignore Attributes.EMPTY, - channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); + channelz, mock(MetricRecorder.class)); final SettableFuture shutdownCompleted = SettableFuture.create(); ns.start(new ServerListener() { @Override @@ -298,7 +299,7 @@ public void multiPortConnections() throws Exception { 0, 0, // ignore Attributes.EMPTY, - channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); + channelz, mock(MetricRecorder.class)); final SettableFuture shutdownCompleted = SettableFuture.create(); ns.start(new ServerListener() { @Override @@ -366,7 +367,7 @@ public void getPort_notStarted() { 0, 0, // ignore Attributes.EMPTY, - channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); + channelz, mock(MetricRecorder.class)); assertThat(ns.getListenSocketAddress()).isEqualTo(addr); assertThat(ns.getListenSocketAddresses()).isEqualTo(addresses); @@ -447,7 +448,7 @@ class TestProtocolNegotiator implements ProtocolNegotiator { 0, 0, // ignore eagAttributes, - channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); + channelz, mock(MetricRecorder.class)); ns.start(new ServerListener() { @Override public ServerTransportListener transportCreated(ServerTransport transport) { @@ -501,7 +502,7 @@ public void channelzListenSocket() throws Exception { 0, 0, // ignore Attributes.EMPTY, - channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); + channelz, mock(MetricRecorder.class)); final SettableFuture shutdownCompleted = SettableFuture.create(); ns.start(new ServerListener() { @Override @@ -649,7 +650,7 @@ private NettyServer getServer(List addr, EventLoopGroup ev) { 0, 0, // ignore Attributes.EMPTY, - channelz, org.mockito.Mockito.mock(io.grpc.MetricRecorder.class)); + channelz, mock(MetricRecorder.class)); } private static class NoopServerTransportListener implements ServerTransportListener { diff --git a/netty/src/test/java/io/grpc/netty/NettyTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyTransportTest.java index d4dcc69bf12..22758a8b727 100644 --- a/netty/src/test/java/io/grpc/netty/NettyTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyTransportTest.java @@ -22,6 +22,7 @@ import com.google.common.util.concurrent.SettableFuture; import io.grpc.Attributes; import io.grpc.ChannelLogger; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.Status; import io.grpc.internal.AbstractTransportTest; @@ -71,8 +72,7 @@ protected InternalServer newServer( .forAddress(new InetSocketAddress("localhost", 0)) .flowControlWindow(AbstractTransportTest.TEST_FLOW_CONTROL_WINDOW) .setTransportTracerFactory(fakeClockTransportTracer) - .buildTransportServers(streamTracerFactories, new io.grpc.MetricRecorder() { - }); + .buildTransportServers(streamTracerFactories, new MetricRecorder() {}); } @Override @@ -82,8 +82,7 @@ protected InternalServer newServer( .forAddress(new InetSocketAddress("localhost", port)) .flowControlWindow(AbstractTransportTest.TEST_FLOW_CONTROL_WINDOW) .setTransportTracerFactory(fakeClockTransportTracer) - .buildTransportServers(streamTracerFactories, new io.grpc.MetricRecorder() { - }); + .buildTransportServers(streamTracerFactories, new MetricRecorder() {}); } @Override diff --git a/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java b/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java index 86667c807c5..403b1b64329 100644 --- a/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java +++ b/netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java @@ -46,6 +46,7 @@ import io.grpc.InternalChannelz; import io.grpc.InternalChannelz.Security; import io.grpc.Metadata; +import io.grpc.MetricRecorder; import io.grpc.SecurityLevel; import io.grpc.ServerCredentials; import io.grpc.ServerStreamTracer; @@ -391,8 +392,7 @@ private Object expectHandshake( .forPort(0, serverCreds) .buildTransportServers( Collections.emptyList(), - new io.grpc.MetricRecorder() { - }); + new MetricRecorder() {}); server.start(serverListener); ManagedClientTransport.Listener clientTransportListener = diff --git a/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java b/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java index b6d9ca05bde..ac59f31c747 100644 --- a/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java +++ b/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java @@ -19,6 +19,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -50,8 +51,10 @@ public class TcpMetricsTest { @Rule public final MockitoRule mocks = MockitoJUnit.rule(); - @Mock private MetricRecorder metricRecorder; - @Mock private Channel channel; + @Mock + private MetricRecorder metricRecorder; + @Mock + private Channel channel; @Mock private EventLoop eventLoop; @Mock @@ -118,7 +121,7 @@ public long rtt() { @Test public void tracker_recordTcpInfo_reflectionSuccess() { - MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + MetricRecorder recorder = mock(MetricRecorder.class); TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, @@ -142,7 +145,7 @@ public void tracker_recordTcpInfo_reflectionSuccess() { @Test public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() { - MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + MetricRecorder recorder = mock(MetricRecorder.class); TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, @@ -177,7 +180,7 @@ public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() { @Test public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() { - MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + MetricRecorder recorder = mock(MetricRecorder.class); TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, @@ -220,7 +223,7 @@ public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() { @Test public void tracker_periodicRecord_reportsDeltaForTotalRetrans() { - MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + MetricRecorder recorder = mock(MetricRecorder.class); TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, @@ -270,7 +273,7 @@ public void tracker_periodicRecord_reportsDeltaForTotalRetrans() { @Test public void tracker_periodicRecord_doesNotReportZeroDeltaForTotalRetrans() { - MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + MetricRecorder recorder = mock(MetricRecorder.class); TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, @@ -322,7 +325,7 @@ public void tcpInfo(FakeEpollTcpInfo info) { @Test public void tracker_reportsDeltas_correctly() { - MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + MetricRecorder recorder = mock(MetricRecorder.class); TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); String fakeChannelName = ConfigurableFakeWithTcpInfo.class.getName(); @@ -349,6 +352,8 @@ public void tracker_reportsDeltas_correctly() { eq(5L), any(), any()); // 15 retransmits total (delta 0) - should NOT report + // also set retransmits to 1 + infoSource.setValues(15, 1, 1000); tracker.recordTcpInfo(channel); // Verify no new interactions with this specific metric and value // We can't easily verify "no interaction" for specific value without capturing. @@ -372,13 +377,13 @@ public void tracker_reportsDeltas_correctly() { tracker.channelInactive(channel); verify(recorder, org.mockito.Mockito.times(1)).addLongCounter( eq(Objects.requireNonNull(metrics.recurringRetransmits)), - eq(0L), // From last infoSource setValues(15, 0, 1000) + eq(1L), // From last infoSource setValues(15, 1, 1000) any(), any()); } @Test public void tracker_recordTcpInfo_reflectionFailure() { - MetricRecorder recorder = org.mockito.Mockito.mock(MetricRecorder.class); + MetricRecorder recorder = mock(MetricRecorder.class); TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, diff --git a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java index b89a0487ae8..df9756333b5 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java @@ -17,6 +17,7 @@ package io.grpc.okhttp; import io.grpc.Internal; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.InternalServer; import io.grpc.internal.TransportTracer; @@ -30,7 +31,7 @@ public final class InternalOkHttpServerBuilder { public static InternalServer buildTransportServers(OkHttpServerBuilder builder, List streamTracerFactories, - io.grpc.MetricRecorder metricRecorder) { + MetricRecorder metricRecorder) { return builder.buildTransportServers(streamTracerFactories, metricRecorder); } diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java index c6507fcbc86..3ace69052b7 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java @@ -27,6 +27,7 @@ import io.grpc.ForwardingServerBuilder; import io.grpc.InsecureServerCredentials; import io.grpc.Internal; +import io.grpc.MetricRecorder; import io.grpc.ServerBuilder; import io.grpc.ServerCredentials; import io.grpc.ServerStreamTracer; @@ -388,7 +389,7 @@ void setStatsEnabled(boolean value) { InternalServer buildTransportServers( List streamTracerFactories, - io.grpc.MetricRecorder metricRecorder) { + MetricRecorder metricRecorder) { return new OkHttpServer(this, streamTracerFactories, serverImplBuilder.getChannelz()); } diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java index 2cd6370bcc7..9317ca96639 100644 --- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java +++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpTransportTest.java @@ -17,6 +17,7 @@ package io.grpc.okhttp; import io.grpc.InsecureServerCredentials; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.ClientTransportFactory; @@ -63,8 +64,7 @@ protected InternalServer newServer( .flowControlWindow(AbstractTransportTest.TEST_FLOW_CONTROL_WINDOW) .setTransportTracerFactory(fakeClockTransportTracer); return InternalOkHttpServerBuilder - .buildTransportServers(builder, streamTracerFactories, new io.grpc.MetricRecorder() { - }); + .buildTransportServers(builder, streamTracerFactories, new MetricRecorder() {}); } @Override diff --git a/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java b/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java index 6904340ac74..5b4172e6052 100644 --- a/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java +++ b/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java @@ -136,7 +136,7 @@ List getOptionalLabels() { return optionalLabels; } - MetricSink getSink() { + public MetricSink getSink() { return sink; } diff --git a/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java b/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java index f3e2d6247ac..240394b1e9e 100644 --- a/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java +++ b/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java @@ -18,6 +18,7 @@ import io.grpc.InternalChannelz; import io.grpc.InternalInstrumented; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.ClientTransportFactory; @@ -60,7 +61,7 @@ protected InternalServer newServer(List streamTracer return new InternalServer() { final InternalServer delegate = new ServletServerBuilder().buildTransportServers( - streamTracerFactories, new io.grpc.MetricRecorder() { + streamTracerFactories, new MetricRecorder() { }); @Override diff --git a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java index 1fc007f969e..17e32f3007c 100644 --- a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java +++ b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java @@ -31,6 +31,7 @@ import io.grpc.InternalInstrumented; import io.grpc.InternalLogId; import io.grpc.Metadata; +import io.grpc.MetricRecorder; import io.grpc.Server; import io.grpc.ServerBuilder; import io.grpc.ServerStreamTracer; @@ -160,7 +161,7 @@ public void transportTerminated() { @VisibleForTesting InternalServer buildTransportServers( List streamTracerFactories, - io.grpc.MetricRecorder metricRecorder) { + MetricRecorder metricRecorder) { checkNotNull(streamTracerFactories, "streamTracerFactories"); this.streamTracerFactories = streamTracerFactories; internalServer = new InternalServerImpl(); diff --git a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java index 20b4fc83833..d869f25ca4f 100644 --- a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java +++ b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java @@ -18,6 +18,7 @@ import io.grpc.InternalChannelz.SocketStats; import io.grpc.InternalInstrumented; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.ClientTransportFactory; @@ -74,7 +75,7 @@ protected InternalServer newServer(List streamTracer final ServletServerBuilder builder = new ServletServerBuilder(); final InternalServer delegate = builder.buildTransportServers( - streamTracerFactories, new io.grpc.MetricRecorder() { + streamTracerFactories, new MetricRecorder() { }); @Override diff --git a/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java b/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java index 85f70dd987e..c072b465c77 100644 --- a/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java +++ b/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java @@ -22,6 +22,7 @@ import io.grpc.InternalChannelz.SocketStats; import io.grpc.InternalInstrumented; +import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.ClientTransportFactory; @@ -92,7 +93,7 @@ protected InternalServer newServer(List return new InternalServer() { final InternalServer delegate = new ServletServerBuilder().buildTransportServers( - streamTracerFactories, new io.grpc.MetricRecorder() { + streamTracerFactories, new MetricRecorder() { }); @Override From 1ee21f3fd3cbfa55cc6d696e6b1035e0991ad42b Mon Sep 17 00:00:00 2001 From: agravator Date: Mon, 9 Mar 2026 14:21:14 +0530 Subject: [PATCH 03/11] fix: format and BinderServerBuilder --- .../io/grpc/binder/BinderServerBuilder.java | 2 +- .../java/io/grpc/internal/InternalServer.java | 6 ++-- .../java/io/grpc/internal/ServerImpl.java | 1 - .../main/java/io/grpc/netty/TcpMetrics.java | 28 ++++++++----------- 4 files changed, 15 insertions(+), 22 deletions(-) diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java index c926c853472..5f0885883a5 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -68,7 +68,7 @@ private BinderServerBuilder( serverImplBuilder = new ServerImplBuilder( - streamTracerFactories -> { + (streamTracerFactories, metricRecorder) -> { internalBuilder.setStreamTracerFactories(streamTracerFactories); BinderServer server = internalBuilder.build(); BinderInternal.setIBinder(binderReceiver, server.getHostBinder()); diff --git a/core/src/main/java/io/grpc/internal/InternalServer.java b/core/src/main/java/io/grpc/internal/InternalServer.java index fdd4f32d0d8..a6079081233 100644 --- a/core/src/main/java/io/grpc/internal/InternalServer.java +++ b/core/src/main/java/io/grpc/internal/InternalServer.java @@ -58,8 +58,7 @@ public interface InternalServer { /** * Returns the first listen socket stats of this server. May return {@code null}. */ - @Nullable - InternalInstrumented getListenSocketStats(); + @Nullable InternalInstrumented getListenSocketStats(); /** * Returns a list of listening socket addresses. May change after {@link #start(ServerListener)} @@ -70,7 +69,6 @@ public interface InternalServer { /** * Returns a list of listen socket stats of this server. May return {@code null}. */ - @Nullable - List> getListenSocketStatsList(); + @Nullable List> getListenSocketStatsList(); } diff --git a/core/src/main/java/io/grpc/internal/ServerImpl.java b/core/src/main/java/io/grpc/internal/ServerImpl.java index 20e4a06ac38..dc0709e1fb8 100644 --- a/core/src/main/java/io/grpc/internal/ServerImpl.java +++ b/core/src/main/java/io/grpc/internal/ServerImpl.java @@ -143,7 +143,6 @@ public final class ServerImpl extends io.grpc.Server implements InternalInstrume InternalServer transportServer, Context rootContext) { this.executorPool = Preconditions.checkNotNull(builder.executorPool, "executorPool"); - this.registry = Preconditions.checkNotNull(builder.registryBuilder.build(), "registryBuilder"); this.fallbackRegistry = Preconditions.checkNotNull(builder.fallbackRegistry, "fallbackRegistry"); diff --git a/netty/src/main/java/io/grpc/netty/TcpMetrics.java b/netty/src/main/java/io/grpc/netty/TcpMetrics.java index d7f8d080d11..2f55c337f1b 100644 --- a/netty/src/main/java/io/grpc/netty/TcpMetrics.java +++ b/netty/src/main/java/io/grpc/netty/TcpMetrics.java @@ -151,14 +151,12 @@ static final class Tracker { private ScheduledFuture reportTimer; void channelActive(Channel channel) { - if (metricRecorder != null) { - List labelValues = getLabelValues(channel); - metricRecorder.addLongCounter(metrics.connectionsCreated, 1, - Collections.emptyList(), labelValues); - metricRecorder.addLongUpDownCounter(metrics.connectionCount, 1, - Collections.emptyList(), labelValues); - scheduleNextReport(channel, true); - } + List labelValues = getLabelValues(channel); + metricRecorder.addLongCounter(metrics.connectionsCreated, 1, + Collections.emptyList(), labelValues); + metricRecorder.addLongUpDownCounter(metrics.connectionCount, 1, + Collections.emptyList(), labelValues); + scheduleNextReport(channel, true); } private void scheduleNextReport(final Channel channel, boolean isInitial) { @@ -188,13 +186,11 @@ void channelInactive(Channel channel) { if (reportTimer != null) { reportTimer.cancel(false); } - if (metricRecorder != null) { - List labelValues = getLabelValues(channel); - metricRecorder.addLongUpDownCounter(metrics.connectionCount, -1, - Collections.emptyList(), labelValues); - // Final collection on close - recordTcpInfo(channel, true); - } + List labelValues = getLabelValues(channel); + metricRecorder.addLongUpDownCounter(metrics.connectionCount, -1, + Collections.emptyList(), labelValues); + // Final collection on close + recordTcpInfo(channel, true); } void recordTcpInfo(Channel channel) { @@ -202,7 +198,7 @@ void recordTcpInfo(Channel channel) { } private void recordTcpInfo(Channel channel, boolean isClose) { - if (metricRecorder == null || epollSocketChannelClass == null + if (epollSocketChannelClass == null || !epollSocketChannelClass.isInstance(channel)) { return; } From 20c614f5409fdbf8e35ae883640779485e51a8e3 Mon Sep 17 00:00:00 2001 From: agrawalabhi Date: Mon, 9 Mar 2026 11:09:44 +0000 Subject: [PATCH 04/11] fix: logs and plumb metric sink --- netty/src/main/java/io/grpc/netty/TcpMetrics.java | 13 +++++++++++-- .../io/grpc/opentelemetry/GrpcOpenTelemetry.java | 1 + 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/netty/src/main/java/io/grpc/netty/TcpMetrics.java b/netty/src/main/java/io/grpc/netty/TcpMetrics.java index 2f55c337f1b..61486cdb01e 100644 --- a/netty/src/main/java/io/grpc/netty/TcpMetrics.java +++ b/netty/src/main/java/io/grpc/netty/TcpMetrics.java @@ -56,6 +56,8 @@ final class TcpMetrics { } catch (Error e) { log.log(Level.FINE, "Failed to load native Epoll library", e); } + log.log(Level.INFO, "Epoll available during static init of TcpMetrics:" + + "{0}", epollAvailable); DEFAULT_METRICS = new Metrics(epollAvailable); } @@ -198,8 +200,15 @@ void recordTcpInfo(Channel channel) { } private void recordTcpInfo(Channel channel, boolean isClose) { - if (epollSocketChannelClass == null - || !epollSocketChannelClass.isInstance(channel)) { + if (epollSocketChannelClass == null) { + log.log(Level.FINE, "Skipping recordTcpInfo because" + + "epollSocketChannelClass is null"); + return; + } + if (!epollSocketChannelClass.isInstance(channel)) { + log.log(Level.FINE, "Skipping recordTcpInfo because channel is not an" + + "instance of epollSocketChannelClass: {0}", channel.getClass() + .getName()); return; } List labelValues = getLabelValues(channel); diff --git a/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java b/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java index 5b4172e6052..dfcd2f1c29f 100644 --- a/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java +++ b/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java @@ -196,6 +196,7 @@ public void configureServerBuilder(ServerBuilder serverBuilder) { serverBuilder.intercept(openTelemetryTracingModule.getServerSpanPropagationInterceptor()); } serverBuilder.addStreamTracerFactory(openTelemetryMetricsModule.getServerTracerFactory()); + serverBuilder.addMetricSink(sink); } @VisibleForTesting From 53a08103d88663bd819564f355670411f53ddcd6 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 9 Mar 2026 09:11:31 -0700 Subject: [PATCH 05/11] core: Wait for backoff timer on address update in pick_first The backoff timer is only used when serializeRetries=true, and that exists to match the old/current pick_first's behavior as closely as possible. InternalSubchannel.updateAddresses() would take no action when in TRANSIENT_FAILURE; it would update the addresses and just wait for the backoff timer to expire. Note that this only impacts serializeRetries=true; in the other cases we do want to start trying to the new addresses immediately, because the backoff timers are in the subchannels. Note that this change was also important because requestConnection() can be directly triggered by the user with channel.getState(true), and that shouldn't defeat the backoff timer. --- .../java/io/grpc/internal/PickFirstLeafLoadBalancer.java | 2 +- .../java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java b/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java index cd9ff9ab58c..f8f5c94f5ba 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java @@ -496,7 +496,7 @@ private void shutdownRemaining(SubchannelData activeSubchannelData) { */ @Override public void requestConnection() { - if (!addressIndex.isValid() || rawConnectivityState == SHUTDOWN) { + if (!addressIndex.isValid() || rawConnectivityState == SHUTDOWN || reconnectTask != null) { return; } diff --git a/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java b/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java index cb73d17d682..eb7b40257c0 100644 --- a/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java @@ -1446,6 +1446,11 @@ public void updateAddresses_disjoint_transient_failure() { loadBalancer.acceptResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(newServers).setAttributes(affinity).build()); + if (serializeRetries) { + inOrder.verify(mockSubchannel3, never()).start(stateListenerCaptor.capture()); + forwardTimeByBackoffDelay(); + } + // subchannel 3 still attempts a connection even though we stay in transient failure assertEquals(TRANSIENT_FAILURE, loadBalancer.getConcludedConnectivityState()); inOrder.verify(mockSubchannel3).start(stateListenerCaptor.capture()); From 30cad78c57dcf3064d9ad01b61adfa8b3a242283 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Tue, 10 Mar 2026 15:33:04 -0700 Subject: [PATCH 06/11] android-interop-testing: Remove usage of MultiDexApplication Since we're only supporting API levels 23+, all the supported Android versions handle multidex natively, and without any bugs to workaround. Also bump some minSdkVersion that didn't get updated in fa7b52b so that multiDex is actually enabled by default. See also b/476359563 --- android-interop-testing/build.gradle | 8 ++------ .../src/androidTest/AndroidManifest.xml | 3 +-- android-interop-testing/src/main/AndroidManifest.xml | 3 +-- binder/build.gradle | 1 - examples/android/clientcache/app/build.gradle | 3 +-- 5 files changed, 5 insertions(+), 13 deletions(-) diff --git a/android-interop-testing/build.gradle b/android-interop-testing/build.gradle index 17551465f05..49569b911e0 100644 --- a/android-interop-testing/build.gradle +++ b/android-interop-testing/build.gradle @@ -33,15 +33,11 @@ android { defaultConfig { applicationId "io.grpc.android.integrationtest" - // Held back to 20 as Gradle fails to build at the 21 level. This is - // presumably a Gradle bug that can be revisited later. - // Maybe this issue: https://github.com/gradle/gradle/issues/20778 - minSdkVersion 20 + minSdkVersion 23 targetSdkVersion 33 versionCode 1 versionName "1.0" testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" - multiDexEnabled = true } buildTypes { debug { minifyEnabled false } @@ -62,11 +58,11 @@ android { dependencies { implementation 'androidx.appcompat:appcompat:1.3.0' - implementation 'androidx.multidex:multidex:2.0.0' implementation libraries.androidx.annotation implementation 'com.google.android.gms:play-services-base:18.0.1' implementation project(':grpc-android'), + project(':grpc-api'), project(':grpc-core'), project(':grpc-census'), project(':grpc-okhttp'), diff --git a/android-interop-testing/src/androidTest/AndroidManifest.xml b/android-interop-testing/src/androidTest/AndroidManifest.xml index b0507f10ab9..3cc0a29a85f 100644 --- a/android-interop-testing/src/androidTest/AndroidManifest.xml +++ b/android-interop-testing/src/androidTest/AndroidManifest.xml @@ -5,8 +5,7 @@ android:name="androidx.test.runner.AndroidJUnitRunner" android:targetPackage="io.grpc.android.integrationtest" /> - + diff --git a/android-interop-testing/src/main/AndroidManifest.xml b/android-interop-testing/src/main/AndroidManifest.xml index da7ccef5b1d..08c139e5880 100644 --- a/android-interop-testing/src/main/AndroidManifest.xml +++ b/android-interop-testing/src/main/AndroidManifest.xml @@ -14,8 +14,7 @@ android:allowBackup="true" android:icon="@mipmap/ic_launcher" android:label="@string/app_name" - android:theme="@style/Base.V7.Theme.AppCompat.Light" - android:name="androidx.multidex.MultiDexApplication"> + android:theme="@style/Base.V7.Theme.AppCompat.Light"> diff --git a/binder/build.gradle b/binder/build.gradle index 3d743129420..0da3f97ceee 100644 --- a/binder/build.gradle +++ b/binder/build.gradle @@ -18,7 +18,6 @@ android { versionCode 1 versionName "1.0" testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" - multiDexEnabled = true } lintOptions { abortOnError = false } publishing { diff --git a/examples/android/clientcache/app/build.gradle b/examples/android/clientcache/app/build.gradle index 0f1dedf11bc..67110a78c43 100644 --- a/examples/android/clientcache/app/build.gradle +++ b/examples/android/clientcache/app/build.gradle @@ -10,9 +10,8 @@ android { defaultConfig { applicationId "io.grpc.clientcacheexample" - minSdkVersion 21 + minSdkVersion 23 targetSdkVersion 33 - multiDexEnabled true versionCode 1 versionName "1.0" testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner" From 9dfa8291d07865c9f91a2e9dc89814bfb352f62f Mon Sep 17 00:00:00 2001 From: agravator Date: Wed, 11 Mar 2026 12:26:00 +0530 Subject: [PATCH 07/11] fix: otel test --- .../test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/opentelemetry/src/test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java b/opentelemetry/src/test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java index 16cb02c61c2..68cb53d015b 100644 --- a/opentelemetry/src/test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java +++ b/opentelemetry/src/test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java @@ -98,6 +98,7 @@ public void buildTracer() { grpcOpenTelemetry.configureServerBuilder(mockServerBuiler); verify(mockServerBuiler, times(2)).addStreamTracerFactory(any()); verify(mockServerBuiler).intercept(any()); + verify(mockServerBuiler).addMetricSink(any()); verifyNoMoreInteractions(mockServerBuiler); ManagedChannelBuilder mockChannelBuilder = mock(ManagedChannelBuilder.class); From 2fda6fad29cf7ac2350ab422fd47fff3d3917a4b Mon Sep 17 00:00:00 2001 From: agravator Date: Wed, 11 Mar 2026 13:48:38 +0530 Subject: [PATCH 08/11] Revert 'android-interop-testing: Remove usage of MultiDexApplication' and 'core: Wait for backoff timer on address update in pick_first' --- android-interop-testing/build.gradle | 8 ++++++-- .../src/androidTest/AndroidManifest.xml | 3 ++- android-interop-testing/src/main/AndroidManifest.xml | 3 ++- binder/build.gradle | 1 + .../java/io/grpc/internal/PickFirstLeafLoadBalancer.java | 2 +- .../io/grpc/internal/PickFirstLeafLoadBalancerTest.java | 5 ----- examples/android/clientcache/app/build.gradle | 3 ++- 7 files changed, 14 insertions(+), 11 deletions(-) diff --git a/android-interop-testing/build.gradle b/android-interop-testing/build.gradle index 49569b911e0..17551465f05 100644 --- a/android-interop-testing/build.gradle +++ b/android-interop-testing/build.gradle @@ -33,11 +33,15 @@ android { defaultConfig { applicationId "io.grpc.android.integrationtest" - minSdkVersion 23 + // Held back to 20 as Gradle fails to build at the 21 level. This is + // presumably a Gradle bug that can be revisited later. + // Maybe this issue: https://github.com/gradle/gradle/issues/20778 + minSdkVersion 20 targetSdkVersion 33 versionCode 1 versionName "1.0" testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" + multiDexEnabled = true } buildTypes { debug { minifyEnabled false } @@ -58,11 +62,11 @@ android { dependencies { implementation 'androidx.appcompat:appcompat:1.3.0' + implementation 'androidx.multidex:multidex:2.0.0' implementation libraries.androidx.annotation implementation 'com.google.android.gms:play-services-base:18.0.1' implementation project(':grpc-android'), - project(':grpc-api'), project(':grpc-core'), project(':grpc-census'), project(':grpc-okhttp'), diff --git a/android-interop-testing/src/androidTest/AndroidManifest.xml b/android-interop-testing/src/androidTest/AndroidManifest.xml index 3cc0a29a85f..b0507f10ab9 100644 --- a/android-interop-testing/src/androidTest/AndroidManifest.xml +++ b/android-interop-testing/src/androidTest/AndroidManifest.xml @@ -5,7 +5,8 @@ android:name="androidx.test.runner.AndroidJUnitRunner" android:targetPackage="io.grpc.android.integrationtest" /> - + diff --git a/android-interop-testing/src/main/AndroidManifest.xml b/android-interop-testing/src/main/AndroidManifest.xml index 08c139e5880..da7ccef5b1d 100644 --- a/android-interop-testing/src/main/AndroidManifest.xml +++ b/android-interop-testing/src/main/AndroidManifest.xml @@ -14,7 +14,8 @@ android:allowBackup="true" android:icon="@mipmap/ic_launcher" android:label="@string/app_name" - android:theme="@style/Base.V7.Theme.AppCompat.Light"> + android:theme="@style/Base.V7.Theme.AppCompat.Light" + android:name="androidx.multidex.MultiDexApplication"> diff --git a/binder/build.gradle b/binder/build.gradle index 0da3f97ceee..3d743129420 100644 --- a/binder/build.gradle +++ b/binder/build.gradle @@ -18,6 +18,7 @@ android { versionCode 1 versionName "1.0" testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" + multiDexEnabled = true } lintOptions { abortOnError = false } publishing { diff --git a/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java b/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java index f8f5c94f5ba..cd9ff9ab58c 100644 --- a/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java +++ b/core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java @@ -496,7 +496,7 @@ private void shutdownRemaining(SubchannelData activeSubchannelData) { */ @Override public void requestConnection() { - if (!addressIndex.isValid() || rawConnectivityState == SHUTDOWN || reconnectTask != null) { + if (!addressIndex.isValid() || rawConnectivityState == SHUTDOWN) { return; } diff --git a/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java b/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java index eb7b40257c0..cb73d17d682 100644 --- a/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java +++ b/core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java @@ -1446,11 +1446,6 @@ public void updateAddresses_disjoint_transient_failure() { loadBalancer.acceptResolvedAddresses( ResolvedAddresses.newBuilder().setAddresses(newServers).setAttributes(affinity).build()); - if (serializeRetries) { - inOrder.verify(mockSubchannel3, never()).start(stateListenerCaptor.capture()); - forwardTimeByBackoffDelay(); - } - // subchannel 3 still attempts a connection even though we stay in transient failure assertEquals(TRANSIENT_FAILURE, loadBalancer.getConcludedConnectivityState()); inOrder.verify(mockSubchannel3).start(stateListenerCaptor.capture()); diff --git a/examples/android/clientcache/app/build.gradle b/examples/android/clientcache/app/build.gradle index 67110a78c43..0f1dedf11bc 100644 --- a/examples/android/clientcache/app/build.gradle +++ b/examples/android/clientcache/app/build.gradle @@ -10,8 +10,9 @@ android { defaultConfig { applicationId "io.grpc.clientcacheexample" - minSdkVersion 23 + minSdkVersion 21 targetSdkVersion 33 + multiDexEnabled true versionCode 1 versionName "1.0" testInstrumentationRunner "android.support.test.runner.AndroidJUnitRunner" From b12a4333a913e28e6e18ed3c0db6954143de82e1 Mon Sep 17 00:00:00 2001 From: agrawalabhi Date: Wed, 11 Mar 2026 15:32:42 +0000 Subject: [PATCH 09/11] fix: make metric recorder non null and register metrics unconditionally --- api/src/main/java/io/grpc/NameResolver.java | 5 +- .../main/java/io/grpc/netty/NettyServer.java | 3 - .../main/java/io/grpc/netty/TcpMetrics.java | 63 +++++++------------ .../java/io/grpc/netty/TcpMetricsTest.java | 29 +++------ .../java/io/grpc/xds/XdsNameResolver.java | 6 +- .../OrcaMetricReportingServerInterceptor.java | 8 ++- 6 files changed, 43 insertions(+), 71 deletions(-) diff --git a/api/src/main/java/io/grpc/NameResolver.java b/api/src/main/java/io/grpc/NameResolver.java index 3494eab93d0..b47bd93332b 100644 --- a/api/src/main/java/io/grpc/NameResolver.java +++ b/api/src/main/java/io/grpc/NameResolver.java @@ -369,7 +369,8 @@ private Args(Builder builder) { this.channelLogger = builder.channelLogger; this.executor = builder.executor; this.overrideAuthority = builder.overrideAuthority; - this.metricRecorder = builder.metricRecorder; + this.metricRecorder = builder.metricRecorder != null ? builder.metricRecorder + : new MetricRecorder() {}; this.nameResolverRegistry = builder.nameResolverRegistry; this.customArgs = cloneCustomArgs(builder.customArgs); } @@ -679,7 +680,7 @@ public Builder setArg(Key key, T value) { * See {@link Args#getMetricRecorder()}. This is an optional field. */ public Builder setMetricRecorder(MetricRecorder metricRecorder) { - this.metricRecorder = metricRecorder; + this.metricRecorder = checkNotNull(metricRecorder); return this; } diff --git a/netty/src/main/java/io/grpc/netty/NettyServer.java b/netty/src/main/java/io/grpc/netty/NettyServer.java index 80c45b490f8..5dcd8c4c9fb 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServer.java +++ b/netty/src/main/java/io/grpc/netty/NettyServer.java @@ -69,7 +69,6 @@ import java.util.concurrent.Callable; import java.util.logging.Level; import java.util.logging.Logger; -import javax.annotation.Nullable; /** * Netty-based server implementation. @@ -178,11 +177,9 @@ class NettyServer implements InternalServer, InternalWithLogId { this.channelz = Preconditions.checkNotNull(channelz); this.logId = InternalLogId.allocate(getClass(), addresses.isEmpty() ? "No address" : String.valueOf(addresses)); - this.metricRecorder = metricRecorder; } @VisibleForTesting - @Nullable MetricRecorder getMetricRecorder() { return metricRecorder; } diff --git a/netty/src/main/java/io/grpc/netty/TcpMetrics.java b/netty/src/main/java/io/grpc/netty/TcpMetrics.java index 61486cdb01e..895df5ffe82 100644 --- a/netty/src/main/java/io/grpc/netty/TcpMetrics.java +++ b/netty/src/main/java/io/grpc/netty/TcpMetrics.java @@ -16,6 +16,7 @@ package io.grpc.netty; +import com.google.common.annotations.VisibleForTesting; import io.grpc.DoubleHistogramMetricInstrument; import io.grpc.InternalTcpMetrics; import io.grpc.LongCounterMetricInstrument; @@ -29,7 +30,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeUnit; import java.util.logging.Level; @@ -58,9 +58,10 @@ final class TcpMetrics { } log.log(Level.INFO, "Epoll available during static init of TcpMetrics:" + "{0}", epollAvailable); - DEFAULT_METRICS = new Metrics(epollAvailable); + DEFAULT_METRICS = new Metrics(); } + @VisibleForTesting static Metrics getDefaultMetrics() { return DEFAULT_METRICS; } @@ -72,19 +73,12 @@ static final class Metrics { final LongCounterMetricInstrument recurringRetransmits; final DoubleHistogramMetricInstrument minRtt; - Metrics(boolean epollAvailable) { + Metrics() { connectionsCreated = InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT; connectionCount = InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT; - - if (epollAvailable) { - packetsRetransmitted = InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT; - recurringRetransmits = InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT; - minRtt = InternalTcpMetrics.MIN_RTT_INSTRUMENT; - } else { - packetsRetransmitted = null; - recurringRetransmits = null; - minRtt = null; - } + packetsRetransmitted = InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT; + recurringRetransmits = InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT; + minRtt = InternalTcpMetrics.MIN_RTT_INSTRUMENT; } } @@ -171,17 +165,12 @@ private void scheduleNextReport(final Channel channel, boolean isInitial) { : 0.9 + ThreadLocalRandom.current().nextDouble() * 0.2; // 90% to 110% long rearmingDelay = (long) (RECORD_INTERVAL_MILLIS * jitter); - try { - reportTimer = channel.eventLoop().schedule(() -> { - if (channel.isActive()) { - Tracker.this.recordTcpInfo(channel, false); - scheduleNextReport(channel, false); // Re-arm - } - }, rearmingDelay, TimeUnit.MILLISECONDS); - } catch (RejectedExecutionException e) { - log.log(Level.FINE, "Failed to schedule next TCP metrics report", e); - // The event loop is likely shutting down. We can safely ignore this. - } + reportTimer = channel.eventLoop().schedule(() -> { + if (channel.isActive()) { + Tracker.this.recordTcpInfo(channel, false); + scheduleNextReport(channel, false); // Re-arm + } + }, rearmingDelay, TimeUnit.MILLISECONDS); } void channelInactive(Channel channel) { @@ -226,25 +215,19 @@ private void recordTcpInfo(Channel channel, boolean isClose) { return; } - if (metrics.packetsRetransmitted != null) { - long deltaTotal = totalRetrans - lastTotalRetrans; - if (deltaTotal > 0) { - metricRecorder.addLongCounter(metrics.packetsRetransmitted, deltaTotal, - Collections.emptyList(), labelValues); - lastTotalRetrans = totalRetrans; - } - } - if (metrics.recurringRetransmits != null && isClose) { - if (retransmits > 0) { - metricRecorder.addLongCounter(metrics.recurringRetransmits, retransmits, - Collections.emptyList(), labelValues); - } + long deltaTotal = totalRetrans - lastTotalRetrans; + if (deltaTotal > 0) { + metricRecorder.addLongCounter(metrics.packetsRetransmitted, deltaTotal, + Collections.emptyList(), labelValues); + lastTotalRetrans = totalRetrans; } - if (metrics.minRtt != null) { - metricRecorder.recordDoubleHistogram(metrics.minRtt, - rtt / 1000000.0, // Convert microseconds to seconds + if (isClose && retransmits > 0) { + metricRecorder.addLongCounter(metrics.recurringRetransmits, retransmits, Collections.emptyList(), labelValues); } + metricRecorder.recordDoubleHistogram(metrics.minRtt, + rtt / 1000000.0, // Convert microseconds to seconds + Collections.emptyList(), labelValues); } } diff --git a/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java b/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java index ac59f31c747..90499b59c85 100644 --- a/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java +++ b/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java @@ -71,19 +71,8 @@ public void setUp() { } @Test - public void metricsInitialization_epollUnavailable() { - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(false); - - org.junit.Assert.assertNotNull(metrics.connectionsCreated); - org.junit.Assert.assertNotNull(metrics.connectionCount); - org.junit.Assert.assertNull(metrics.packetsRetransmitted); - org.junit.Assert.assertNull(metrics.recurringRetransmits); - org.junit.Assert.assertNull(metrics.minRtt); - } - - @Test - public void metricsInitialization_epollAvailable() { - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + public void metricsInitialization() { + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); org.junit.Assert.assertNotNull(metrics.connectionsCreated); org.junit.Assert.assertNotNull(metrics.connectionCount); @@ -122,7 +111,7 @@ public long rtt() { @Test public void tracker_recordTcpInfo_reflectionSuccess() { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, ConfigurableFakeWithTcpInfo.class.getName(), @@ -146,7 +135,7 @@ public void tracker_recordTcpInfo_reflectionSuccess() { @Test public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, ConfigurableFakeWithTcpInfo.class.getName(), @@ -181,7 +170,7 @@ public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() { @Test public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, ConfigurableFakeWithTcpInfo.class.getName(), @@ -224,7 +213,7 @@ public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() { @Test public void tracker_periodicRecord_reportsDeltaForTotalRetrans() { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, ConfigurableFakeWithTcpInfo.class.getName(), @@ -274,7 +263,7 @@ public void tracker_periodicRecord_reportsDeltaForTotalRetrans() { @Test public void tracker_periodicRecord_doesNotReportZeroDeltaForTotalRetrans() { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, ConfigurableFakeWithTcpInfo.class.getName(), @@ -326,7 +315,7 @@ public void tcpInfo(FakeEpollTcpInfo info) { @Test public void tracker_reportsDeltas_correctly() { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); String fakeChannelName = ConfigurableFakeWithTcpInfo.class.getName(); String fakeInfoName = FakeEpollTcpInfo.class.getName(); @@ -384,7 +373,7 @@ public void tracker_reportsDeltas_correctly() { @Test public void tracker_recordTcpInfo_reflectionFailure() { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true); + TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, "non.existent.Class", "non.existent.Info"); diff --git a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java index 196d51fb5a6..06bd66008f4 100644 --- a/xds/src/main/java/io/grpc/xds/XdsNameResolver.java +++ b/xds/src/main/java/io/grpc/xds/XdsNameResolver.java @@ -1052,18 +1052,18 @@ private static final class BootstrappingXdsClientPool implements XdsClientPool { private final XdsClientPoolFactory xdsClientPoolFactory; private final String target; private final @Nullable Map bootstrapOverride; - private final @Nullable MetricRecorder metricRecorder; + private final MetricRecorder metricRecorder; private ObjectPool xdsClientPool; BootstrappingXdsClientPool( XdsClientPoolFactory xdsClientPoolFactory, String target, @Nullable Map bootstrapOverride, - @Nullable MetricRecorder metricRecorder) { + MetricRecorder metricRecorder) { this.xdsClientPoolFactory = checkNotNull(xdsClientPoolFactory, "xdsClientPoolFactory"); this.target = checkNotNull(target, "target"); this.bootstrapOverride = bootstrapOverride; - this.metricRecorder = metricRecorder; + this.metricRecorder = checkNotNull(metricRecorder, "metricRecorder"); } @Override diff --git a/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java b/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java index 1b767e0303c..b68dafa7f5b 100644 --- a/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java +++ b/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java @@ -16,6 +16,8 @@ package io.grpc.xds.orca; +import static com.google.common.base.Preconditions.checkNotNull; + import com.github.xds.data.orca.v3.OrcaLoadReport; import com.google.common.annotations.VisibleForTesting; import io.grpc.Context; @@ -60,8 +62,8 @@ public final class OrcaMetricReportingServerInterceptor implements ServerInterce private final MetricRecorder metricRecorder; @VisibleForTesting - OrcaMetricReportingServerInterceptor(@Nullable MetricRecorder metricRecorder) { - this.metricRecorder = metricRecorder; + OrcaMetricReportingServerInterceptor(MetricRecorder metricRecorder) { + this.metricRecorder = checkNotNull(metricRecorder, "metricRecorder"); } public static OrcaMetricReportingServerInterceptor getInstance() { @@ -75,7 +77,7 @@ public static OrcaMetricReportingServerInterceptor getInstance() { * higher precedence compared to metrics from {@link MetricRecorder}. */ public static OrcaMetricReportingServerInterceptor create( - @Nullable MetricRecorder metricRecorder) { + MetricRecorder metricRecorder) { return new OrcaMetricReportingServerInterceptor(metricRecorder); } From d8180d6d66e945b82f6471cafbcf42e9dd5868d6 Mon Sep 17 00:00:00 2001 From: agrawalabhi Date: Wed, 11 Mar 2026 16:12:22 +0000 Subject: [PATCH 10/11] fix: revert non-nullable metric recorder in orca --- .../grpc/xds/orca/OrcaMetricReportingServerInterceptor.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java b/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java index b68dafa7f5b..b2139a0ee6e 100644 --- a/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java +++ b/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java @@ -16,8 +16,6 @@ package io.grpc.xds.orca; -import static com.google.common.base.Preconditions.checkNotNull; - import com.github.xds.data.orca.v3.OrcaLoadReport; import com.google.common.annotations.VisibleForTesting; import io.grpc.Context; @@ -62,8 +60,8 @@ public final class OrcaMetricReportingServerInterceptor implements ServerInterce private final MetricRecorder metricRecorder; @VisibleForTesting - OrcaMetricReportingServerInterceptor(MetricRecorder metricRecorder) { - this.metricRecorder = checkNotNull(metricRecorder, "metricRecorder"); + OrcaMetricReportingServerInterceptor(@Nullable MetricRecorder metricRecorder) { + this.metricRecorder = metricRecorder; } public static OrcaMetricReportingServerInterceptor getInstance() { From 20f1d735c0c39b46e0e4774186d565f173dcdb94 Mon Sep 17 00:00:00 2001 From: agravator Date: Fri, 13 Mar 2026 16:19:52 +0530 Subject: [PATCH 11/11] fix: suggested changes --- .../main/java/io/grpc/InternalTcpMetrics.java | 108 ++++--- api/src/main/java/io/grpc/ServerBuilder.java | 3 +- .../main/java/io/grpc/netty/NettyServer.java | 5 - .../main/java/io/grpc/netty/TcpMetrics.java | 184 +++++------- .../grpc/netty/NettyClientTransportTest.java | 219 +++++++------- .../io/grpc/netty/NettyServerBuilderTest.java | 9 - .../io/grpc/netty/NettyServerHandlerTest.java | 17 +- .../java/io/grpc/netty/TcpMetricsTest.java | 279 ++++++++++-------- .../okhttp/InternalOkHttpServerBuilder.java | 2 +- .../io/grpc/okhttp/OkHttpServerBuilder.java | 13 +- .../grpc/opentelemetry/GrpcOpenTelemetry.java | 2 +- .../opentelemetry/GrpcOpenTelemetryTest.java | 2 - .../io/grpc/servlet/JettyTransportTest.java | 5 +- .../io/grpc/servlet/ServletServerBuilder.java | 8 +- .../io/grpc/servlet/TomcatTransportTest.java | 6 +- .../grpc/servlet/UndertowTransportTest.java | 5 +- .../OrcaMetricReportingServerInterceptor.java | 2 +- 17 files changed, 430 insertions(+), 439 deletions(-) diff --git a/api/src/main/java/io/grpc/InternalTcpMetrics.java b/api/src/main/java/io/grpc/InternalTcpMetrics.java index dd1048ff765..3dd89b6f76c 100644 --- a/api/src/main/java/io/grpc/InternalTcpMetrics.java +++ b/api/src/main/java/io/grpc/InternalTcpMetrics.java @@ -16,18 +16,22 @@ package io.grpc; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; /** * TCP Metrics defined to be shared across transport implementations. + * These metrics and their definitions are specified in + * gRFC + * A80. */ @Internal public final class InternalTcpMetrics { - private InternalTcpMetrics() {} + private InternalTcpMetrics() { + } private static final List OPTIONAL_LABELS = Arrays.asList( "network.local.address", @@ -35,70 +39,60 @@ private InternalTcpMetrics() {} "network.peer.address", "network.peer.port"); - public static final DoubleHistogramMetricInstrument MIN_RTT_INSTRUMENT = + public static final DoubleHistogramMetricInstrument MIN_RTT_INSTRUMENT = MetricInstrumentRegistry.getDefaultRegistry() .registerDoubleHistogram( - "grpc.tcp.min_rtt", - "Minimum round-trip time of a TCP connection", - "s", - getMinRttBuckets(), - Collections.emptyList(), - OPTIONAL_LABELS, - false); + "grpc.tcp.min_rtt", + "Minimum round-trip time of a TCP connection", + "s", + Collections.emptyList(), + Collections.emptyList(), + OPTIONAL_LABELS, + false); - public static final LongCounterMetricInstrument CONNECTIONS_CREATED_INSTRUMENT = + public static final LongCounterMetricInstrument CONNECTIONS_CREATED_INSTRUMENT = MetricInstrumentRegistry - .getDefaultRegistry() - .registerLongCounter( - "grpc.tcp.connections_created", - "The total number of TCP connections established.", - "{connection}", - Collections.emptyList(), - OPTIONAL_LABELS, - false); + .getDefaultRegistry() + .registerLongCounter( + "grpc.tcp.connections_created", + "The total number of TCP connections established.", + "{connection}", + Collections.emptyList(), + OPTIONAL_LABELS, + false); - public static final LongUpDownCounterMetricInstrument CONNECTION_COUNT_INSTRUMENT = + public static final LongUpDownCounterMetricInstrument CONNECTION_COUNT_INSTRUMENT = MetricInstrumentRegistry - .getDefaultRegistry() - .registerLongUpDownCounter( - "grpc.tcp.connection_count", - "The current number of active TCP connections.", - "{connection}", - Collections.emptyList(), - OPTIONAL_LABELS, - false - ); + .getDefaultRegistry() + .registerLongUpDownCounter( + "grpc.tcp.connection_count", + "The current number of active TCP connections.", + "{connection}", + Collections.emptyList(), + OPTIONAL_LABELS, + false); - public static final LongCounterMetricInstrument PACKETS_RETRANSMITTED_INSTRUMENT = + public static final LongCounterMetricInstrument PACKETS_RETRANSMITTED_INSTRUMENT = MetricInstrumentRegistry - .getDefaultRegistry() - .registerLongCounter( - "grpc.tcp.packets_retransmitted", - "The total number of packets retransmitted for all TCP connections.", - "{packet}", - Collections.emptyList(), - OPTIONAL_LABELS, - false - ); + .getDefaultRegistry() + .registerLongCounter( + "grpc.tcp.packets_retransmitted", + "The total number of packets retransmitted for all TCP connections.", + "{packet}", + Collections.emptyList(), + OPTIONAL_LABELS, + false); - public static final LongCounterMetricInstrument RECURRING_RETRANSMITS_INSTRUMENT = + public static final LongCounterMetricInstrument RECURRING_RETRANSMITS_INSTRUMENT = MetricInstrumentRegistry - .getDefaultRegistry() - .registerLongCounter( - "grpc.tcp.recurring_retransmits", - "The total number of times the retransmit timer popped for all TCP" - + " connections.", - "{timeout}", - Collections.emptyList(), - OPTIONAL_LABELS, - false - ); + .getDefaultRegistry() + .registerLongCounter( + "grpc.tcp.recurring_retransmits", + "The total number of times the retransmit timer " + + "popped for all TCP connections.", + "{timeout}", + Collections.emptyList(), + OPTIONAL_LABELS, + false); - private static List getMinRttBuckets() { - List buckets = new ArrayList<>(100); - for (int i = 1; i <= 100; i++) { - buckets.add(1e-6 * Math.pow(2.0, i * 0.24)); - } - return Collections.unmodifiableList(buckets); - } } diff --git a/api/src/main/java/io/grpc/ServerBuilder.java b/api/src/main/java/io/grpc/ServerBuilder.java index 7dbc7bf6077..3effe593e57 100644 --- a/api/src/main/java/io/grpc/ServerBuilder.java +++ b/api/src/main/java/io/grpc/ServerBuilder.java @@ -441,8 +441,9 @@ public T setBinaryLog(BinaryLog binaryLog) { * @param metricSink the metric sink to add. * @return this */ + @ExperimentalApi("https://github.com/grpc/grpc-java/issues/12693") public T addMetricSink(MetricSink metricSink) { - throw new UnsupportedOperationException(); + return thisT(); } /** diff --git a/netty/src/main/java/io/grpc/netty/NettyServer.java b/netty/src/main/java/io/grpc/netty/NettyServer.java index 5dcd8c4c9fb..2bb6b2c5921 100644 --- a/netty/src/main/java/io/grpc/netty/NettyServer.java +++ b/netty/src/main/java/io/grpc/netty/NettyServer.java @@ -21,7 +21,6 @@ import static io.netty.channel.ChannelOption.ALLOCATOR; import static io.netty.channel.ChannelOption.SO_KEEPALIVE; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; import com.google.common.base.Preconditions; import com.google.common.util.concurrent.ListenableFuture; @@ -179,10 +178,6 @@ class NettyServer implements InternalServer, InternalWithLogId { String.valueOf(addresses)); } - @VisibleForTesting - MetricRecorder getMetricRecorder() { - return metricRecorder; - } @Override public SocketAddress getListenSocketAddress() { diff --git a/netty/src/main/java/io/grpc/netty/TcpMetrics.java b/netty/src/main/java/io/grpc/netty/TcpMetrics.java index 895df5ffe82..0123c774771 100644 --- a/netty/src/main/java/io/grpc/netty/TcpMetrics.java +++ b/netty/src/main/java/io/grpc/netty/TcpMetrics.java @@ -16,11 +16,7 @@ package io.grpc.netty; -import com.google.common.annotations.VisibleForTesting; -import io.grpc.DoubleHistogramMetricInstrument; import io.grpc.InternalTcpMetrics; -import io.grpc.LongCounterMetricInstrument; -import io.grpc.LongUpDownCounterMetricInstrument; import io.grpc.MetricRecorder; import io.netty.channel.Channel; import io.netty.util.concurrent.ScheduledFuture; @@ -41,106 +37,82 @@ final class TcpMetrics { private static final Logger log = Logger.getLogger(TcpMetrics.class.getName()); - private static final Metrics DEFAULT_METRICS; + static EpollInfo epollInfo = loadEpollInfo(); + + static final class EpollInfo { + final Class channelClass; + final Class infoClass; + final java.lang.reflect.Constructor infoConstructor; + final Method tcpInfo; + final Method totalRetrans; + final Method retransmits; + final Method rtt; + + EpollInfo( + Class channelClass, + Class infoClass, + java.lang.reflect.Constructor infoConstructor, + Method tcpInfo, + Method totalRetrans, + Method retransmits, + Method rtt) { + this.channelClass = channelClass; + this.infoClass = infoClass; + this.infoConstructor = infoConstructor; + this.tcpInfo = tcpInfo; + this.totalRetrans = totalRetrans; + this.retransmits = retransmits; + this.rtt = rtt; + } + } - static { + private static EpollInfo loadEpollInfo() { boolean epollAvailable = false; try { Class epollClass = Class.forName("io.netty.channel.epoll.Epoll"); Method isAvailableMethod = epollClass.getDeclaredMethod("isAvailable"); epollAvailable = (Boolean) isAvailableMethod.invoke(null); - } catch (ClassNotFoundException e) { - log.log(Level.FINE, "Epoll is not available", e); - } catch (Exception e) { - log.log(Level.FINE, "Failed to determine Epoll availability", e); + if (epollAvailable) { + Class channelClass = Class.forName("io.netty.channel.epoll.EpollSocketChannel"); + Class infoClass = Class.forName("io.netty.channel.epoll.EpollTcpInfo"); + return new EpollInfo( + channelClass, + infoClass, + infoClass.getDeclaredConstructor(), + channelClass.getMethod("tcpInfo", infoClass), + infoClass.getMethod("totalRetrans"), + infoClass.getMethod("retrans"), + infoClass.getMethod("rtt")); + } + } catch (ReflectiveOperationException | RuntimeException e) { + log.log(Level.FINE, "Failed to initialize Epoll tcp_info reflection", e); } catch (Error e) { log.log(Level.FINE, "Failed to load native Epoll library", e); + } finally { + log.log(Level.INFO, "Epoll available during static init of TcpMetrics:" + + "{0}", epollAvailable); } - log.log(Level.INFO, "Epoll available during static init of TcpMetrics:" - + "{0}", epollAvailable); - DEFAULT_METRICS = new Metrics(); - } - - @VisibleForTesting - static Metrics getDefaultMetrics() { - return DEFAULT_METRICS; - } - - static final class Metrics { - final LongCounterMetricInstrument connectionsCreated; - final LongUpDownCounterMetricInstrument connectionCount; - final LongCounterMetricInstrument packetsRetransmitted; - final LongCounterMetricInstrument recurringRetransmits; - final DoubleHistogramMetricInstrument minRtt; - - Metrics() { - connectionsCreated = InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT; - connectionCount = InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT; - packetsRetransmitted = InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT; - recurringRetransmits = InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT; - minRtt = InternalTcpMetrics.MIN_RTT_INSTRUMENT; - } + return null; } static final class Tracker { private final MetricRecorder metricRecorder; - private final Metrics metrics; - private final Class epollSocketChannelClass; - private final Method tcpInfoMethod; - private final Method totalRetransMethod; - private final Method retransmitsMethod; - private final Method rttMethod; private final Object tcpInfo; private long lastTotalRetrans = 0; Tracker(MetricRecorder metricRecorder) { - this(metricRecorder, DEFAULT_METRICS); - } - - Tracker(MetricRecorder metricRecorder, Metrics metrics) { - this( - metricRecorder, metrics, - "io.netty.channel.epoll.EpollSocketChannel", - "io.netty.channel.epoll.EpollTcpInfo"); - } - - Tracker(MetricRecorder metricRecorder, Metrics metrics, - String epollSocketChannelClassName, String epollTcpInfoClassName) { this.metricRecorder = metricRecorder; - this.metrics = metrics; - - Class epollSocketChannelClass; - Method tcpInfoMethod; - Object tcpInfo; - Method totalRetransMethod; - Method retransMethod; - Method rttMethod; - try { - epollSocketChannelClass = Class.forName(epollSocketChannelClassName); - Class epollTcpInfoClass = Class.forName(epollTcpInfoClassName); - tcpInfo = epollTcpInfoClass.getDeclaredConstructor().newInstance(); - tcpInfoMethod = epollSocketChannelClass.getMethod("tcpInfo", epollTcpInfoClass); - totalRetransMethod = epollTcpInfoClass.getMethod("totalRetrans"); - retransMethod = epollTcpInfoClass.getMethod("retrans"); - rttMethod = epollTcpInfoClass.getMethod("rtt"); - } catch (Exception | Error t) { - // Epoll not available or error getting tcp_info, features disabled - log.log(Level.FINE, "Failed to initialize Epoll tcp_info reflection", t); - epollSocketChannelClass = null; - tcpInfoMethod = null; - tcpInfo = null; - totalRetransMethod = null; - retransMethod = null; - rttMethod = null; + Object tcpInfo = null; + if (epollInfo != null) { + try { + tcpInfo = epollInfo.infoConstructor.newInstance(); + } catch (ReflectiveOperationException e) { + log.log(Level.FINE, "Failed to instantiate EpollTcpInfo", e); + } } - this.epollSocketChannelClass = epollSocketChannelClass; - this.tcpInfoMethod = tcpInfoMethod; this.tcpInfo = tcpInfo; - this.totalRetransMethod = totalRetransMethod; - this.retransmitsMethod = retransMethod; - this.rttMethod = rttMethod; } private static final long RECORD_INTERVAL_MILLIS = TimeUnit.MINUTES.toMillis(5); @@ -148,9 +120,9 @@ static final class Tracker { void channelActive(Channel channel) { List labelValues = getLabelValues(channel); - metricRecorder.addLongCounter(metrics.connectionsCreated, 1, + metricRecorder.addLongCounter(InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT, 1, Collections.emptyList(), labelValues); - metricRecorder.addLongUpDownCounter(metrics.connectionCount, 1, + metricRecorder.addLongUpDownCounter(InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT, 1, Collections.emptyList(), labelValues); scheduleNextReport(channel, true); } @@ -178,7 +150,7 @@ void channelInactive(Channel channel) { reportTimer.cancel(false); } List labelValues = getLabelValues(channel); - metricRecorder.addLongUpDownCounter(metrics.connectionCount, -1, + metricRecorder.addLongUpDownCounter(InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT, -1, Collections.emptyList(), labelValues); // Final collection on close recordTcpInfo(channel, true); @@ -189,15 +161,16 @@ void recordTcpInfo(Channel channel) { } private void recordTcpInfo(Channel channel, boolean isClose) { - if (epollSocketChannelClass == null) { + if (epollInfo == null) { log.log(Level.FINE, "Skipping recordTcpInfo because" - + "epollSocketChannelClass is null"); + + "epollInfo is null"); return; } - if (!epollSocketChannelClass.isInstance(channel)) { + if (!epollInfo.channelClass.isInstance(channel)) { log.log(Level.FINE, "Skipping recordTcpInfo because channel is not an" - + "instance of epollSocketChannelClass: {0}", channel.getClass() - .getName()); + + "instance of epollSocketChannelClass: {0}", + channel.getClass() + .getName()); return; } List labelValues = getLabelValues(channel); @@ -205,27 +178,26 @@ private void recordTcpInfo(Channel channel, boolean isClose) { long retransmits; long rtt; try { - tcpInfoMethod.invoke(channel, tcpInfo); - - totalRetrans = (Long) totalRetransMethod.invoke(tcpInfo); - retransmits = (Long) retransmitsMethod.invoke(tcpInfo); - rtt = (Long) rttMethod.invoke(tcpInfo); - } catch (Exception e) { + epollInfo.tcpInfo.invoke(channel, tcpInfo); + totalRetrans = (Long) epollInfo.totalRetrans.invoke(tcpInfo); + retransmits = (Long) epollInfo.retransmits.invoke(tcpInfo); + rtt = (Long) epollInfo.rtt.invoke(tcpInfo); + } catch (ReflectiveOperationException e) { log.log(Level.FINE, "Error computing TCP metrics", e); return; } long deltaTotal = totalRetrans - lastTotalRetrans; if (deltaTotal > 0) { - metricRecorder.addLongCounter(metrics.packetsRetransmitted, deltaTotal, - Collections.emptyList(), labelValues); + metricRecorder.addLongCounter(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT, + deltaTotal, Collections.emptyList(), labelValues); lastTotalRetrans = totalRetrans; } if (isClose && retransmits > 0) { - metricRecorder.addLongCounter(metrics.recurringRetransmits, retransmits, - Collections.emptyList(), labelValues); + metricRecorder.addLongCounter(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT, + retransmits, Collections.emptyList(), labelValues); } - metricRecorder.recordDoubleHistogram(metrics.minRtt, + metricRecorder.recordDoubleHistogram(InternalTcpMetrics.MIN_RTT_INSTRUMENT, rtt / 1000000.0, // Convert microseconds to seconds Collections.emptyList(), labelValues); } @@ -236,24 +208,24 @@ private static List getLabelValues(Channel channel) { String localPort = ""; String peerAddress = ""; String peerPort = ""; - + SocketAddress local = channel.localAddress(); if (local instanceof InetSocketAddress) { InetSocketAddress inetLocal = (InetSocketAddress) local; localAddress = inetLocal.getAddress().getHostAddress(); localPort = String.valueOf(inetLocal.getPort()); } - + SocketAddress remote = channel.remoteAddress(); if (remote instanceof InetSocketAddress) { InetSocketAddress inetRemote = (InetSocketAddress) remote; peerAddress = inetRemote.getAddress().getHostAddress(); peerPort = String.valueOf(inetRemote.getPort()); } - + return Arrays.asList(localAddress, localPort, peerAddress, peerPort); } - - private TcpMetrics() {} + private TcpMetrics() { + } } diff --git a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java index 08ec1e83b84..7023acc947c 100644 --- a/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java @@ -37,7 +37,6 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -230,32 +229,31 @@ public void setSoLingerChannelOption() throws IOException, GeneralSecurityExcept // set SO_LINGER option int soLinger = 123; channelOptions.put(ChannelOption.SO_LINGER, soLinger); - NettyClientTransport transport = - new NettyClientTransport( - address, - new ReflectiveChannelFactory<>(NioSocketChannel.class), - channelOptions, - group, - newNegotiator(), - false, - DEFAULT_WINDOW_SIZE, - DEFAULT_MAX_MESSAGE_SIZE, - GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, - GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, - KEEPALIVE_TIME_NANOS_DISABLED, - 1L, - false, - authority, - null /* user agent */, - tooManyPingsRunnable, - new TransportTracer(), - Attributes.EMPTY, - new SocketPicker(), - new FakeChannelLogger(), - false, - new MetricRecorder() { - }, - Ticker.systemTicker()); + NettyClientTransport transport = new NettyClientTransport( + address, + new ReflectiveChannelFactory<>(NioSocketChannel.class), + channelOptions, + group, + newNegotiator(), + false, + DEFAULT_WINDOW_SIZE, + DEFAULT_MAX_MESSAGE_SIZE, + GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, + GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, + KEEPALIVE_TIME_NANOS_DISABLED, + 1L, + false, + authority, + null /* user agent */, + tooManyPingsRunnable, + new TransportTracer(), + Attributes.EMPTY, + new SocketPicker(), + new FakeChannelLogger(), + false, + new MetricRecorder() { + }, + Ticker.systemTicker()); transports.add(transport); callMeMaybe(transport.start(clientTransportListener)); @@ -507,32 +505,31 @@ private static class CantConstructChannelError extends Error {} public void failingToConstructChannelShouldFailGracefully() throws Exception { address = TestUtils.testServerAddress(new InetSocketAddress(12345)); authority = GrpcUtil.authorityFromHostAndPort(address.getHostString(), address.getPort()); - NettyClientTransport transport = - new NettyClientTransport( - address, - new ReflectiveChannelFactory<>(CantConstructChannel.class), - new HashMap, Object>(), - group, - newNegotiator(), - false, - DEFAULT_WINDOW_SIZE, - DEFAULT_MAX_MESSAGE_SIZE, - GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, - GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, - KEEPALIVE_TIME_NANOS_DISABLED, - 1, - false, - authority, - null, - tooManyPingsRunnable, - new TransportTracer(), - Attributes.EMPTY, - new SocketPicker(), - new FakeChannelLogger(), - false, - new MetricRecorder() { - }, - Ticker.systemTicker()); + NettyClientTransport transport = new NettyClientTransport( + address, + new ReflectiveChannelFactory<>(CantConstructChannel.class), + new HashMap, Object>(), + group, + newNegotiator(), + false, + DEFAULT_WINDOW_SIZE, + DEFAULT_MAX_MESSAGE_SIZE, + GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, + GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE, + KEEPALIVE_TIME_NANOS_DISABLED, + 1, + false, + authority, + null, + tooManyPingsRunnable, + new TransportTracer(), + Attributes.EMPTY, + new SocketPicker(), + new FakeChannelLogger(), + false, + new MetricRecorder() { + }, + Ticker.systemTicker()); transports.add(transport); // Should not throw @@ -995,7 +992,7 @@ public void authorityOverrideInCallOptions_matchesServerPeerHost_newStreamCreati new Rpc(transport, new Metadata(), "foo.test.google.fr").waitForResponse(); } finally { - NettyClientHandler.enablePerRpcAuthorityCheck = false;; + NettyClientHandler.enablePerRpcAuthorityCheck = false; } } @@ -1018,7 +1015,7 @@ public void authorityOverrideInCallOptions_portNumberInAuthority_isStrippedForPe new Rpc(transport, new Metadata(), "foo.test.google.fr:12345").waitForResponse(); } finally { - NettyClientHandler.enablePerRpcAuthorityCheck = false;; + NettyClientHandler.enablePerRpcAuthorityCheck = false; } } @@ -1052,7 +1049,7 @@ public void authorityOverrideInCallOptions_portNumberAndIpv6_isStrippedForPeerVe "No subject alternative names matching IP address 2001:db8:3333:4444:5555:6666:1.2.3.4 " + "found"); } finally { - NettyClientHandler.enablePerRpcAuthorityCheck = false;; + NettyClientHandler.enablePerRpcAuthorityCheck = false; } } @@ -1131,32 +1128,31 @@ private NettyClientTransport newTransport(ProtocolNegotiator negotiator, int max if (!enableKeepAlive) { keepAliveTimeNano = KEEPALIVE_TIME_NANOS_DISABLED; } - NettyClientTransport transport = - new NettyClientTransport( - address, - channelFactory, - new HashMap, Object>(), - group, - negotiator, - false, - DEFAULT_WINDOW_SIZE, - maxMsgSize, - maxHeaderListSize, - maxHeaderListSize, - keepAliveTimeNano, - keepAliveTimeoutNano, - false, - authority, - userAgent, - tooManyPingsRunnable, - new TransportTracer(), - eagAttributes, - new SocketPicker(), - new FakeChannelLogger(), - false, - new MetricRecorder() { - }, - Ticker.systemTicker()); + NettyClientTransport transport = new NettyClientTransport( + address, + channelFactory, + new HashMap, Object>(), + group, + negotiator, + false, + DEFAULT_WINDOW_SIZE, + maxMsgSize, + maxHeaderListSize, + maxHeaderListSize, + keepAliveTimeNano, + keepAliveTimeoutNano, + false, + authority, + userAgent, + tooManyPingsRunnable, + new TransportTracer(), + eagAttributes, + new SocketPicker(), + new FakeChannelLogger(), + false, + new MetricRecorder() { + }, + Ticker.systemTicker()); transports.add(transport); return transport; } @@ -1175,36 +1171,35 @@ private void startServer(int maxStreamsPerConnection, int maxHeaderListSize) thr private void startServer(int maxStreamsPerConnection, int maxHeaderListSize, ServerListener serverListener) throws IOException { - server = - new NettyServer( - TestUtils.testServerAddresses(new InetSocketAddress(0)), - new ReflectiveChannelFactory<>(NioServerSocketChannel.class), - new HashMap, Object>(), - new HashMap, Object>(), - new FixedObjectPool<>(group), - new FixedObjectPool<>(group), - false, - negotiator, - Collections.emptyList(), - TransportTracer.getDefaultFactory(), - maxStreamsPerConnection, - false, - DEFAULT_WINDOW_SIZE, - DEFAULT_MAX_MESSAGE_SIZE, - maxHeaderListSize, - maxHeaderListSize, - DEFAULT_SERVER_KEEPALIVE_TIME_NANOS, - DEFAULT_SERVER_KEEPALIVE_TIMEOUT_NANOS, - MAX_CONNECTION_IDLE_NANOS_DISABLED, - MAX_CONNECTION_AGE_NANOS_DISABLED, - MAX_CONNECTION_AGE_GRACE_NANOS_INFINITE, - true, - 0, - MAX_RST_COUNT_DISABLED, - 0, - Attributes.EMPTY, - channelz, - mock(MetricRecorder.class)); + server = new NettyServer( + TestUtils.testServerAddresses(new InetSocketAddress(0)), + new ReflectiveChannelFactory<>(NioServerSocketChannel.class), + new HashMap, Object>(), + new HashMap, Object>(), + new FixedObjectPool<>(group), + new FixedObjectPool<>(group), + false, + negotiator, + Collections.emptyList(), + TransportTracer.getDefaultFactory(), + maxStreamsPerConnection, + false, + DEFAULT_WINDOW_SIZE, + DEFAULT_MAX_MESSAGE_SIZE, + maxHeaderListSize, + maxHeaderListSize, + DEFAULT_SERVER_KEEPALIVE_TIME_NANOS, + DEFAULT_SERVER_KEEPALIVE_TIMEOUT_NANOS, + MAX_CONNECTION_IDLE_NANOS_DISABLED, + MAX_CONNECTION_AGE_NANOS_DISABLED, + MAX_CONNECTION_AGE_GRACE_NANOS_INFINITE, + true, + 0, + MAX_RST_COUNT_DISABLED, + 0, + Attributes.EMPTY, + channelz, + new MetricRecorder() {}); server.start(serverListener); address = TestUtils.testServerAddress((InetSocketAddress) server.getListenSocketAddress()); authority = GrpcUtil.authorityFromHostAndPort(address.getHostString(), address.getPort()); diff --git a/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java b/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java index 2fef700970e..f3b73a515b5 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerBuilderTest.java @@ -191,13 +191,4 @@ public void useNioTransport_shouldNotThrow() { builder.assertEventLoopsAndChannelType(); } - @Test - public void metricRecorder_propagatedToServer() { - MetricRecorder recorder = mock(MetricRecorder.class); - - NettyServer server = builder.buildTransportServers( - ImmutableList.of(), recorder); - - assertThat(server.getMetricRecorder()).isSameInstanceAs(recorder); - } } diff --git a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java index 5c010793e08..2c0ab21cb56 100644 --- a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java +++ b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java @@ -130,6 +130,7 @@ public class NettyServerHandlerTest extends NettyHandlerTestBase streamListenerMessageQueue = new LinkedList<>(); @@ -206,6 +207,20 @@ protected void manualSetUp() throws Exception { channel().releaseOutbound(); } + @Test + public void tcpMetrics_recorded() throws Exception { + manualSetUp(); + handler().channelActive(ctx()); + // Verify that channelActive triggered TcpMetrics + ArgumentCaptor countCaptor = ArgumentCaptor.forClass(Long.class); + verify(metricRecorder, atLeastOnce()).addLongCounter( + eq(io.grpc.InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT), + countCaptor.capture(), + any(), + any()); + assertEquals(1L, countCaptor.getValue().longValue()); + } + @Test public void transportReadyDelayedUntilConnectionPreface() throws Exception { initChannel(new GrpcHttp2ServerHeadersDecoder(GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE)); @@ -1418,7 +1433,7 @@ protected NettyServerHandler newHandler() { maxRstPeriodNanos, Attributes.EMPTY, fakeClock().getTicker(), - new MetricRecorder() {}); + metricRecorder); } @Override diff --git a/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java b/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java index 90499b59c85..874025369ee 100644 --- a/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java +++ b/netty/src/test/java/io/grpc/netty/TcpMetricsTest.java @@ -24,6 +24,7 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; +import io.grpc.InternalTcpMetrics; import io.grpc.MetricRecorder; import io.netty.channel.Channel; import io.netty.channel.EventLoop; @@ -49,7 +50,8 @@ @RunWith(JUnit4.class) public class TcpMetricsTest { - @Rule public final MockitoRule mocks = MockitoJUnit.rule(); + @Rule + public final MockitoRule mocks = MockitoJUnit.rule(); @Mock private MetricRecorder metricRecorder; @@ -63,22 +65,21 @@ public class TcpMetricsTest { private TcpMetrics.Tracker metrics; @Before - public void setUp() { + public void setUp() throws Exception { when(channel.eventLoop()).thenReturn(eventLoop); when(eventLoop.schedule(any(Runnable.class), anyLong(), any(TimeUnit.class))) - .thenAnswer(invocation -> scheduledFuture); + .thenAnswer(invocation -> scheduledFuture); metrics = new TcpMetrics.Tracker(metricRecorder); } @Test - public void metricsInitialization() { - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); - - org.junit.Assert.assertNotNull(metrics.connectionsCreated); - org.junit.Assert.assertNotNull(metrics.connectionCount); - org.junit.Assert.assertNotNull(metrics.packetsRetransmitted); - org.junit.Assert.assertNotNull(metrics.recurringRetransmits); - org.junit.Assert.assertNotNull(metrics.minRtt); + public void metricsInitialization() throws Exception { + + org.junit.Assert.assertNotNull(InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT); + org.junit.Assert.assertNotNull(InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT); + org.junit.Assert.assertNotNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT); + org.junit.Assert.assertNotNull(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT); + org.junit.Assert.assertNotNull(InternalTcpMetrics.MIN_RTT_INSTRUMENT); } public static class FakeEpollTcpInfo { @@ -109,13 +110,16 @@ public long rtt() { } @Test - public void tracker_recordTcpInfo_reflectionSuccess() { + public void tracker_recordTcpInfo_reflectionSuccess() throws Exception { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); - - TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, - ConfigurableFakeWithTcpInfo.class.getName(), - FakeEpollTcpInfo.class.getName()); + TcpMetrics.epollInfo = new TcpMetrics.EpollInfo( + ConfigurableFakeWithTcpInfo.class, FakeEpollTcpInfo.class, + FakeEpollTcpInfo.class.getConstructor(), + ConfigurableFakeWithTcpInfo.class.getMethod("tcpInfo", FakeEpollTcpInfo.class), + FakeEpollTcpInfo.class.getMethod("totalRetrans"), + FakeEpollTcpInfo.class.getMethod("retrans"), + FakeEpollTcpInfo.class.getMethod("rtt")); + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder); FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); infoSource.setValues(123, 4, 5000); @@ -124,27 +128,33 @@ public void tracker_recordTcpInfo_reflectionSuccess() { tracker.channelInactive(channel); - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), eq(123L), any(), any()); - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.recurringRetransmits)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT)), eq(4L), any(), any()); - verify(recorder).recordDoubleHistogram(eq(Objects.requireNonNull(metrics.minRtt)), + verify(recorder).recordDoubleHistogram( + eq(Objects.requireNonNull(InternalTcpMetrics.MIN_RTT_INSTRUMENT)), eq(0.005), any(), any()); } @Test - public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() { + public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() throws Exception { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); - - TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, - ConfigurableFakeWithTcpInfo.class.getName(), - FakeEpollTcpInfo.class.getName()); + TcpMetrics.epollInfo = new TcpMetrics.EpollInfo( + ConfigurableFakeWithTcpInfo.class, FakeEpollTcpInfo.class, + FakeEpollTcpInfo.class.getConstructor(), + ConfigurableFakeWithTcpInfo.class.getMethod("tcpInfo", FakeEpollTcpInfo.class), + FakeEpollTcpInfo.class.getMethod("totalRetrans"), + FakeEpollTcpInfo.class.getMethod("retrans"), + FakeEpollTcpInfo.class.getMethod("rtt")); + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder); FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); infoSource.setValues(123, 4, 5000); - ConfigurableFakeWithTcpInfo channel = - org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource)); + ConfigurableFakeWithTcpInfo channel = org.mockito.Mockito.spy( + new ConfigurableFakeWithTcpInfo(infoSource)); when(channel.eventLoop()).thenReturn(eventLoop); when(channel.isActive()).thenReturn(true); @@ -157,29 +167,35 @@ public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() { org.mockito.Mockito.clearInvocations(recorder); periodicTask.run(); - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), eq(123L), any(), any()); - verify(recorder).recordDoubleHistogram(eq(Objects.requireNonNull(metrics.minRtt)), + verify(recorder).recordDoubleHistogram( + eq(Objects.requireNonNull(InternalTcpMetrics.MIN_RTT_INSTRUMENT)), eq(0.005), any(), any()); // Should NOT record recurring retransmits during periodic polling verify(recorder, org.mockito.Mockito.never()) - .addLongCounter(eq(Objects.requireNonNull(metrics.recurringRetransmits)), + .addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT)), anyLong(), any(), any()); } @Test - public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() { + public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() throws Exception { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); - - TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, - ConfigurableFakeWithTcpInfo.class.getName(), - FakeEpollTcpInfo.class.getName()); + TcpMetrics.epollInfo = new TcpMetrics.EpollInfo( + ConfigurableFakeWithTcpInfo.class, FakeEpollTcpInfo.class, + FakeEpollTcpInfo.class.getConstructor(), + ConfigurableFakeWithTcpInfo.class.getMethod("tcpInfo", FakeEpollTcpInfo.class), + FakeEpollTcpInfo.class.getMethod("totalRetrans"), + FakeEpollTcpInfo.class.getMethod("retrans"), + FakeEpollTcpInfo.class.getMethod("rtt")); + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder); FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); infoSource.setValues(123, 4, 5000); - ConfigurableFakeWithTcpInfo channel = - org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource)); + ConfigurableFakeWithTcpInfo channel = org.mockito.Mockito.spy( + new ConfigurableFakeWithTcpInfo(infoSource)); when(channel.eventLoop()).thenReturn(eventLoop); when(channel.isActive()).thenReturn(true); @@ -196,33 +212,38 @@ public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() { // Let's just create a new channel instance where tcpInfo sets retrans=5. FakeEpollTcpInfo infoSource2 = new FakeEpollTcpInfo(); infoSource2.setValues(130, 5, 5000); - ConfigurableFakeWithTcpInfo channel2 = - org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource2)); + ConfigurableFakeWithTcpInfo channel2 = org.mockito.Mockito.spy( + new ConfigurableFakeWithTcpInfo(infoSource2)); when(channel2.eventLoop()).thenReturn(eventLoop); tracker.channelInactive(channel2); // It should record delta for totalRetrans (130 - 123 = 7) - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), eq(7L), any(), any()); // But for recurringRetransmits it MUST record the raw value 5, not the delta! - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.recurringRetransmits)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT)), eq(5L), any(), any()); } @Test - public void tracker_periodicRecord_reportsDeltaForTotalRetrans() { + public void tracker_periodicRecord_reportsDeltaForTotalRetrans() throws Exception { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); - - TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, - ConfigurableFakeWithTcpInfo.class.getName(), - FakeEpollTcpInfo.class.getName()); + TcpMetrics.epollInfo = new TcpMetrics.EpollInfo( + ConfigurableFakeWithTcpInfo.class, FakeEpollTcpInfo.class, + FakeEpollTcpInfo.class.getConstructor(), + ConfigurableFakeWithTcpInfo.class.getMethod("tcpInfo", FakeEpollTcpInfo.class), + FakeEpollTcpInfo.class.getMethod("totalRetrans"), + FakeEpollTcpInfo.class.getMethod("retrans"), + FakeEpollTcpInfo.class.getMethod("rtt")); + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder); FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); infoSource.setValues(123, 4, 5000); - ConfigurableFakeWithTcpInfo channel = - org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource)); + ConfigurableFakeWithTcpInfo channel = org.mockito.Mockito.spy( + new ConfigurableFakeWithTcpInfo(infoSource)); when(channel.eventLoop()).thenReturn(eventLoop); when(channel.isActive()).thenReturn(true); @@ -235,7 +256,8 @@ public void tracker_periodicRecord_reportsDeltaForTotalRetrans() { // First periodic record org.mockito.Mockito.clearInvocations(recorder); periodicTask.run(); - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), eq(123L), any(), any()); // Change tcpInfo for second periodic record @@ -251,28 +273,34 @@ public void tracker_periodicRecord_reportsDeltaForTotalRetrans() { periodicTask.run(); // Only the delta (150 - 123 = 27) should be recorded - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), eq(27L), any(), any()); - verify(recorder).recordDoubleHistogram(eq(Objects.requireNonNull(metrics.minRtt)), + verify(recorder).recordDoubleHistogram( + eq(Objects.requireNonNull(InternalTcpMetrics.MIN_RTT_INSTRUMENT)), eq(0.006), any(), any()); verify(recorder, org.mockito.Mockito.never()) - .addLongCounter(eq(Objects.requireNonNull(metrics.recurringRetransmits)), + .addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT)), anyLong(), any(), any()); } @Test - public void tracker_periodicRecord_doesNotReportZeroDeltaForTotalRetrans() { + public void tracker_periodicRecord_doesNotReportZeroDeltaForTotalRetrans() throws Exception { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); - - TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, - ConfigurableFakeWithTcpInfo.class.getName(), - FakeEpollTcpInfo.class.getName()); + TcpMetrics.epollInfo = new TcpMetrics.EpollInfo( + ConfigurableFakeWithTcpInfo.class, FakeEpollTcpInfo.class, + FakeEpollTcpInfo.class.getConstructor(), + ConfigurableFakeWithTcpInfo.class.getMethod("tcpInfo", FakeEpollTcpInfo.class), + FakeEpollTcpInfo.class.getMethod("totalRetrans"), + FakeEpollTcpInfo.class.getMethod("retrans"), + FakeEpollTcpInfo.class.getMethod("rtt")); + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder); FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); infoSource.setValues(123, 4, 5000); - ConfigurableFakeWithTcpInfo channel = - org.mockito.Mockito.spy(new ConfigurableFakeWithTcpInfo(infoSource)); + ConfigurableFakeWithTcpInfo channel = org.mockito.Mockito.spy( + new ConfigurableFakeWithTcpInfo(infoSource)); when(channel.eventLoop()).thenReturn(eventLoop); when(channel.isActive()).thenReturn(true); @@ -291,9 +319,11 @@ public void tracker_periodicRecord_doesNotReportZeroDeltaForTotalRetrans() { // NO delta (123 - 123 = 0), so it should not be recorded verify(recorder, org.mockito.Mockito.never()) - .addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + .addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), anyLong(), any(), any()); - verify(recorder).recordDoubleHistogram(eq(Objects.requireNonNull(metrics.minRtt)), + verify(recorder).recordDoubleHistogram( + eq(Objects.requireNonNull(InternalTcpMetrics.MIN_RTT_INSTRUMENT)), eq(0.005), any(), any()); } @@ -313,15 +343,17 @@ public void tcpInfo(FakeEpollTcpInfo info) { } @Test - public void tracker_reportsDeltas_correctly() { + public void tracker_reportsDeltas_correctly() throws Exception { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); - String fakeChannelName = ConfigurableFakeWithTcpInfo.class.getName(); - String fakeInfoName = FakeEpollTcpInfo.class.getName(); - - TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, - fakeChannelName, fakeInfoName); + TcpMetrics.epollInfo = new TcpMetrics.EpollInfo( + ConfigurableFakeWithTcpInfo.class, FakeEpollTcpInfo.class, + FakeEpollTcpInfo.class.getConstructor(), + ConfigurableFakeWithTcpInfo.class.getMethod("tcpInfo", FakeEpollTcpInfo.class), + FakeEpollTcpInfo.class.getMethod("totalRetrans"), + FakeEpollTcpInfo.class.getMethod("retrans"), + FakeEpollTcpInfo.class.getMethod("rtt")); + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder); FakeEpollTcpInfo infoSource = new FakeEpollTcpInfo(); ConfigurableFakeWithTcpInfo channel = new ConfigurableFakeWithTcpInfo(infoSource); @@ -330,14 +362,16 @@ public void tracker_reportsDeltas_correctly() { infoSource.setValues(10, 2, 1000); tracker.recordTcpInfo(channel); - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), eq(10L), any(), any()); // 15 retransmits total (delta 5) infoSource.setValues(15, 0, 1000); tracker.recordTcpInfo(channel); - verify(recorder).addLongCounter(eq(Objects.requireNonNull(metrics.packetsRetransmitted)), + verify(recorder).addLongCounter( + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), eq(5L), any(), any()); // 15 retransmits total (delta 0) - should NOT report @@ -347,36 +381,35 @@ public void tracker_reportsDeltas_correctly() { // Verify no new interactions with this specific metric and value // We can't easily verify "no interaction" for specific value without capturing. verify(recorder, org.mockito.Mockito.times(1)).addLongCounter( - eq(Objects.requireNonNull(metrics.packetsRetransmitted)), - eq(10L), any(), any()); + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), + eq(10L), any(), any()); verify(recorder, org.mockito.Mockito.times(1)).addLongCounter( - eq(Objects.requireNonNull(metrics.packetsRetransmitted)), - eq(5L), any(), any()); + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), + eq(5L), any(), any()); // Total interactions for packetsRetransmitted should be 2 verify(recorder, org.mockito.Mockito.times(2)).addLongCounter( - eq(Objects.requireNonNull(metrics.packetsRetransmitted)), - anyLong(), any(), any()); + eq(Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT)), + anyLong(), any(), any()); // recurringRetransmits should NOT have been reported yet (periodic calls) verify(recorder, org.mockito.Mockito.times(0)).addLongCounter( - eq(Objects.requireNonNull(metrics.recurringRetransmits)), - anyLong(), any(), any()); + eq(Objects.requireNonNull(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT)), + anyLong(), any(), any()); // Close channel - should report recurringRetransmits tracker.channelInactive(channel); verify(recorder, org.mockito.Mockito.times(1)).addLongCounter( - eq(Objects.requireNonNull(metrics.recurringRetransmits)), + eq(Objects.requireNonNull(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT)), eq(1L), // From last infoSource setValues(15, 1, 1000) any(), any()); } @Test - public void tracker_recordTcpInfo_reflectionFailure() { + public void tracker_recordTcpInfo_reflectionFailure() throws Exception { MetricRecorder recorder = mock(MetricRecorder.class); - TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(); - TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics, - "non.existent.Class", "non.existent.Info"); + TcpMetrics.epollInfo = null; + TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder); Channel channel = org.mockito.Mockito.mock(Channel.class); when(channel.isActive()).thenReturn(true); @@ -384,35 +417,34 @@ public void tracker_recordTcpInfo_reflectionFailure() { // Should catch exception and ignore tracker.channelInactive(channel); } - + @Test - public void registeredMetrics_haveCorrectOptionalLabels() { + public void registeredMetrics_haveCorrectOptionalLabels() throws Exception { List expectedOptionalLabels = Arrays.asList( "network.local.address", "network.local.port", "network.peer.address", - "network.peer.port" - ); + "network.peer.port"); org.junit.Assert.assertEquals( expectedOptionalLabels, - TcpMetrics.getDefaultMetrics().connectionsCreated.getOptionalLabelKeys()); + InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT.getOptionalLabelKeys()); org.junit.Assert.assertEquals( expectedOptionalLabels, - TcpMetrics.getDefaultMetrics().connectionCount.getOptionalLabelKeys()); + InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT.getOptionalLabelKeys()); - if (TcpMetrics.getDefaultMetrics().packetsRetransmitted != null) { + if (InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT != null) { org.junit.Assert.assertEquals( expectedOptionalLabels, - Objects.requireNonNull(TcpMetrics.getDefaultMetrics().packetsRetransmitted) + Objects.requireNonNull(InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT) .getOptionalLabelKeys()); org.junit.Assert.assertEquals( expectedOptionalLabels, - Objects.requireNonNull(TcpMetrics.getDefaultMetrics().recurringRetransmits) + Objects.requireNonNull(InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT) .getOptionalLabelKeys()); org.junit.Assert.assertEquals( expectedOptionalLabels, - Objects.requireNonNull(TcpMetrics.getDefaultMetrics().minRtt).getOptionalLabelKeys()); + Objects.requireNonNull(InternalTcpMetrics.MIN_RTT_INSTRUMENT).getOptionalLabelKeys()); } } @@ -425,16 +457,16 @@ public void channelActive_extractsLabels_ipv4() throws Exception { when(channel.remoteAddress()).thenReturn(new InetSocketAddress(remoteInet, 443)); metrics.channelActive(channel); - + verify(metricRecorder).addLongCounter( - eq(TcpMetrics.getDefaultMetrics().connectionsCreated), eq(1L), + eq(InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT), eq(1L), eq(Collections.emptyList()), - eq(Arrays.asList( + eq(Arrays.asList( localInet.getHostAddress(), "8080", remoteInet.getHostAddress(), "443"))); verify(metricRecorder).addLongUpDownCounter( - eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(1L), + eq(InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT), eq(1L), eq(Collections.emptyList()), - eq(Arrays.asList( + eq(Arrays.asList( localInet.getHostAddress(), "8080", remoteInet.getHostAddress(), "443"))); verifyNoMoreInteractions(metricRecorder); } @@ -450,67 +482,68 @@ public void channelInactive_extractsLabels_ipv6() throws Exception { when(channel.remoteAddress()).thenReturn(new InetSocketAddress(remoteInet, 443)); metrics.channelInactive(channel); - + verify(metricRecorder).addLongUpDownCounter( - eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(-1L), + eq(InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT), eq(-1L), eq(Collections.emptyList()), - eq(Arrays.asList( + eq(Arrays.asList( localInet.getHostAddress(), "8080", remoteInet.getHostAddress(), "443"))); verifyNoMoreInteractions(metricRecorder); } @Test - public void channelActive_extractsLabels_nonInetAddress() { - SocketAddress dummyAddress = new SocketAddress() {}; + public void channelActive_extractsLabels_nonInetAddress() throws Exception { + SocketAddress dummyAddress = new SocketAddress() { + }; when(channel.localAddress()).thenReturn(dummyAddress); when(channel.remoteAddress()).thenReturn(dummyAddress); metrics.channelActive(channel); - + verify(metricRecorder).addLongCounter( - eq(TcpMetrics.getDefaultMetrics().connectionsCreated), eq(1L), + eq(InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT), eq(1L), eq(Collections.emptyList()), - eq(Arrays.asList("", "", "", ""))); + eq(Arrays.asList("", "", "", ""))); verify(metricRecorder).addLongUpDownCounter( - eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(1L), + eq(InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT), eq(1L), eq(Collections.emptyList()), - eq(Arrays.asList("", "", "", ""))); + eq(Arrays.asList("", "", "", ""))); verifyNoMoreInteractions(metricRecorder); } @Test - public void channelActive_incrementsCounts() { + public void channelActive_incrementsCounts() throws Exception { metrics.channelActive(channel); verify(metricRecorder).addLongCounter( - eq(TcpMetrics.getDefaultMetrics().connectionsCreated), eq(1L), + eq(InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT), eq(1L), eq(Collections.emptyList()), - eq(Arrays.asList("", "", "", ""))); + eq(Arrays.asList("", "", "", ""))); verify(metricRecorder).addLongUpDownCounter( - eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(1L), + eq(InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT), eq(1L), eq(Collections.emptyList()), - eq(Arrays.asList("", "", "", ""))); + eq(Arrays.asList("", "", "", ""))); verifyNoMoreInteractions(metricRecorder); } @Test - public void channelInactive_decrementsCount_noEpoll_noError() { + public void channelInactive_decrementsCount_noEpoll_noError() throws Exception { metrics.channelInactive(channel); verify(metricRecorder).addLongUpDownCounter( - eq(TcpMetrics.getDefaultMetrics().connectionCount), eq(-1L), + eq(InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT), eq(-1L), eq(Collections.emptyList()), - eq(Arrays.asList("", "", "", ""))); + eq(Arrays.asList("", "", "", ""))); verifyNoMoreInteractions(metricRecorder); } @Test - public void channelActive_schedulesReportTimer() { + public void channelActive_schedulesReportTimer() throws Exception { when(channel.isActive()).thenReturn(true); metrics.channelActive(channel); ArgumentCaptor runnableCaptor = ArgumentCaptor.forClass(Runnable.class); ArgumentCaptor delayCaptor = ArgumentCaptor.forClass(Long.class); verify(eventLoop).schedule( - runnableCaptor.capture(), delayCaptor.capture(), eq(TimeUnit.MILLISECONDS)); + runnableCaptor.capture(), delayCaptor.capture(), eq(TimeUnit.MILLISECONDS)); Runnable task = runnableCaptor.getValue(); long delay = delayCaptor.getValue(); @@ -535,7 +568,7 @@ public void channelActive_schedulesReportTimer() { } @Test - public void channelInactive_cancelsReportTimer() { + public void channelInactive_cancelsReportTimer() throws Exception { when(channel.isActive()).thenReturn(true); metrics.channelActive(channel); diff --git a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java index df9756333b5..0032972756d 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/InternalOkHttpServerBuilder.java @@ -32,7 +32,7 @@ public final class InternalOkHttpServerBuilder { public static InternalServer buildTransportServers(OkHttpServerBuilder builder, List streamTracerFactories, MetricRecorder metricRecorder) { - return builder.buildTransportServers(streamTracerFactories, metricRecorder); + return builder.buildTransportServers(streamTracerFactories); } public static void setTransportTracerFactory(OkHttpServerBuilder builder, diff --git a/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java b/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java index 3ace69052b7..163d2023b1c 100644 --- a/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java +++ b/okhttp/src/main/java/io/grpc/okhttp/OkHttpServerBuilder.java @@ -112,7 +112,15 @@ public static OkHttpServerBuilder forPort(SocketAddress address, ServerCredentia return new OkHttpServerBuilder(address, result.factory); } - final ServerImplBuilder serverImplBuilder = new ServerImplBuilder(this::buildTransportServers); + final ServerImplBuilder serverImplBuilder = new ServerImplBuilder( + new ServerImplBuilder.ClientTransportServersBuilder() { + @Override + public InternalServer buildClientTransportServers( + List streamTracerFactories, + MetricRecorder metricRecorder) { + return buildTransportServers(streamTracerFactories); + } + }); final SocketAddress listenAddress; final HandshakerSocketFactory handshakerSocketFactory; TransportTracer.Factory transportTracerFactory = TransportTracer.getDefaultFactory(); @@ -388,8 +396,7 @@ void setStatsEnabled(boolean value) { } InternalServer buildTransportServers( - List streamTracerFactories, - MetricRecorder metricRecorder) { + List streamTracerFactories) { return new OkHttpServer(this, streamTracerFactories, serverImplBuilder.getChannelz()); } diff --git a/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java b/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java index dfcd2f1c29f..87ad61c9f27 100644 --- a/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java +++ b/opentelemetry/src/main/java/io/grpc/opentelemetry/GrpcOpenTelemetry.java @@ -136,7 +136,7 @@ List getOptionalLabels() { return optionalLabels; } - public MetricSink getSink() { + MetricSink getSink() { return sink; } diff --git a/opentelemetry/src/test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java b/opentelemetry/src/test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java index 68cb53d015b..f0bd6f93098 100644 --- a/opentelemetry/src/test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java +++ b/opentelemetry/src/test/java/io/grpc/opentelemetry/GrpcOpenTelemetryTest.java @@ -26,7 +26,6 @@ import com.google.common.collect.ImmutableList; import io.grpc.ClientInterceptor; import io.grpc.ManagedChannelBuilder; -import io.grpc.MetricSink; import io.grpc.ServerBuilder; import io.grpc.internal.GrpcUtil; import io.grpc.opentelemetry.GrpcOpenTelemetry.TargetFilter; @@ -122,7 +121,6 @@ public void builderDefaults() { .build()); assertThat(module.getEnableMetrics()).isEmpty(); assertThat(module.getOptionalLabels()).isEmpty(); - assertThat(module.getSink()).isInstanceOf(MetricSink.class); assertThat(module.getTracer()).isSameInstanceAs(noopOpenTelemetry .getTracerProvider() diff --git a/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java b/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java index 240394b1e9e..59936cdd485 100644 --- a/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java +++ b/servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java @@ -18,7 +18,6 @@ import io.grpc.InternalChannelz; import io.grpc.InternalInstrumented; -import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.ClientTransportFactory; @@ -60,9 +59,7 @@ public class JettyTransportTest extends AbstractTransportTest { protected InternalServer newServer(List streamTracerFactories) { return new InternalServer() { final InternalServer delegate = - new ServletServerBuilder().buildTransportServers( - streamTracerFactories, new MetricRecorder() { - }); + new ServletServerBuilder().buildTransportServers(streamTracerFactories); @Override public void start(ServerListener listener) throws IOException { diff --git a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java index 17e32f3007c..5bea4c6e03b 100644 --- a/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java +++ b/servlet/src/main/java/io/grpc/servlet/ServletServerBuilder.java @@ -31,7 +31,6 @@ import io.grpc.InternalInstrumented; import io.grpc.InternalLogId; import io.grpc.Metadata; -import io.grpc.MetricRecorder; import io.grpc.Server; import io.grpc.ServerBuilder; import io.grpc.ServerStreamTracer; @@ -79,7 +78,9 @@ public final class ServletServerBuilder extends ForwardingServerBuilder + buildTransportServers(streamTracerFactories)); } /** @@ -160,8 +161,7 @@ public void transportTerminated() { @VisibleForTesting InternalServer buildTransportServers( - List streamTracerFactories, - MetricRecorder metricRecorder) { + List streamTracerFactories) { checkNotNull(streamTracerFactories, "streamTracerFactories"); this.streamTracerFactories = streamTracerFactories; internalServer = new InternalServerImpl(); diff --git a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java index d869f25ca4f..cd73b096ccb 100644 --- a/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java +++ b/servlet/src/tomcatTest/java/io/grpc/servlet/TomcatTransportTest.java @@ -18,7 +18,6 @@ import io.grpc.InternalChannelz.SocketStats; import io.grpc.InternalInstrumented; -import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.ClientTransportFactory; @@ -72,11 +71,8 @@ public void tearDown() throws InterruptedException { @Override protected InternalServer newServer(List streamTracerFactories) { return new InternalServer() { - final ServletServerBuilder builder = new ServletServerBuilder(); final InternalServer delegate = - builder.buildTransportServers( - streamTracerFactories, new MetricRecorder() { - }); + new ServletServerBuilder().buildTransportServers(streamTracerFactories); @Override public void start(ServerListener listener) throws IOException { diff --git a/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java b/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java index c072b465c77..ef897c87d70 100644 --- a/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java +++ b/servlet/src/undertowTest/java/io/grpc/servlet/UndertowTransportTest.java @@ -22,7 +22,6 @@ import io.grpc.InternalChannelz.SocketStats; import io.grpc.InternalInstrumented; -import io.grpc.MetricRecorder; import io.grpc.ServerStreamTracer; import io.grpc.internal.AbstractTransportTest; import io.grpc.internal.ClientTransportFactory; @@ -92,9 +91,7 @@ protected InternalServer newServer(List streamTracerFactories) { return new InternalServer() { final InternalServer delegate = - new ServletServerBuilder().buildTransportServers( - streamTracerFactories, new MetricRecorder() { - }); + new ServletServerBuilder().buildTransportServers(streamTracerFactories); @Override public void start(ServerListener listener) throws IOException { diff --git a/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java b/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java index b2139a0ee6e..1b767e0303c 100644 --- a/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java +++ b/xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java @@ -75,7 +75,7 @@ public static OrcaMetricReportingServerInterceptor getInstance() { * higher precedence compared to metrics from {@link MetricRecorder}. */ public static OrcaMetricReportingServerInterceptor create( - MetricRecorder metricRecorder) { + @Nullable MetricRecorder metricRecorder) { return new OrcaMetricReportingServerInterceptor(metricRecorder); }