internal/stats: Add metrics recorder list and usage in ClientConn #7428

zasweq · 2024-07-19T23:01:10Z

This PR adds a metrics recorder list component as a top level component of the ClientConn. It also forwards this to balancer by scaling up balancer.BuildOptions.

The Server Side equivalent will come when needed to pass down to xDS Client, but no need for now.

RELEASE NOTES: N/A

codecov · 2024-07-19T23:05:08Z

Codecov Report

Attention: Patch coverage is 81.74603% with 23 lines in your changes missing coverage. Please review.

Project coverage is 81.32%. Comparing base (2bcbcab) to head (619b20e).
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7428      +/-   ##
==========================================
- Coverage   81.52%   81.32%   -0.20%     
==========================================
  Files         350      354       +4     
  Lines       26879    27053     +174     
==========================================
+ Hits        21913    22002      +89     
- Misses       3772     3839      +67     
- Partials     1194     1212      +18

Files	Coverage Δ
balancer/balancer.go	`95.45% <ø> (ø)`
balancer_wrapper.go	`86.84% <100.00%> (+0.74%)`	⬆️
clientconn.go	`93.10% <100.00%> (+<0.01%)`	⬆️
experimental/stats/metricregistry.go	`94.52% <100.00%> (+0.86%)`	⬆️
internal/stats/metrics_recorder_list.go	`100.00% <100.00%> (ø)`
internal/testutils/stats/test_metrics_recorder.go	`72.61% <72.61%> (ø)`

... and 19 files with indirect coverage changes

internal/stats/metrics_recorder_list.go

internal/stats/test/metrics_recorder_list_test.go

zasweq · 2024-07-24T01:21:00Z

Thanks for the pass; got to all comments.

dfawley

Looks good overall, just some minor things..

internal/stats/metrics_recorder_list.go

dfawley · 2024-07-24T22:48:29Z

internal/stats/metrics_recorder_list.go

-		logger.Infof("length of labels passed to RecordInt64Count incorrect got: %v, want: %v", got, want)
+func verifyLabels(labels []string, optionalLabels []string, labelsRecv ...string) {
+	if got, want := len(labelsRecv), len(labels)+len(optionalLabels); got != want {
+		logger.Fatalf("Length of labels passed to Record incorrect, got: %v, want: %v.", got, want)


IMO delete the logger now and call panic(fmt.Sprintf) here.

Also if we pass the descriptor let's print the metric name with %q as well. Maybe lose the got, want formatting which I believe is unique to Go test assertions?

Got rid of got, want. Recorded the metric name with %q from desc.

Oh I didn't mean to remove the counts.. Just to turn it into a more english-y thing than a Go test error message. E.g.

Like Received %d labels in call to record metric %q, but expected %d

Ah ok noted switched to that.

internal/stats/metrics_recorder_list.go

internal/testutils/stats/test_metrics_recorder.go

dfawley · 2024-07-25T17:24:26Z

internal/stats/metrics_recorder_list.go

 	}
 }

 func (l *MetricsRecorderList) RecordInt64Count(handle *estats.Int64CountHandle, incr int64, labels ...string) {
-	verifyLabels(handle.Labels, handle.OptionalLabels, labels...)
+	verifyLabels((*estats.MetricDescriptor)(handle), labels...)


Oh..I was thinking we wouldn't need to do the cast here for some reason... I wonder if it's worth adding a method to the handles like:

func (h handle) Descriptor() *estats.MetricDescriptor { return (*estats.MetricDescriptor)(h) }

...in order to keep the ugliness out of the places we do the cast.

Yeah, these are also cast like this in OTel, we could add but then that populates exported namespace. Although if a user does use the metrics registry and writes their own stats handlers, they will need to key on a property of the descriptor, and perhaps they will use the pointer as well.

I'll keep it as a separate commit and you can see how it looks.

Sent out a separate commit: 619b20e, let me know what you think.

dfawley · 2024-07-25T17:26:17Z

internal/stats/metrics_recorder_list.go

-		logger.Infof("length of labels passed to RecordInt64Count incorrect got: %v, want: %v", got, want)
+func verifyLabels(labels []string, optionalLabels []string, labelsRecv ...string) {
+	if got, want := len(labelsRecv), len(labels)+len(optionalLabels); got != want {
+		logger.Fatalf("Length of labels passed to Record incorrect, got: %v, want: %v.", got, want)


Oh I didn't mean to remove the counts.. Just to turn it into a more english-y thing than a Go test error message. E.g.

Like Received %d labels in call to record metric %q, but expected %d

dfawley · 2024-07-25T17:27:27Z

internal/stats/metrics_recorder_list_test.go

@@ -223,7 +223,7 @@ func (s) TestMetricRecorderListPanic(t *testing.T) {
 	})
 	mrl := istats.NewMetricsRecorderList(nil)

-	want := "Length of labels passed to Record incorrect, got: 1, want: 2."
+	want := `Length of labels passed to Record incorrect for metric "simple counter".`


It's not super important to check the string, especially in its entirety, if you don't want to keep adjusting this.

Left it I like it.

…pc#7428)

zasweq requested a review from dfawley July 19, 2024 23:01

zasweq assigned dfawley Jul 19, 2024

zasweq added the Type: Feature New features or improvements in behavior label Jul 19, 2024

zasweq added this to the 1.66 Release milestone Jul 19, 2024

zasweq force-pushed the add-metrics-recorder-list-cc branch from 01f6995 to c777496 Compare July 19, 2024 23:45

Add Metrics Recorder List and usage in ClientConn

dc37567

zasweq force-pushed the add-metrics-recorder-list-cc branch from c777496 to dc37567 Compare July 19, 2024 23:54

dfawley reviewed Jul 23, 2024

View reviewed changes

internal/stats/metrics_recorder_list.go Outdated Show resolved Hide resolved

internal/stats/metrics_recorder_list.go Outdated Show resolved Hide resolved

internal/stats/test/metrics_recorder_list_test.go Outdated Show resolved Hide resolved

Responded to Doug's comments

6c69fc7

zasweq force-pushed the add-metrics-recorder-list-cc branch from 997f9a0 to 6c69fc7 Compare July 24, 2024 01:12

zasweq assigned dfawley and unassigned dfawley Jul 24, 2024

dfawley reviewed Jul 24, 2024

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Jul 24, 2024

zasweq assigned dfawley and unassigned zasweq Jul 25, 2024

zasweq force-pushed the add-metrics-recorder-list-cc branch 2 times, most recently from 52373fa to 8183987 Compare July 25, 2024 01:46

Responded to Doug's comments

e410855

zasweq force-pushed the add-metrics-recorder-list-cc branch from 8183987 to e410855 Compare July 25, 2024 01:50

zasweq assigned dfawley and unassigned dfawley Jul 25, 2024

dfawley reviewed Jul 25, 2024

View reviewed changes

dfawley approved these changes Jul 25, 2024

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Jul 25, 2024

Responded to some of Doug's comments

b9fedbf

Add metrics descriptor helper on handles

619b20e

zasweq assigned dfawley and unassigned zasweq Jul 25, 2024

zasweq merged commit 84a4ef1 into grpc:master Jul 25, 2024
13 checks passed

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

internal/stats: Add metrics recorder list and usage in ClientConn (gr…

567283f

…pc#7428)

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

internal/stats: Add metrics recorder list and usage in ClientConn (gr…

7b19fac

…pc#7428)

tbg mentioned this pull request Nov 27, 2024

DEPS: upgrade grpc to v1.68.0 cockroachdb/cockroach#136278

Closed

16 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/stats: Add metrics recorder list and usage in ClientConn #7428

internal/stats: Add metrics recorder list and usage in ClientConn #7428

zasweq commented Jul 19, 2024

codecov bot commented Jul 19, 2024 •

edited

Loading

zasweq commented Jul 24, 2024

dfawley left a comment

dfawley Jul 24, 2024

zasweq Jul 25, 2024

dfawley Jul 25, 2024

zasweq Jul 25, 2024

dfawley Jul 25, 2024

zasweq Jul 25, 2024

zasweq Jul 25, 2024

zasweq Jul 25, 2024

dfawley Jul 25, 2024

dfawley Jul 25, 2024

zasweq Jul 25, 2024

internal/stats: Add metrics recorder list and usage in ClientConn #7428

internal/stats: Add metrics recorder list and usage in ClientConn #7428

Conversation

zasweq commented Jul 19, 2024

codecov bot commented Jul 19, 2024 • edited Loading

Codecov Report

zasweq commented Jul 24, 2024

dfawley 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

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

codecov bot commented Jul 19, 2024 •

edited

Loading