Skip to content

Conversation

@danielzhaotongliu
Copy link
Collaborator

@danielzhaotongliu danielzhaotongliu commented Feb 17, 2023

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
Copy link

linux-foundation-easycla bot commented Feb 17, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@danielzhaotongliu danielzhaotongliu marked this pull request as ready for review February 18, 2023 02:09
@ejona86 ejona86 requested a review from YifeiZhuang February 23, 2023 18:48
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")?

@ejona86 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 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
@danielzhaotongliu
Copy link
Collaborator Author

@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'.

@ejona86 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 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
Copy link
Member

ejona86 commented Mar 30, 2023

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:

  1. Click "Details"
  2. Click "Invocation Details"
  3. Follow "FUSION_URL"
  4. 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":

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

@YifeiZhuang
Copy link
Member

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)

Copy link
Member

@YifeiZhuang YifeiZhuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@danielzhaotongliu
Copy link
Collaborator Author

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

@ejona86 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 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 ejona86 merged commit 5201e49 into grpc:master Apr 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants