Skip to content

Commit b12a433

Browse files
AgraVatorAgraVator
committed
fix: make metric recorder non null and register metrics unconditionally
1 parent 2fda6fa commit b12a433

6 files changed

Lines changed: 43 additions & 71 deletions

File tree

‎api/src/main/java/io/grpc/NameResolver.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -369,7 +369,8 @@ private Args(Builder builder) {
369369
this.channelLogger = builder.channelLogger;
370370
this.executor = builder.executor;
371371
this.overrideAuthority = builder.overrideAuthority;
372-
this.metricRecorder = builder.metricRecorder;
372+
this.metricRecorder = builder.metricRecorder != null ? builder.metricRecorder
373+
: new MetricRecorder() {};
373374
this.nameResolverRegistry = builder.nameResolverRegistry;
374375
this.customArgs = cloneCustomArgs(builder.customArgs);
375376
}
@@ -679,7 +680,7 @@ public <T> Builder setArg(Key<T> key, T value) {
679680
* See {@link Args#getMetricRecorder()}. This is an optional field.
680681
*/
681682
public Builder setMetricRecorder(MetricRecorder metricRecorder) {
682-
this.metricRecorder = metricRecorder;
683+
this.metricRecorder = checkNotNull(metricRecorder);
683684
return this;
684685
}
685686

‎netty/src/main/java/io/grpc/netty/NettyServer.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,6 @@
6969
import java.util.concurrent.Callable;
7070
import java.util.logging.Level;
7171
import java.util.logging.Logger;
72-
import javax.annotation.Nullable;
7372

7473
/**
7574
* Netty-based server implementation.
@@ -178,11 +177,9 @@ class NettyServer implements InternalServer, InternalWithLogId {
178177
this.channelz = Preconditions.checkNotNull(channelz);
179178
this.logId = InternalLogId.allocate(getClass(), addresses.isEmpty() ? "No address" :
180179
String.valueOf(addresses));
181-
this.metricRecorder = metricRecorder;
182180
}
183181

184182
@VisibleForTesting
185-
@Nullable
186183
MetricRecorder getMetricRecorder() {
187184
return metricRecorder;
188185
}

‎netty/src/main/java/io/grpc/netty/TcpMetrics.java‎

Lines changed: 23 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package io.grpc.netty;
1818

19+
import com.google.common.annotations.VisibleForTesting;
1920
import io.grpc.DoubleHistogramMetricInstrument;
2021
import io.grpc.InternalTcpMetrics;
2122
import io.grpc.LongCounterMetricInstrument;
@@ -29,7 +30,6 @@
2930
import java.util.Arrays;
3031
import java.util.Collections;
3132
import java.util.List;
32-
import java.util.concurrent.RejectedExecutionException;
3333
import java.util.concurrent.ThreadLocalRandom;
3434
import java.util.concurrent.TimeUnit;
3535
import java.util.logging.Level;
@@ -58,9 +58,10 @@ final class TcpMetrics {
5858
}
5959
log.log(Level.INFO, "Epoll available during static init of TcpMetrics:"
6060
+ "{0}", epollAvailable);
61-
DEFAULT_METRICS = new Metrics(epollAvailable);
61+
DEFAULT_METRICS = new Metrics();
6262
}
6363

64+
@VisibleForTesting
6465
static Metrics getDefaultMetrics() {
6566
return DEFAULT_METRICS;
6667
}
@@ -72,19 +73,12 @@ static final class Metrics {
7273
final LongCounterMetricInstrument recurringRetransmits;
7374
final DoubleHistogramMetricInstrument minRtt;
7475

75-
Metrics(boolean epollAvailable) {
76+
Metrics() {
7677
connectionsCreated = InternalTcpMetrics.CONNECTIONS_CREATED_INSTRUMENT;
7778
connectionCount = InternalTcpMetrics.CONNECTION_COUNT_INSTRUMENT;
78-
79-
if (epollAvailable) {
80-
packetsRetransmitted = InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT;
81-
recurringRetransmits = InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT;
82-
minRtt = InternalTcpMetrics.MIN_RTT_INSTRUMENT;
83-
} else {
84-
packetsRetransmitted = null;
85-
recurringRetransmits = null;
86-
minRtt = null;
87-
}
79+
packetsRetransmitted = InternalTcpMetrics.PACKETS_RETRANSMITTED_INSTRUMENT;
80+
recurringRetransmits = InternalTcpMetrics.RECURRING_RETRANSMITS_INSTRUMENT;
81+
minRtt = InternalTcpMetrics.MIN_RTT_INSTRUMENT;
8882
}
8983
}
9084

@@ -171,17 +165,12 @@ private void scheduleNextReport(final Channel channel, boolean isInitial) {
171165
: 0.9 + ThreadLocalRandom.current().nextDouble() * 0.2; // 90% to 110%
172166
long rearmingDelay = (long) (RECORD_INTERVAL_MILLIS * jitter);
173167

174-
try {
175-
reportTimer = channel.eventLoop().schedule(() -> {
176-
if (channel.isActive()) {
177-
Tracker.this.recordTcpInfo(channel, false);
178-
scheduleNextReport(channel, false); // Re-arm
179-
}
180-
}, rearmingDelay, TimeUnit.MILLISECONDS);
181-
} catch (RejectedExecutionException e) {
182-
log.log(Level.FINE, "Failed to schedule next TCP metrics report", e);
183-
// The event loop is likely shutting down. We can safely ignore this.
184-
}
168+
reportTimer = channel.eventLoop().schedule(() -> {
169+
if (channel.isActive()) {
170+
Tracker.this.recordTcpInfo(channel, false);
171+
scheduleNextReport(channel, false); // Re-arm
172+
}
173+
}, rearmingDelay, TimeUnit.MILLISECONDS);
185174
}
186175

187176
void channelInactive(Channel channel) {
@@ -226,25 +215,19 @@ private void recordTcpInfo(Channel channel, boolean isClose) {
226215
return;
227216
}
228217

229-
if (metrics.packetsRetransmitted != null) {
230-
long deltaTotal = totalRetrans - lastTotalRetrans;
231-
if (deltaTotal > 0) {
232-
metricRecorder.addLongCounter(metrics.packetsRetransmitted, deltaTotal,
233-
Collections.emptyList(), labelValues);
234-
lastTotalRetrans = totalRetrans;
235-
}
236-
}
237-
if (metrics.recurringRetransmits != null && isClose) {
238-
if (retransmits > 0) {
239-
metricRecorder.addLongCounter(metrics.recurringRetransmits, retransmits,
240-
Collections.emptyList(), labelValues);
241-
}
218+
long deltaTotal = totalRetrans - lastTotalRetrans;
219+
if (deltaTotal > 0) {
220+
metricRecorder.addLongCounter(metrics.packetsRetransmitted, deltaTotal,
221+
Collections.emptyList(), labelValues);
222+
lastTotalRetrans = totalRetrans;
242223
}
243-
if (metrics.minRtt != null) {
244-
metricRecorder.recordDoubleHistogram(metrics.minRtt,
245-
rtt / 1000000.0, // Convert microseconds to seconds
224+
if (isClose && retransmits > 0) {
225+
metricRecorder.addLongCounter(metrics.recurringRetransmits, retransmits,
246226
Collections.emptyList(), labelValues);
247227
}
228+
metricRecorder.recordDoubleHistogram(metrics.minRtt,
229+
rtt / 1000000.0, // Convert microseconds to seconds
230+
Collections.emptyList(), labelValues);
248231
}
249232
}
250233

‎netty/src/test/java/io/grpc/netty/TcpMetricsTest.java‎

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -71,19 +71,8 @@ public void setUp() {
7171
}
7272

7373
@Test
74-
public void metricsInitialization_epollUnavailable() {
75-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(false);
76-
77-
org.junit.Assert.assertNotNull(metrics.connectionsCreated);
78-
org.junit.Assert.assertNotNull(metrics.connectionCount);
79-
org.junit.Assert.assertNull(metrics.packetsRetransmitted);
80-
org.junit.Assert.assertNull(metrics.recurringRetransmits);
81-
org.junit.Assert.assertNull(metrics.minRtt);
82-
}
83-
84-
@Test
85-
public void metricsInitialization_epollAvailable() {
86-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true);
74+
public void metricsInitialization() {
75+
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics();
8776

8877
org.junit.Assert.assertNotNull(metrics.connectionsCreated);
8978
org.junit.Assert.assertNotNull(metrics.connectionCount);
@@ -122,7 +111,7 @@ public long rtt() {
122111
@Test
123112
public void tracker_recordTcpInfo_reflectionSuccess() {
124113
MetricRecorder recorder = mock(MetricRecorder.class);
125-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true);
114+
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics();
126115

127116
TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics,
128117
ConfigurableFakeWithTcpInfo.class.getName(),
@@ -146,7 +135,7 @@ public void tracker_recordTcpInfo_reflectionSuccess() {
146135
@Test
147136
public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() {
148137
MetricRecorder recorder = mock(MetricRecorder.class);
149-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true);
138+
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics();
150139

151140
TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics,
152141
ConfigurableFakeWithTcpInfo.class.getName(),
@@ -181,7 +170,7 @@ public void tracker_periodicRecord_doesNotRecordRecurringRetransmits() {
181170
@Test
182171
public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() {
183172
MetricRecorder recorder = mock(MetricRecorder.class);
184-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true);
173+
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics();
185174

186175
TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics,
187176
ConfigurableFakeWithTcpInfo.class.getName(),
@@ -224,7 +213,7 @@ public void tracker_channelInactive_recordsRecurringRetransmits_raw_notDelta() {
224213
@Test
225214
public void tracker_periodicRecord_reportsDeltaForTotalRetrans() {
226215
MetricRecorder recorder = mock(MetricRecorder.class);
227-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true);
216+
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics();
228217

229218
TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics,
230219
ConfigurableFakeWithTcpInfo.class.getName(),
@@ -274,7 +263,7 @@ public void tracker_periodicRecord_reportsDeltaForTotalRetrans() {
274263
@Test
275264
public void tracker_periodicRecord_doesNotReportZeroDeltaForTotalRetrans() {
276265
MetricRecorder recorder = mock(MetricRecorder.class);
277-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true);
266+
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics();
278267

279268
TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics,
280269
ConfigurableFakeWithTcpInfo.class.getName(),
@@ -326,7 +315,7 @@ public void tcpInfo(FakeEpollTcpInfo info) {
326315
@Test
327316
public void tracker_reportsDeltas_correctly() {
328317
MetricRecorder recorder = mock(MetricRecorder.class);
329-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true);
318+
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics();
330319

331320
String fakeChannelName = ConfigurableFakeWithTcpInfo.class.getName();
332321
String fakeInfoName = FakeEpollTcpInfo.class.getName();
@@ -384,7 +373,7 @@ public void tracker_reportsDeltas_correctly() {
384373
@Test
385374
public void tracker_recordTcpInfo_reflectionFailure() {
386375
MetricRecorder recorder = mock(MetricRecorder.class);
387-
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics(true);
376+
TcpMetrics.Metrics metrics = new TcpMetrics.Metrics();
388377

389378
TcpMetrics.Tracker tracker = new TcpMetrics.Tracker(recorder, metrics,
390379
"non.existent.Class", "non.existent.Info");

‎xds/src/main/java/io/grpc/xds/XdsNameResolver.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,18 +1052,18 @@ private static final class BootstrappingXdsClientPool implements XdsClientPool {
10521052
private final XdsClientPoolFactory xdsClientPoolFactory;
10531053
private final String target;
10541054
private final @Nullable Map<String, ?> bootstrapOverride;
1055-
private final @Nullable MetricRecorder metricRecorder;
1055+
private final MetricRecorder metricRecorder;
10561056
private ObjectPool<XdsClient> xdsClientPool;
10571057

10581058
BootstrappingXdsClientPool(
10591059
XdsClientPoolFactory xdsClientPoolFactory,
10601060
String target,
10611061
@Nullable Map<String, ?> bootstrapOverride,
1062-
@Nullable MetricRecorder metricRecorder) {
1062+
MetricRecorder metricRecorder) {
10631063
this.xdsClientPoolFactory = checkNotNull(xdsClientPoolFactory, "xdsClientPoolFactory");
10641064
this.target = checkNotNull(target, "target");
10651065
this.bootstrapOverride = bootstrapOverride;
1066-
this.metricRecorder = metricRecorder;
1066+
this.metricRecorder = checkNotNull(metricRecorder, "metricRecorder");
10671067
}
10681068

10691069
@Override

‎xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package io.grpc.xds.orca;
1818

19+
import static com.google.common.base.Preconditions.checkNotNull;
20+
1921
import com.github.xds.data.orca.v3.OrcaLoadReport;
2022
import com.google.common.annotations.VisibleForTesting;
2123
import io.grpc.Context;
@@ -60,8 +62,8 @@ public final class OrcaMetricReportingServerInterceptor implements ServerInterce
6062
private final MetricRecorder metricRecorder;
6163

6264
@VisibleForTesting
63-
OrcaMetricReportingServerInterceptor(@Nullable MetricRecorder metricRecorder) {
64-
this.metricRecorder = metricRecorder;
65+
OrcaMetricReportingServerInterceptor(MetricRecorder metricRecorder) {
66+
this.metricRecorder = checkNotNull(metricRecorder, "metricRecorder");
6567
}
6668

6769
public static OrcaMetricReportingServerInterceptor getInstance() {
@@ -75,7 +77,7 @@ public static OrcaMetricReportingServerInterceptor getInstance() {
7577
* higher precedence compared to metrics from {@link MetricRecorder}.
7678
*/
7779
public static OrcaMetricReportingServerInterceptor create(
78-
@Nullable MetricRecorder metricRecorder) {
80+
MetricRecorder metricRecorder) {
7981
return new OrcaMetricReportingServerInterceptor(metricRecorder);
8082
}
8183

0 commit comments

Comments
 (0)