grpc: update clientStreamAttempt context with transport context #7096

aranjans · 2024-04-04T17:30:35Z

RELEASE NOTES:

stats: Fix bug where peer was not set in context when calling stats handler for OutPayload, InPayload, End.

codecov · 2024-04-04T17:35:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.32%. Comparing base (92f6dd0) to head (82b47c5).
Report is 49 commits behind head on master.

❗ Current head 82b47c5 differs from pull request most recent head 776e341. Consider uploading reports for the commit 776e341 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7096      +/-   ##
==========================================
+ Coverage   81.29%   81.32%   +0.03%     
==========================================
  Files         340      345       +5     
  Lines       33890    33926      +36     
==========================================
+ Hits        27550    27590      +40     
+ Misses       5178     5176       -2     
+ Partials     1162     1160       -2

Files	Coverage Δ
stream.go	`81.49% <100.00%> (+0.10%)`	⬆️

... and 18 files with indirect coverage changes

arvindbr8 · 2024-04-05T18:32:34Z

Great work!

“the best code is no code at all..."

Are we sure this fixes everything that #6947 talks about? If so, do you mind adding the test output to this PR?

Also it's interesting that our CI didn't catch this. Let's add one as part of this change. One good place I can think of is to add this would be when tests (extras) run the examples/stats example.
@dfawley: do you have a recommendation?

arvindbr8 · 2024-04-05T18:36:53Z

Actually I would just add another go test for this. Let's not complicate this with adding it to extras

arvindbr8

For accounting: so that you can re-request a review when its ready

test/stats_test.go

arvindbr8 · 2024-04-10T21:32:47Z

test/stats_test.go

+
+	ctx := context.Background()
+	client := testgrpc.NewTestServiceClient(conn)
+	interop.DoClientStreaming(ctx, client)


Might be useful to have a subtest for each RPC type (unary, bidi, etc)

This doesnt seem to be resolved?

Zach suggested here to have single RPC type.

test/stats_test.go

arvindbr8

LGTM. I would like @zasweq to take a look at this as well.

arvindbr8 · 2024-04-16T17:15:01Z

test/stats_test.go

+	}
+	t.Cleanup(func() {
+		if err := conn.Close(); err != nil {
+			t.Error(err)


optional: This t.Error is not really in the essence of the test. I think maybe we can get rid of it.

zasweq

Looks great overall, some minor nits and replied to some comments with technical context.

test/stats_test.go

zasweq · 2024-04-22T17:17:41Z

test/stats_test.go

+	// Define expected stats callouts and whether a peer object should be populated.
+	// Note:
+	//   * Begin stats don't have peer information as the RPC begins before peer resolution.
+	//   * PickerUpdated stats don't have peer information as the picker operates without transport-level knowledge.


Please wrap comments to 80 characters. The guidance for this repo is comments wrapped to 80 characters, but we do not have a length maximum for code line characters.

test/stats_test.go

zasweq · 2024-04-22T17:21:12Z

test/stats_test.go

+	go func() {
+		errCh <- grpcServer.Serve(l)
+	}()
+	t.Cleanup(func() {


Can you switch one below too?

test/stats_test.go

zasweq

LGTM.

zasweq · 2024-04-22T20:26:34Z

test/stats_test.go

-// verifies that peer is sent for OutPayload, InPayload, End
-// stats handlers.
+// verifies that peer is sent all stats handler callouts instead
+// of begin and pickerUpdated


Capitalize and period at the end.

zasweq · 2024-04-22T20:27:04Z

test/stats_test.go

+	// Stats callouts & peer object population.
 	// Note:
-	//   * Begin stats don't have peer information as the RPC begins before peer resolution.
-	//   * PickerUpdated stats don't have peer information as the picker operates without transport-level knowledge.
+	// * Begin stats lack peer info (RPC starts pre-resolution).
+	// * PickerUpdated: no peer info (picker lacks transport details).


No you can keep the full comment, just wrap it to 80 characters. Just make sure it wraps 80 by 80.

zasweq · 2024-04-22T20:27:46Z

test/stats_test.go

+	go func() {
+		errCh <- grpcServer.Serve(l)
+	}()
+	t.Cleanup(func() {


Ping on this.

zasweq · 2024-04-23T18:32:09Z

Please figure out and fix the race condition before we merge this.

arvindbr8

Few comments from my previous pass seems to be unresolved, but marked resolved. Could you please respond to them before we can merge this?

arvindbr8 · 2024-04-23T19:10:59Z

test/stats_test.go

+
+	ctx := context.Background()
+	client := testgrpc.NewTestServiceClient(conn)
+	interop.DoClientStreaming(ctx, client)


This doesnt seem to be resolved?

arvindbr8 · 2024-04-23T19:12:09Z

test/stats_test.go

+	}
+	t.Cleanup(func() {
+		if err := conn.Close(); err != nil {
+			t.Error(err)


arvindbr8 · 2024-04-23T19:12:43Z

test/stats_test.go

+	client := testgrpc.NewTestServiceClient(cc)
+	interop.DoClientStreaming(ctx, client)
+
+	sc := getUniqueRPCStatsCount(psh.args)


I would inline the logic here, better for readability and this helper is only being used once.

Added the comment

arvindbr8

LGTM

purnesh42H

lgtm

aranjans marked this pull request as ready for review April 4, 2024 17:34

arvindbr8 added the Type: Bug label Apr 4, 2024

arvindbr8 self-requested a review April 4, 2024 18:52

arvindbr8 self-assigned this Apr 4, 2024

arvindbr8 added this to the 1.64 Release milestone Apr 5, 2024

arvindbr8 assigned aranjans and unassigned arvindbr8 Apr 5, 2024

arvindbr8 requested changes Apr 6, 2024

View reviewed changes

Update clientStreamAttempt context with transport context

c9b7f57

aranjans force-pushed the aranjans_6947 branch 2 times, most recently from 9152685 to 1463ef5 Compare April 10, 2024 17:15

ginayeh assigned arvindbr8 and unassigned aranjans Apr 10, 2024

arvindbr8 requested changes Apr 10, 2024

View reviewed changes

arvindbr8 assigned aranjans and unassigned arvindbr8 Apr 10, 2024

aranjans force-pushed the aranjans_6947 branch from 1463ef5 to 4bafa93 Compare April 16, 2024 10:48

Added test

2282dec

aranjans force-pushed the aranjans_6947 branch from 4bafa93 to 2282dec Compare April 16, 2024 10:52

aranjans removed their assignment Apr 16, 2024

aranjans requested a review from arvindbr8 April 16, 2024 10:58

aranjans assigned arvindbr8 Apr 16, 2024

arvindbr8 approved these changes Apr 17, 2024

View reviewed changes

arvindbr8 requested a review from zasweq April 17, 2024 00:32

arvindbr8 assigned zasweq and unassigned arvindbr8 Apr 17, 2024

dfawley changed the title ~~Update clientStreamAttempt context with transport context~~ grpc: update clientStreamAttempt context with transport context Apr 17, 2024

aranjans removed their assignment Apr 19, 2024

aranjans requested a review from zasweq April 19, 2024 06:50

zasweq assigned aranjans and unassigned zasweq Apr 19, 2024

final fix

1500079

aranjans force-pushed the aranjans_6947 branch from fde22f9 to 1500079 Compare April 20, 2024 03:32

aranjans assigned zasweq and unassigned aranjans Apr 20, 2024

zasweq requested changes Apr 22, 2024

View reviewed changes

zasweq assigned aranjans and unassigned zasweq Apr 22, 2024

zasweq approved these changes Apr 22, 2024

View reviewed changes

aranjans force-pushed the aranjans_6947 branch from a2392f8 to fa41c6e Compare April 23, 2024 17:00

aranjans assigned zasweq and unassigned aranjans Apr 23, 2024

aranjans force-pushed the aranjans_6947 branch 3 times, most recently from 72a3c5f to deb839a Compare April 23, 2024 17:43

zasweq assigned aranjans and unassigned zasweq Apr 23, 2024

arvindbr8 requested changes Apr 23, 2024

View reviewed changes

aranjans force-pushed the aranjans_6947 branch 3 times, most recently from 9decab5 to 4cdfa42 Compare April 25, 2024 05:50

arvindbr8 approved these changes Apr 25, 2024

View reviewed changes

final fix

776e341

aranjans force-pushed the aranjans_6947 branch from 4cdfa42 to 776e341 Compare April 25, 2024 17:14

purnesh42H approved these changes Apr 30, 2024

View reviewed changes

arvindbr8 merged commit 796c615 into grpc:master May 2, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc: update clientStreamAttempt context with transport context #7096

grpc: update clientStreamAttempt context with transport context #7096

aranjans commented Apr 4, 2024 •

edited

codecov bot commented Apr 4, 2024 •

edited

arvindbr8 commented Apr 5, 2024

arvindbr8 commented Apr 5, 2024

arvindbr8 left a comment

arvindbr8 Apr 10, 2024

arvindbr8 Apr 23, 2024

aranjans Apr 24, 2024

arvindbr8 left a comment

arvindbr8 Apr 16, 2024

arvindbr8 Apr 23, 2024

aranjans Apr 24, 2024

zasweq left a comment

zasweq Apr 22, 2024

aranjans Apr 22, 2024

zasweq Apr 22, 2024

zasweq left a comment

zasweq Apr 22, 2024

aranjans Apr 24, 2024

zasweq Apr 22, 2024

aranjans Apr 24, 2024

zasweq Apr 22, 2024

zasweq commented Apr 23, 2024

arvindbr8 left a comment

arvindbr8 Apr 23, 2024

arvindbr8 Apr 23, 2024

arvindbr8 Apr 23, 2024

aranjans Apr 25, 2024

arvindbr8 left a comment

purnesh42H left a comment

grpc: update clientStreamAttempt context with transport context #7096

grpc: update clientStreamAttempt context with transport context #7096

Conversation

aranjans commented Apr 4, 2024 • edited

codecov bot commented Apr 4, 2024 • edited

Codecov Report

arvindbr8 commented Apr 5, 2024

arvindbr8 commented Apr 5, 2024

arvindbr8 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

arvindbr8 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

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

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

zasweq commented Apr 23, 2024

arvindbr8 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

arvindbr8 left a comment

Choose a reason for hiding this comment

purnesh42H left a comment

Choose a reason for hiding this comment

aranjans commented Apr 4, 2024 •

edited

codecov bot commented Apr 4, 2024 •

edited