Skip to content

Commit bbd99e1

Browse files
danielzhaotongliudanielzhaotongliu
committed
merge all metrics & optimize to 1 Builder.build(), remove file formatting, addressed other review comments
1 parent 1bc4f15 commit bbd99e1

File tree

8 files changed

+63
-94
lines changed

8 files changed

+63
-94
lines changed

‎examples/example-orca/src/main/java/io/grpc/examples/orca/CustomBackendMetricsServer.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ private void start() throws IOException {
6363
// Enable OOB custom backend metrics reporting.
6464
.addService(orcaOobService)
6565
// Enable per-query custom backend metrics reporting.
66-
.intercept(OrcaMetricReportingServerInterceptor.getOrCreateInstance(metricRecorder))
66+
.intercept(OrcaMetricReportingServerInterceptor.create(metricRecorder))
6767
.build()
6868
.start();
6969
logger.info("Server started, listening on " + port);

‎interop-testing/src/main/java/io/grpc/testing/integration/TestServiceServer.java‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,9 @@
3434
import java.util.concurrent.ScheduledExecutorService;
3535
import java.util.concurrent.TimeUnit;
3636

37-
/**
38-
* Server that manages startup/shutdown of a single {@code TestService}.
39-
*/
37+
/** Server that manages startup/shutdown of a single {@code TestService}. */
4038
public class TestServiceServer {
41-
42-
/**
43-
* The main application allowing this server to be launched from the command line.
44-
*/
39+
/** The main application allowing this server to be launched from the command line. */
4540
public static void main(String[] args) throws Exception {
4641
// Let Netty use Conscrypt if it is available.
4742
TestUtils.installConscryptIfAvailable();
@@ -169,7 +164,7 @@ void start() throws Exception {
169164
ServerInterceptors.intercept(
170165
new TestServiceImpl(executor, metricRecorder), TestServiceImpl.interceptors()))
171166
.addService(orcaOobService)
172-
.intercept(OrcaMetricReportingServerInterceptor.getOrCreateInstance(metricRecorder))
167+
.intercept(OrcaMetricReportingServerInterceptor.create(metricRecorder))
173168
.build()
174169
.start();
175170
}
@@ -188,9 +183,7 @@ int getPort() {
188183
return server.getPort();
189184
}
190185

191-
/**
192-
* Await termination on the main thread since the grpc library uses daemon threads.
193-
*/
186+
/** Await termination on the main thread since the grpc library uses daemon threads. */
194187
private void blockUntilShutdown() throws InterruptedException {
195188
if (server != null) {
196189
server.awaitTermination();

‎services/src/main/java/io/grpc/services/CallMetricRecorder.java‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/6012")
3333
@ThreadSafe
3434
public final class CallMetricRecorder {
35-
3635
private static final CallMetricRecorder NOOP = new CallMetricRecorder().disable();
3736

3837
static final Context.Key<CallMetricRecorder> CONTEXT_KEY =
@@ -67,7 +66,7 @@ public static CallMetricRecorder getCurrent() {
6766

6867
/**
6968
* Records a call metric measurement for utilization in the range [0, 1]. Values outside the valid
70-
* range are rejected. If RPC has already finished, this method is no-op.
69+
* range are ignored. If RPC has already finished, this method is no-op.
7170
*
7271
* <p>A latter record will overwrite its former name-sakes.
7372
*
@@ -88,15 +87,15 @@ public CallMetricRecorder recordUtilizationMetric(String name, double value) {
8887
}
8988

9089
/**
91-
* Records a call metric measurement for request cost. If RPC has already finished, this method is
92-
* no-op.
90+
* Records a call metric measurement for request cost.
91+
* If RPC has already finished, this method is no-op.
9392
*
9493
* <p>A latter record will overwrite its former name-sakes.
9594
*
9695
* @return this recorder object
9796
* @since 1.47.0
98-
* @deprecated use {@link #recordRequestCostMetric} instead. This method will be removed in the
99-
* future.
97+
* @deprecated use {@link #recordRequestCostMetric} instead.
98+
* This method will be removed in the future.
10099
*/
101100
@Deprecated
102101
@InlineMe(replacement = "this.recordRequestCostMetric(name, value)")
@@ -105,8 +104,8 @@ public CallMetricRecorder recordCallMetric(String name, double value) {
105104
}
106105

107106
/**
108-
* Records a call metric measurement for request cost. If RPC has already finished, this method is
109-
* no-op.
107+
* Records a call metric measurement for request cost.
108+
* If RPC has already finished, this method is no-op.
110109
*
111110
* <p>A latter record will overwrite its former name-sakes.
112111
*
@@ -128,7 +127,7 @@ public CallMetricRecorder recordRequestCostMetric(String name, double value) {
128127

129128
/**
130129
* Records a call metric measurement for CPU utilization in the range [0, 1]. Values outside the
131-
* valid range are rejected. If RPC has already finished, this method is no-op.
130+
* valid range are ignored. If RPC has already finished, this method is no-op.
132131
*
133132
* <p>A latter record will overwrite its former name-sakes.
134133
*
@@ -145,7 +144,7 @@ public CallMetricRecorder recordCpuUtilizationMetric(double value) {
145144

146145
/**
147146
* Records a call metric measurement for memory utilization in the range [0, 1]. Values outside
148-
* the valid range are rejected. If RPC has already finished, this method is no-op.
147+
* the valid range are ignored. If RPC has already finished, this method is no-op.
149148
*
150149
* <p>A latter record will overwrite its former name-sakes.
151150
*
@@ -162,7 +161,7 @@ public CallMetricRecorder recordMemoryUtilizationMetric(double value) {
162161

163162
/**
164163
* Records a call metric measurement for qps in the range [0, inf). Values outside the valid range
165-
* are rejected. If RPC has already finished, this method is no-op.
164+
* are ignored. If RPC has already finished, this method is no-op.
166165
*
167166
* <p>A latter record will overwrite its former name-sakes.
168167
*
@@ -177,6 +176,7 @@ public CallMetricRecorder recordQpsMetric(double value) {
177176
return this;
178177
}
179178

179+
180180
/**
181181
* Returns all request cost metric values. No more metric values will be recorded after this
182182
* method is called. Calling this method multiple times returns the same collection of metric

‎services/src/main/java/io/grpc/services/MetricRecorder.java‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,11 @@
2222
import java.util.concurrent.ConcurrentHashMap;
2323

2424
/**
25-
* Implements the service/APIs for Out-of-Band metrics reporting, only for utilization metrics. A
26-
* user should use the public set-APIs to update the server machine's utilization metrics data.
25+
* Implements the service/APIs for Out-of-Band metrics reporting, only for utilization metrics.
26+
* A user should use the public set-APIs to update the server machine's utilization metrics data.
2727
*/
2828
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/9006")
2929
public final class MetricRecorder {
30-
3130
private volatile ConcurrentHashMap<String, Double> metricsData = new ConcurrentHashMap<>();
3231
private volatile double cpuUtilization;
3332
private volatile double memoryUtilization;
@@ -37,12 +36,11 @@ public static MetricRecorder newInstance() {
3736
return new MetricRecorder();
3837
}
3938

40-
private MetricRecorder() {
41-
}
39+
private MetricRecorder() {}
4240

4341
/**
4442
* Update the metrics value in the range [0, 1] corresponding to the specified key. Values outside
45-
* the valid range are rejected.
43+
* the valid range are ignored.
4644
*/
4745
public void putUtilizationMetric(String key, double value) {
4846
if (!MetricRecorderHelper.isUtilizationValid(value)) {
@@ -67,7 +65,7 @@ public void removeUtilizationMetric(String key) {
6765

6866
/**
6967
* Update the CPU utilization metrics data in the range [0, 1]. Values outside the valid range are
70-
* rejected.
68+
* ignored.
7169
*/
7270
public void setCpuUtilizationMetric(double value) {
7371
if (!MetricRecorderHelper.isUtilizationValid(value)) {
@@ -85,7 +83,7 @@ public void clearCpuUtilizationMetric() {
8583

8684
/**
8785
* Update the memory utilization metrics data in the range [0, 1]. Values outside the valid range
88-
* are rejected.
86+
* are ignored.
8987
*/
9088
public void setMemoryUtilizationMetric(double value) {
9189
if (!MetricRecorderHelper.isUtilizationValid(value)) {
@@ -102,8 +100,7 @@ public void clearMemoryUtilizationMetric() {
102100
}
103101

104102
/**
105-
* Update the QPS metrics data in the range [0, inf). Values outside the valid range are
106-
* rejected.
103+
* Update the QPS metrics data in the range [0, inf). Values outside the valid range are ignored.
107104
*/
108105
public void setQps(double value) {
109106
if (!MetricRecorderHelper.isQpsValid(value)) {
@@ -120,7 +117,7 @@ public void clearQps() {
120117
}
121118

122119
MetricReport getMetricReport() {
123-
return new MetricReport(cpuUtilization, memoryUtilization, qps, Collections.emptyMap(),
124-
Collections.unmodifiableMap(metricsData));
120+
return new MetricReport(cpuUtilization, memoryUtilization, qps,
121+
Collections.emptyMap(), Collections.unmodifiableMap(metricsData));
125122
}
126123
}

‎services/src/test/java/io/grpc/services/CallMetricRecorderTest.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,7 @@
2525
import org.junit.runner.RunWith;
2626
import org.junit.runners.JUnit4;
2727

28-
/**
29-
* Tests for {@link CallMetricRecorder}.
30-
*/
28+
/** Tests for {@link CallMetricRecorder}. */
3129
@RunWith(JUnit4.class)
3230
public class CallMetricRecorderTest {
3331

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

Lines changed: 26 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -38,16 +38,14 @@
3838
/**
3939
* A {@link ServerInterceptor} that intercepts a {@link ServerCall} by running server-side RPC
4040
* handling under a {@link Context} that records custom per-request metrics provided by server
41-
* applications and sends to client side along with the response in the format of Open Request Cost
42-
* Aggregation (ORCA).
41+
* applications and sends to client side along with the response in the format of Open Request
42+
* Cost Aggregation (ORCA).
4343
*
4444
* @since 1.23.0
4545
*/
4646
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/9127")
4747
public final class OrcaMetricReportingServerInterceptor implements ServerInterceptor {
4848

49-
private static volatile OrcaMetricReportingServerInterceptor instance;
50-
5149
@VisibleForTesting
5250
static final Metadata.Key<OrcaLoadReport> ORCA_ENDPOINT_LOAD_METRICS_KEY =
5351
Metadata.Key.of(
@@ -59,19 +57,10 @@ public final class OrcaMetricReportingServerInterceptor implements ServerInterce
5957
@VisibleForTesting
6058
OrcaMetricReportingServerInterceptor(MetricRecorder metricRecorder) {
6159
this.metricRecorder = metricRecorder;
62-
OrcaMetricReportingServerInterceptor.instance = this;
6360
}
6461

65-
public static OrcaMetricReportingServerInterceptor getOrCreateInstance(
66-
MetricRecorder metricRecorder) {
67-
if (instance == null) {
68-
synchronized (OrcaMetricReportingServerInterceptor.class) {
69-
if (instance == null) {
70-
instance = new OrcaMetricReportingServerInterceptor(metricRecorder);
71-
}
72-
}
73-
}
74-
return instance;
62+
public static OrcaMetricReportingServerInterceptor create(MetricRecorder metricRecorder) {
63+
return new OrcaMetricReportingServerInterceptor(metricRecorder);
7564
}
7665

7766
@Override
@@ -88,12 +77,12 @@ public <ReqT, RespT> Listener<ReqT> interceptCall(
8877
new SimpleForwardingServerCall<ReqT, RespT>(call) {
8978
@Override
9079
public void close(Status status, Metadata trailers) {
91-
OrcaLoadReport report = fromInternalReport(
80+
OrcaLoadReport.Builder reportBuilder = metricRecorder != null ? fromInternalReport(
81+
InternalMetricRecorder.getMetricReport(metricRecorder)) :
82+
OrcaLoadReport.newBuilder();
83+
mergeMetrics(reportBuilder,
9284
InternalCallMetricRecorder.finalizeAndDump2(finalCallMetricRecorder));
93-
if (metricRecorder != null) {
94-
report = mergeMetrics(report,
95-
fromInternalReport(InternalMetricRecorder.getMetricReport(metricRecorder)));
96-
}
85+
OrcaLoadReport report = reportBuilder.build();
9786
if (!report.equals(OrcaLoadReport.getDefaultInstance())) {
9887
trailers.put(ORCA_ENDPOINT_LOAD_METRICS_KEY, report);
9988
}
@@ -107,43 +96,39 @@ public void close(Status status, Metadata trailers) {
10796
next);
10897
}
10998

110-
private static OrcaLoadReport fromInternalReport(MetricReport internalReport) {
99+
private static OrcaLoadReport.Builder fromInternalReport(MetricReport internalReport) {
111100
return OrcaLoadReport.newBuilder()
112101
.setCpuUtilization(internalReport.getCpuUtilization())
113102
.setMemUtilization(internalReport.getMemoryUtilization())
114103
.setRpsFractional(internalReport.getQps())
115104
.putAllUtilization(internalReport.getUtilizationMetrics())
116-
.putAllRequestCost(internalReport.getRequestCostMetrics())
117-
.build();
105+
.putAllRequestCost(internalReport.getRequestCostMetrics());
118106
}
119107

120108
/**
121-
* Return a merged {@link OrcaLoadReport} where the metrics from {@link CallMetricRecorder} takes
122-
* a higher precedence compared to {@link MetricRecorder}.
109+
* Modify the given {@link OrcaLoadReport.Builder} containing metrics for {@link MetricRecorder}
110+
* such that metrics from the given {@link MetricReport} for {@link CallMetricRecorder} takes a
111+
* higher precedence.
123112
*/
124-
private static OrcaLoadReport mergeMetrics(OrcaLoadReport callMetricRecorderReport,
125-
OrcaLoadReport metricRecorderReport) {
126-
// Merge metrics from the MetricRecorder first since metrics from the CallMetricRecorder takes a
127-
// higher precedence.
128-
OrcaLoadReport.Builder builder = metricRecorderReport.toBuilder()
129-
.clearUtilization()
130-
.clearRequestCost()
131-
.putAllUtilization(callMetricRecorderReport.getUtilizationMap())
132-
.putAllRequestCost(callMetricRecorderReport.getRequestCostMap());
133-
// Overwrite only if the values from CallMetricRecorder are set
113+
private static void mergeMetrics(
114+
OrcaLoadReport.Builder metricRecorderReportBuilder,
115+
MetricReport callMetricRecorderReport
116+
) {
117+
metricRecorderReportBuilder.putAllUtilization(callMetricRecorderReport.getUtilizationMetrics())
118+
.putAllRequestCost(callMetricRecorderReport.getRequestCostMetrics());
119+
// Overwrite only if the values from the given MetricReport for CallMetricRecorder are set
134120
double cpu = callMetricRecorderReport.getCpuUtilization();
135121
if (isReportValueSet(cpu)) {
136-
builder.setCpuUtilization(cpu);
122+
metricRecorderReportBuilder.setCpuUtilization(cpu);
137123
}
138-
double mem = callMetricRecorderReport.getMemUtilization();
124+
double mem = callMetricRecorderReport.getMemoryUtilization();
139125
if (isReportValueSet(mem)) {
140-
builder.setMemUtilization(mem);
126+
metricRecorderReportBuilder.setMemUtilization(mem);
141127
}
142-
double rps = callMetricRecorderReport.getRpsFractional();
128+
double rps = callMetricRecorderReport.getQps();
143129
if (isReportValueSet(rps)) {
144-
builder.setRpsFractional(rps);
130+
metricRecorderReportBuilder.setRpsFractional(rps);
145131
}
146-
return builder.build();
147132
}
148133

149134
private static boolean isReportValueSet(double value) {

‎xds/src/test/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptorTest.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@ public class OrcaMetricReportingServerInterceptorTest {
7575
private double cpuUtilizationMetrics = 0;
7676
private double memoryUtilizationMetrics = 0;
7777
private double qpsMetrics = 0;
78-
7978
private MetricRecorder metricRecorder;
8079

8180
private final AtomicReference<Metadata> trailersCapture = new AtomicReference<>();
@@ -136,8 +135,7 @@ public ServerStreamTracer newServerStreamTracer(String fullMethodName, Metadata
136135
return new ServerStreamTracer() {
137136
@Override
138137
public Context filterContext(Context context) {
139-
return context.withValue(InternalCallMetricRecorder.CONTEXT_KEY,
140-
callMetricRecorder);
138+
return context.withValue(InternalCallMetricRecorder.CONTEXT_KEY, callMetricRecorder);
141139
}
142140
};
143141
}
@@ -196,12 +194,10 @@ public void responseTrailersContainAllReportedMetricsFromCallMetricRecorder() {
196194
cpuUtilizationMetrics = 0.3465;
197195
memoryUtilizationMetrics = 0.764;
198196
qpsMetrics = 3.1415926535;
199-
200197
ClientCalls.blockingUnaryCall(channelToUse, SIMPLE_METHOD, CallOptions.DEFAULT, REQUEST);
201198
Metadata receivedTrailers = trailersCapture.get();
202199
OrcaLoadReport report =
203200
receivedTrailers.get(OrcaMetricReportingServerInterceptor.ORCA_ENDPOINT_LOAD_METRICS_KEY);
204-
205201
assertThat(report.getUtilizationMap())
206202
.containsExactly("util1", 0.1082, "util2", 0.4936, "util3", 0.5342);
207203
assertThat(report.getRequestCostMap())
@@ -222,14 +218,17 @@ public void responseTrailersContainMergedMetricsFromCallMetricRecorderAndMetricR
222218
metricRecorder.setQps(1.618);
223219
metricRecorder.putUtilizationMetric("serverUtil1", 0.7467);
224220
metricRecorder.putUtilizationMetric("serverUtil2", 0.2233);
221+
metricRecorder.putUtilizationMetric("util1", 0.01);
222+
metricRecorder.putUtilizationMetric("util3", 0.99);
225223

226224
ClientCalls.blockingUnaryCall(channelToUse, SIMPLE_METHOD, CallOptions.DEFAULT, REQUEST);
227225
Metadata receivedTrailers = trailersCapture.get();
228226
OrcaLoadReport report =
229227
receivedTrailers.get(OrcaMetricReportingServerInterceptor.ORCA_ENDPOINT_LOAD_METRICS_KEY);
230228

231229
assertThat(report.getUtilizationMap())
232-
.containsExactly("util1", 0.1482, "util2", 0.4036, "util3", 0.5742);
230+
.containsExactly("util1", 0.1482, "util2", 0.4036, "util3", 0.5742, "serverUtil1", 0.7467,
231+
"serverUtil2", 0.2233);
233232
assertThat(report.getRequestCostMap()).isEmpty();
234233
assertThat(report.getCpuUtilization()).isEqualTo(0.3465);
235234
assertThat(report.getMemUtilization()).isEqualTo(0.967);
@@ -256,7 +255,6 @@ public void responseTrailersContainMergedMetricsFromCallMetricRecorderAndMetricR
256255
}
257256

258257
private static final class TrailersCapturingClientInterceptor implements ClientInterceptor {
259-
260258
final AtomicReference<Metadata> trailersCapture;
261259

262260
TrailersCapturingClientInterceptor(AtomicReference<Metadata> trailersCapture) {
@@ -284,7 +282,6 @@ public void start(Listener<RespT> responseListener, Metadata headers) {
284282

285283
private final class TrailersCapturingClientCallListener
286284
extends SimpleForwardingClientCallListener<RespT> {
287-
288285
TrailersCapturingClientCallListener(ClientCall.Listener<RespT> responseListener) {
289286
super(responseListener);
290287
}

0 commit comments

Comments
 (0)