orca: add application utilization and range checking #6357

dfawley · 2023-06-07T17:14:02Z

Implement the new field described here, and add missing range checks and tests.

RELEASE NOTES: none

zasweq · 2023-06-07T18:30:56Z

orca/server_metrics.go

 // DeleteMemoryUtilization deletes the relevant server metric to prevent it
 // from being sent.


Nit on these docstrings and also the interface docstrings. Most of them define "the relevant server metric", but I feel like it should just be as specific as "for the memory utilization metric" as on line 211 for all docstrings because it maps 1:1 with the name.

Yeah it didn't feel like any documentation at all should be necessary, and was redundant...Changed, though.

Yeah I agree it's redundant but Go lint

zasweq · 2023-06-07T18:35:11Z

orca/server_metrics.go

 }

 // SetMemoryUtilization records a measurement for the memory utilization metric.
 func (s *serverMetricsRecorder) SetMemoryUtilization(val float64) {
+	if val < 0 || val > 1 {
+		return


Here and all other places with a range validation on this serverMetricsRecorder: do we want to plumb a logger in here and log in a non spammy way that we're ignoring an out of range metric, similar to how xDS Client gives info if nack, or say CDS encounters an error in it's serial handling of Client Conn State that returns no error, it logs the reason why the Client Conn State wasn't processed?

zasweq · 2023-06-07T18:44:26Z

orca/server_metrics.go

+	mergeMap(sm.Utilization, o.Utilization)
+	mergeMap(sm.RequestCost, o.RequestCost)
+	mergeMap(sm.NamedMetrics, o.NamedMetrics)


What happens if one of the values of this map is also -1 like below? Do we still want to overwrite the value for the key in the map? Should we ignore as below?

For maps, -1 has no special meaning. Deleting from the map is how you delete the value. -1 is unique to the fields that are scalar values.

zasweq · 2023-06-07T18:46:37Z

orca/server_metrics.go

@@ -63,21 +67,24 @@ func (sm *ServerMetrics) toLoadReportProto() *v3orcapb.OrcaLoadReport {

 // merge merges o into sm, overwriting any values present in both.


This isn't correct? You overwrite any values for keys in map unconditionally, but only overwrite sm's memory if it's set. I guess if an explicit -1 means unset 1:1 this is right. I just don't know all the invariants of this system/how this data is plumbed around.

You overwrite any values for keys in map unconditionally, but only overwrite sm's memory if it's set.

No? I range over o.<map field> and add all those key/value pairs into sm.<map field>. It's a merge, not a replacement, so the idea is to keep anything in sm that's not in o.

Oh my bad. I wrote this assuming there was a distinction on map values between -1 and !-1. I guess values refers to both map values and top level struct values.

zasweq · 2023-06-07T18:48:44Z

orca/producer_test.go

@@ -444,8 +444,8 @@ func (s) TestProducerMultipleListeners(t *testing.T) {
 	// Define a load report to send and expect the client to see.
 	loadReportWant := &v3orcapb.OrcaLoadReport{
 		CpuUtilization: 10,
-		MemUtilization: 100,
-		Utilization:    map[string]float64{"bob": 555},
+		MemUtilization: 0.1,


Now that you had to move these floats to be within 0 and 1, can you please add a test for the case it isn't within range and also set to -1 wrt overwriting behavior in merge(). This will also show what happens at this producer layer if these cases happen.

I have range checks in the new tests.

I know, but what happens at this layer? Just ignored? I want it tested at this layer too, or are you saying specific behaviors are fine to encapsulate to it's granular components units.

What happens at this layer is, it directly uses the other layer. So tests there for corner-casey details like that are sufficient. These tests are about testing the ORCA producer, not the semantics of the server metrics recorder.

zasweq · 2023-06-07T18:50:36Z

orca/server_metrics_test.go

+	smr.SetEPS(-0.6)
+	smr.SetNamedUtilization("x", -2)
+
+	// Memory and named utilizations are max of 1.


So this should be ignored.

zasweq · 2023-06-07T18:51:02Z

orca/server_metrics_test.go

+	if d := cmp.Diff(got, want); d != "" {
+		t.Fatalf("unexpected server metrics: -got +want: %v", d)
+	}
+}


After thinking about my comment on merge() wrt overwriting at each layer (maps and top level metrics), can you also add tests for that.

Added unit tests for merge.. It is already covered implicitly by the call metrics tests that use it, though.

zasweq · 2023-06-07T18:51:53Z

orca/service_test.go

@@ -60,16 +60,18 @@ func (t *testServiceImpl) UnaryCall(context.Context, *testpb.SimpleRequest) (*te
 	t.requests++
 	t.mu.Unlock()

-	t.smr.SetNamedUtilization(requestsMetricKey, float64(t.requests))
+	t.smr.SetNamedUtilization(requestsMetricKey, float64(t.requests)*0.01)


Like my comment above, what happens in this layer if these metrics aren't within range? Can you please add a test to show what behavior is going to be.

This is covered in the unit tests. It's just calls into the same object so if that works right then it must work right here, too.

This relates to discussions we've had and I've had with Eric about specific functionality scoped to the granular component for testing, and higher layers just do sanity to avoid excessive higher level tests. I'm actually running into this problem with my od change, since ParseConfig gets invoked at every layer of unit test, whether it's cds, cluster resolver, and obv. on the od ParseConfig test itself.

...Which is why I was saying to test the proto->JSON conversion as a unit test so you can get all the corner cases in there and not worry about it when testing the higher level logic.

zasweq

LGTM.

zasweq · 2023-06-07T20:09:14Z

orca/server_metrics.go

@@ -193,6 +195,9 @@ func copyMap(m map[string]float64) map[string]float64 {
 // SetCPUUtilization records a measurement for the CPU utilization metric.
 func (s *serverMetricsRecorder) SetCPUUtilization(val float64) {
 	if val < 0 {
+		if logger.V(2) {
+			logger.Infof("Ignoring CPU Utilization value out of range: %v", val)


Log valid range here and elsewhere?

Ehh, they'll figure it out :)

"[0, infy)" this is very few characters to add to logs lol

zasweq · 2023-06-07T20:11:08Z

orca/server_metrics_test.go

+
+	sm2 := &ServerMetrics{
+		CPUUtilization: -1,
+		AppUtilization: 0,


Unfortunate that MemUtilization's zero value = set valid metric which overwrites, but can't do anything about that.

Yeah the only option would be to use pointers and I don't really want to do that either. We basically control everything around this struct and the values of the fields, though, so it's pretty safe.

Yeah as long as we control it and we make sure it's good. Pointers would add a lot of complexity.

zasweq

LGTM.

orca: add application utilization and range checking

5a94a77

dfawley added the Type: Feature New features or improvements in behavior label Jun 7, 2023

dfawley added this to the 1.56 Release milestone Jun 7, 2023

dfawley requested a review from zasweq June 7, 2023 17:14

dfawley assigned zasweq Jun 7, 2023

dfawley mentioned this pull request Jun 7, 2023

weightedroundrobin: prefer application_utilization to cpu_utilization #6358

Merged

dfawley force-pushed the app_util branch from a4a3c7a to 5a94a77 Compare June 7, 2023 18:05

zasweq requested changes Jun 7, 2023

View reviewed changes

review comments

a68d00a

zasweq approved these changes Jun 7, 2023

View reviewed changes

zasweq assigned dfawley and unassigned zasweq Jun 7, 2023

dfawley merged commit 7aeea8f into grpc:master Jun 7, 2023
11 checks passed

dfawley deleted the app_util branch June 7, 2023 20:31

dfawley added a commit to dfawley/grpc-go that referenced this pull request Jun 7, 2023

orca: add application utilization and range checking (grpc#6357)

6b7c519

github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

orca: add application utilization and range checking #6357

orca: add application utilization and range checking #6357

dfawley commented Jun 7, 2023 •

edited

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq left a comment

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

zasweq Jun 7, 2023

dfawley Jun 7, 2023

zasweq Jun 7, 2023

zasweq left a comment

		// DeleteMemoryUtilization deletes the relevant server metric to prevent it
		// from being sent.

		@@ -63,21 +67,24 @@ func (sm ServerMetrics) toLoadReportProto() v3orcapb.OrcaLoadReport {

		// merge merges o into sm, overwriting any values present in both.

orca: add application utilization and range checking #6357

orca: add application utilization and range checking #6357

Conversation

dfawley commented Jun 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq left a comment

Choose a reason for hiding this comment

dfawley commented Jun 7, 2023 •

edited