services,orca: update backend metrics support to allow for server-wide metrics recording (per-call and OOB) by danielzhaotongliu · Pull Request #9902 · grpc/grpc-java

danielzhaotongliu · 2023-02-17T18:29:43Z

Design doc: go/grpc-backend-metrics-update

This PR includes:

Add range validation checks for setters in MetricRecorder and CallMetricRecorder (for consistency with the OrcaLoadReport proto and "push down" validation checks as early as possible)
MetricRecorder is passed to constructor for OrcaMetricReportingServerInterceptor (for per-call load reporting) and if the MetricRecorder is set, then merges with metrics from CallMetricRecorder which takes a higher precedence.
update/add unit tests such that it reflects the range validation checks

linux-foundation-easycla · 2023-02-17T18:29:54Z

The committers listed above are authorized under a signed CLA.

✅ login: danielzhaotongliu / name: Daniel Liu (755a50e, d6f1167, 28d4d8d, edcb0e0)

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

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

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

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

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

…icRecorder in per-call reporting

…verInterceptor & update tests

…ingleton for ORCA server interceptor

…ting, addressed other review comments

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

ejona86

Assuming "don't break the existing API" is fixed, this is fine.

@YifeiZhuang, do you think the changes to the example and interop-testing won't break anything ("the example doesn't work like it claims" or "interop-testing now fails")?

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

…es/BUILD.bazel

danielzhaotongliu · 2023-03-30T20:42:56Z

@YifeiZhuang could you also adding another kokoro:run label after your review since there was a previous kokoro build failure that I fixed in the latest commit. I followed go/grpc-local-testing#kokoro-ephemeral-one-off-builds using my fork, branch and the job as grpc/java/master/presubmit/bazel.cfg and but it gave error: Pool 'grpc-ubuntu16' does not exist for cluster 'UNKNOWN_CLUSTER'.

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

ejona86 · 2023-03-30T21:03:39Z

go/grpc-local-testing#kokoro-ephemeral-one-off-builds

That's C-centric documentation. I don't know what it does. I don't know if you have permission, but if you notice a Kokoro build flake you may be able to re-trigger via:

Click "Details"
Click "Invocation Details"
Follow "FUSION_URL"
Look on the left for the run that has a blue box around it, which denotes the run you are currently looking at. (Dunno if you have permissions here, but I'd wager you do:) Click the "rebuild" button in the top-right of that's run's small box

To try against your own repo, you can do the same flow (choose a random Details from a random PR), but instead of "rebuild":

Create a PR in your repo, just to make the tool happy
Click "Trigger Build" at the top-right
Set "Owner" to be your github username
Set PR number
Click Trigger

YifeiZhuang · 2023-03-30T22:00:16Z

Assuming "don't break the existing API" is fixed, this is fine.

@YifeiZhuang, do you think the changes to the example and interop-testing won't break anything ("the example doesn't work like it claims" or "interop-testing now fails")?

it does look like the orca-oob intergration test is failing.

zivy-macbookpro:grpc-java-v1 zivy$ ./run-test-server.sh --use_tls=false
Server started on port 8080

zivy-macbookpro:grpc-java-v1 zivy$ ./run-test-client.sh --use_tls=false --test_case=orca_oob --service_config_json='{"loadBalancingConfig":[{"test_backend_metrics_load_balancer":{}}]}'
Running test orca_oob
Exception in thread "main" expected to be less than: 5
but was                 : 5
	at io.grpc.testing.integration.AbstractInteropTest.testOrcaOob(AbstractInteropTest.java:1830)
	at io.grpc.testing.integration.TestServiceClient.runTest(TestServiceClient.java:499)
	at io.grpc.testing.integration.TestServiceClient.run(TestServiceClient.java:274)
	at io.grpc.testing.integration.TestServiceClient.main(TestServiceClient.java:70)

YifeiZhuang

Could you apply change:

diff --git a/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java b/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java
index a8d4776ad..2f0575b6c 100644
--- a/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java
+++ b/interop-testing/src/main/java/io/grpc/testing/integration/AbstractInteropTest.java
@@ -1777,7 +1777,7 @@ public abstract class AbstractInteropTest {
     final TestOrcaReport answer2 = TestOrcaReport.newBuilder()
         .setCpuUtilization(0.29309)
         .setMemoryUtilization(0.2)
-        .putUtilization("util", 100.2039)
+        .putUtilization("util", 0.2039)
         .build();

This would prevent interop test from failing.
refer to: grpc/grpc@fd7c85f

…il for AbstractInteropTest

danielzhaotongliu · 2023-03-31T05:20:16Z

Applied change from #9902 (review) to AbstractInteropTest.java in latest commit.

danielzhaotongliu marked this pull request as ready for review February 18, 2023 02:09

ejona86 requested a review from YifeiZhuang February 23, 2023 18:48

YifeiZhuang reviewed Feb 24, 2023

View reviewed changes

xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java Outdated Show resolved Hide resolved

ejona86 reviewed Mar 6, 2023

View reviewed changes

xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java Outdated Show resolved Hide resolved

YifeiZhuang approved these changes Mar 6, 2023

View reviewed changes

ejona86 reviewed Mar 15, 2023

View reviewed changes

danielzhaotongliu added 9 commits March 20, 2023 21:20

add range validation & merge metrics from MetricRecorder and CallMetr…

54a51c1

…icRecorder in per-call reporting

add/modify unit tests for OrcaServiceImpl & CallMetricRecorder

e9267a0

add/modify unit tests for OrcaMetricReportingServerInterceptor

e6caf14

fix indentation

82575e1

add tests for MetricRecorder and merging of metrics

34ec7f0

make MetricReporter non-static and volatile in OrcaMetricReportingSer…

98c2dd7

…verInterceptor & update tests

MetricRecorder is final & passed into ctor, update to DCL lazy init s…

7d1c666

…ingleton for ORCA server interceptor

Remove unused method in ORCA server interceptor

150fb71

merge all metrics & optimize to 1 Builder.build(), remove file format…

d07c661

…ting, addressed other review comments

danielzhaotongliu force-pushed the server-metric-recorder branch from bbd99e1 to d07c661 Compare March 20, 2023 21:21

danielzhaotongliu added 3 commits March 20, 2023 21:59

remove unnecessary code styling

9ff90ce

add default create() method to replace old getInstance() method

e11426c

remove unnecessary null check in OrcaMetricReportingServerInterceptor

2b32b98

danielzhaotongliu commented Mar 21, 2023

View reviewed changes

xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java Outdated Show resolved Hide resolved

danielzhaotongliu requested a review from ejona86 March 21, 2023 01:10

create() allows null & interceptor handles null

096193e

ejona86 approved these changes Mar 28, 2023

View reviewed changes

xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java Outdated Show resolved Hide resolved

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 28, 2023

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 28, 2023

sergiitk mentioned this pull request Mar 29, 2023

A51 update: add ServerMetricRecorder API and QPS field grpc/proposal#345

Merged

fix kokoro build failure: add MetricRecorderHelper.java dep to servic…

9c8d7a9

…es/BUILD.bazel

danielzhaotongliu requested a review from YifeiZhuang March 30, 2023 20:36

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 30, 2023

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 30, 2023

ejona86 reviewed Mar 30, 2023

View reviewed changes

xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/orca/OrcaMetricReportingServerInterceptor.java Outdated Show resolved Hide resolved

YifeiZhuang reviewed Mar 30, 2023

View reviewed changes

keep getInstance(), add Javadoc for create(MetricRecorder), update ut…

395957d

…il for AbstractInteropTest

ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 31, 2023

grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 31, 2023

ejona86 merged commit 5201e49 into grpc:master Apr 10, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 10, 2023

Conversation

danielzhaotongliu commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danielzhaotongliu commented Mar 30, 2023

Uh oh!

Uh oh!

Uh oh!

ejona86 commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YifeiZhuang commented Mar 30, 2023

Uh oh!

YifeiZhuang left a comment

Choose a reason for hiding this comment

Uh oh!

danielzhaotongliu commented Mar 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

danielzhaotongliu commented Feb 17, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 17, 2023 •

edited

Loading

ejona86 commented Mar 30, 2023 •

edited

Loading