stats: Improved sequencing documentation for server-side stats events and added tests. #7885

RyanBlaney · 2024-12-02T03:06:36Z

Response to Issue #7824

RELEASE NOTES:

Documentation Clarification: Clarified the expected sequencing of stats events in the server-side stats.Handler.
New Test Cases: Added tests to verify the event sequencing for various RPC types, ensuring consistency and accuracy in server-side stats handling.

…s to verify the order of server-side events.

codecov · 2024-12-02T03:11:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.18%. Comparing base (4c07bca) to head (fb39711).
Report is 36 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7885      +/-   ##
==========================================
+ Coverage   81.84%   82.18%   +0.33%     
==========================================
  Files         377      381       +4     
  Lines       38120    38539     +419     
==========================================
+ Hits        31201    31672     +471     
+ Misses       5603     5565      -38     
+ Partials     1316     1302      -14

Files with missing lines	Coverage Δ
stats/stats.go	`68.42% <ø> (ø)`

... and 56 files with indirect coverage changes

arjan-bal · 2024-12-02T09:34:22Z

stats/stats_test.go

+func (s *testServer) UnaryCall(
+	ctx context.Context,
+	in *testpb.SimpleRequest,
+) (*testpb.SimpleResponse, error) {


grpc-go follows the Google style guide. Based on this section in the style guide:

The signature of a function or method declaration should remain on a single line to avoid indentation confusion.

Please revert the changes made to wrap function definationations and calls throughout the PR.

Got it! I'll disable my autoformatter going forward or reconfigure it to follow the Google style guide.

arjan-bal · 2024-12-02T09:38:15Z

stats/stats_test.go

@@ -786,8 +884,14 @@ func checkConnEnd(t *testing.T, d *gotData) {
 	st.IsClient() // TODO remove this.
 }

+type event struct {
+	eventType string
+	timestamp time.Time


What is the use of storing a timestamp? I believe we're only interested in the ordering of the events and not the absolute times they were emitted.

The timestamp was included for debugging purposes in case of test failures, since it could help identify delays or other issues. If you feel this isn’t valuable, I can to remove it for simplicity.

RyanBlaney · 2024-12-02T21:55:00Z

I have fixed the wrapping errors for formatting and I removed the timestamps from the event structs. Thank you for your feedback @arjan-bal, is there anything else you need me to do?

…. Added a server stats Server Stream RPC test.

purnesh42H · 2024-12-06T07:05:31Z

stats/stats.go

@@ -36,7 +36,49 @@ type RPCStats interface {
 	IsClient() bool
 }

+// InHeader contain stats when the header is received.
+//  First event in the server side event sequence.


nix the extra space. Applicable everywhere. If you want to move the sentence to new line, you need to put a blank line in between. If you want bullets, you can use markdown style "-"

purnesh42H · 2024-12-06T07:08:59Z

stats/stats.go

@@ -98,31 +140,6 @@ func (s *InPayload) IsClient() bool { return s.Client }

 func (s *InPayload) isRPCStats() {}

-// InHeader contains stats when a header is received.
-type InHeader struct {


@RyanBlaney we should just edit the documentation in current struct instead of deleting it entirely and re-creating. Also, restrict the documentation to struct instead of writing the whole sequence. Mentioning that it is the first event on server side after rpc is received is fine.

I'll get right to it! Thank you!

purnesh42H · 2024-12-06T07:09:47Z

stats/stats_test.go

@@ -136,7 +197,14 @@ func (s *testServer) UnaryCall(ctx context.Context, in *testpb.SimpleRequest) (*

 func (s *testServer) FullDuplexCall(stream testgrpc.TestService_FullDuplexCallServer) error {
 	if err := stream.SendHeader(testHeaderMetadata); err != nil {
-		return status.Errorf(status.Code(err), "%v.SendHeader(%v) = %v, want %v", stream, testHeaderMetadata, err, nil)
+		return status.Errorf(


golang doesn't restrict columns for code. Having it in one is fine.

purnesh42H · 2024-12-06T07:10:20Z

stats/stats_test.go

@@ -81,6 +83,55 @@ var (
 	}
 	// The id for which the service handler should return error.
 	errorID int32 = 32202
+
+    // Server Stats


Please fix the documentation everywhere. Remove extra spaces at start and end etc.

Everything should be resolved now. Thank you for your help!

…der of the InHeader and Begin structs to their original place in the stats.go file.

…opts.

RyanBlaney · 2024-12-11T22:32:40Z

I added a note for the StartServer function in the stats_test.go. It uses a few deprecated opts, so if you want me to resolve these issues I can do that. Also there is a TODO that got left for testing the OutHeader. If you need me to test that I can do that as well.

purnesh42H · 2024-12-16T04:37:11Z

stats/stats.go

@@ -37,6 +37,9 @@ type RPCStats interface {
 }

 // Begin contains stats when an RPC attempt begins.
+// This event is called AFTER the InHeader event, as headers must


why is AFTER caps?

Also, you don't have to begin from next line unless you reaching the max columns 80 or if you want to have a new line then you have to put a blank line in between two sentences.

nit: InHeader

Got it! Cleaning this up in the next commit.

purnesh42H · 2024-12-16T04:39:28Z

stats/stats.go

+// InHeader contain stats when the header is received.
+// 
+// First event in the server side event sequence.
+// Follows last OutPayload for server side events.


Same thing. Can start in same line. It is the first event at the server after receiving the RPC. It is followed by OutPayload server event.

purnesh42H · 2024-12-16T04:39:42Z

stats/stats.go

@@ -123,6 +129,7 @@ func (s *InHeader) IsClient() bool { return s.Client }

 func (s *InHeader) isRPCStats() {}

+


nix new line

What does nix mean in this context? I'm assuming you mean delete. I don't see an extra space between functions on my end so I'm confused what you're asking here.

purnesh42H · 2024-12-16T04:42:29Z

stats/stats_test.go

@@ -59,8 +59,10 @@ func init() {
 	grpc.EnableTracing = false
 }

-type connCtxKey struct{}
-type rpcCtxKey struct{}
+type (


We don't declare types like that anywhere in our codebase so let's revert this as its not relevant to the change?

My bad. Remnant of an gofmt mistake.

purnesh42H · 2024-12-16T04:45:08Z

stats/stats_test.go

@@ -81,6 +83,52 @@ var (
 	}
 	// The id for which the service handler should return error.
 	errorID int32 = 32202
+    // Server Stats
+	// Ensure that Unary RPC server stats events are logged in the correct order.


variable name is self explanatory. If at all we want to put comment here, something like this is more suitable, To verify if the Unary RPC server stats events are logged in the correct order.

purnesh42H · 2024-12-16T04:46:08Z

stats/stats_test.go

@@ -81,6 +83,52 @@ var (
 	}
 	// The id for which the service handler should return error.
 	errorID int32 = 32202
+    // Server Stats


why do we need this extra comment ServerStats? Probably just remove since you have a sentence already for each. Applicable everywhere

purnesh42H · 2024-12-16T04:47:03Z

stats/stats_test.go

@@ -326,7 +376,11 @@ func (te *test) doUnaryCall(c *rpcConfig) (*testpb.SimpleRequest, *testpb.Simple

 	tCtx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer cancel()
-	resp, err = tc.UnaryCall(metadata.NewOutgoingContext(tCtx, testMetadata), req, grpc.WaitForReady(!c.failfast))


golang doesn't restrict columns for code so let's avoid the changes that are not relevant to the issue. Applicable everywhere.

Remnant of an autoformatting mistake. Correcting now.

purnesh42H · 2024-12-16T05:03:42Z

@RyanBlaney could you please revert the refactor/cleanup changes that are unrelated to the issue? Its hard to review what is changing.

RyanBlaney · 2024-12-18T00:20:58Z

I fixed the autoformatting mistakes and cleaned up the comments. I will not use an autoformatter going forward, since it caused me a headache. Is there anything else needed in terms of testing?

purnesh42H · 2024-12-21T20:04:41Z

I fixed the autoformatting mistakes and cleaned up the comments. I will not use an autoformatter going forward, since it caused me a headache. Is there anything else needed in terms of testing?

Thanks, I will take a look. Looks like you have some vet check failures. Could you run the vet check locally and fix https://github.com/grpc/grpc-go/blob/master/CONTRIBUTING.md ?

RyanBlaney · 2024-12-21T21:57:14Z

The vet errors should be fixed now. Happy holidays!

RyanBlaney · 2024-12-21T22:41:11Z

I'm still failing one test for some reason. I didn't see that locally. I'll look into it when I get the chance.

purnesh42H · 2024-12-26T20:33:20Z

stats/stats.go

@@ -36,7 +36,10 @@ type RPCStats interface {
 	IsClient() bool
 }

-// Begin contains stats when an RPC attempt begins.
+// Begin contains stats when an RPC attempt begins. This event is called after
+// the InHeader event, as headers must be processed before the RPC lifecycle


nit InHeader

What specifically is the issue here? Is it related to style?

purnesh42H · 2024-12-26T20:34:00Z

stats/stats.go

@@ -98,7 +101,9 @@ func (s *InPayload) IsClient() bool { return s.Client }

 func (s *InPayload) isRPCStats() {}

-// InHeader contains stats when a header is received.
+// InHeader contain stats when the header is received. It is the first event in
+// the server after receiving the RPC. It is followed by the OutPayload


OutPayload

Can you clarify what you are asking for here?

purnesh42H · 2024-12-26T20:36:37Z

stats/stats_test.go

@@ -81,6 +81,50 @@ var (
 	}
 	// The id for which the service handler should return error.
 	errorID int32 = 32202
+	// To verify if the Unary RPC server stats events are logged in the
+	// correct order.
+	expectedUnarySequence = []string{


this can be initialized within the test itself because we are not using it anywhere else. Also, convention is to use want/wanted instead of expected

Got it! I will keep this in mind going forward. Thank you!

purnesh42H · 2024-12-26T20:37:13Z

stats/stats_test.go

+	}
+	// To verify if the Client Stream RPC server stats events are logged in the
+	// correct order.
+	expectedClientStreamSequence = []string{


same. can be moved to the respective test. Also, wantClientStreamSequence

purnesh42H · 2024-12-26T20:37:55Z

stats/stats_test.go

+	}
+	// To verify if the Server Stream RPC server stats events are logged in the
+	// correct order.
+	expectedServerStreamSequence = []string{


same. can be moved to respective test. s/expectedServerStreamSequence/wantServerStreamSequence

purnesh42H · 2024-12-26T20:39:38Z

stats/stats_test.go

+
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	verifyEventSequence(t, h.events, expectedServerStreamSequence)


can't we use cmp.Equal? Here we do care about order being same as well.

I replaced with cmp.Equal although I used it in the helper function for reusability.

purnesh42H · 2024-12-26T20:39:54Z

stats/stats_test.go

+
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	verifyEventSequence(t, h.events, expectedClientStreamSequence)


same. Prefer cmp.Equal

purnesh42H · 2024-12-26T20:40:05Z

stats/stats_test.go

+	// Verify sequence
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	verifyEventSequence(t, h.events, expectedUnarySequence)


same. cmp.Equal

…nally used cmp.Equal rather than iterating.

Improved sequencing Documentation for the stats events and added test…

44abfd0

…s to verify the order of server-side events.

RyanBlaney changed the title ~~Documentation, Test: Improved sequencing documentation for server-side stats events and added tests.~~ *: Improved sequencing documentation for server-side stats events and added tests. Dec 2, 2024

RyanBlaney marked this pull request as ready for review December 2, 2024 03:13

arjan-bal changed the title *: Improved sequencing documentation for server-side stats events and added tests. stats: Improved sequencing documentation for server-side stats events and added tests. Dec 2, 2024

arjan-bal self-requested a review December 2, 2024 09:25

arjan-bal self-assigned this Dec 2, 2024

arjan-bal added the Type: Documentation Documentation or examples label Dec 2, 2024

arjan-bal added this to the 1.69 Release milestone Dec 2, 2024

arjan-bal requested changes Dec 2, 2024

View reviewed changes

arjan-bal assigned RyanBlaney and unassigned arjan-bal Dec 2, 2024

arjan-bal mentioned this pull request Dec 2, 2024

stats: document server-side stats event ordering and add tests #7824

Open

RyanBlaney added 2 commits December 2, 2024 13:47

Fix formatting issues according to Google style guide

10f6726

Removed unnecessary time stamps from event tracing

92ec341

Cleared up difference between client side and server side stat events…

71adf5e

…. Added a server stats Server Stream RPC test.

arjan-bal assigned arjan-bal and unassigned RyanBlaney Dec 3, 2024

purnesh42H assigned purnesh42H and unassigned arjan-bal Dec 3, 2024

arjan-bal requested review from purnesh42H and arjan-bal and removed request for arjan-bal December 3, 2024 08:01

purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024

purnesh42H reviewed Dec 6, 2024

View reviewed changes

purnesh42H assigned RyanBlaney and unassigned purnesh42H Dec 6, 2024

purnesh42H added the Status: Requires Reporter Clarification label Dec 6, 2024

RyanBlaney added 2 commits December 11, 2024 14:23

Fixed documentation style and whitespace between comments. Put the or…

8622eab

…der of the InHeader and Begin structs to their original place in the stats.go file.

Fixed return wrapping. Added note about StartServer using deprecated …

f291e58

…opts.

arjan-bal assigned purnesh42H and unassigned RyanBlaney Dec 12, 2024

arjan-bal removed the Status: Requires Reporter Clarification label Dec 12, 2024

purnesh42H reviewed Dec 16, 2024

View reviewed changes

purnesh42H assigned RyanBlaney and unassigned purnesh42H Dec 16, 2024

RyanBlaney added 4 commits December 17, 2024 15:45

Fixed documentation in stats.go with unnecessary line breaks.

dcb64c1

Removed unnecessary comments and fixed format wrapping mistakes.

c157b93

Fixed remaining wrapping mistakes

480ce6b

Fixed a few style issues

8b90cae

Fixed the last of the style issues

443d960

purnesh42H assigned purnesh42H and unassigned RyanBlaney Dec 21, 2024

RyanBlaney added 2 commits December 21, 2024 13:44

Fixed vet errors.

436bc06

Ran gofmt -s -w

19b4974

purnesh42H reviewed Dec 26, 2024

View reviewed changes

purnesh42H assigned RyanBlaney and unassigned purnesh42H Dec 26, 2024

RyanBlaney added 4 commits December 27, 2024 20:08

Put wanted test sequences in their respective test functions. Additio…

53b7249

…nally used cmp.Equal rather than iterating.

minor style adjustment

6a8c733

Got rid of vet error

4a18e7e

Ran gofmt and goimports

fb39711

		@@ -123,6 +129,7 @@ func (s *InHeader) IsClient() bool { return s.Client }

		func (s *InHeader) isRPCStats() {}

stats: Improved sequencing documentation for server-side stats events and added tests. #7885

Are you sure you want to change the base?

stats: Improved sequencing documentation for server-side stats events and added tests. #7885

Conversation

RyanBlaney commented Dec 2, 2024

Response to Issue #7824

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

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

RyanBlaney commented Dec 2, 2024

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

RyanBlaney commented Dec 11, 2024

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

RyanBlaney Dec 17, 2024 • edited Loading

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

RyanBlaney Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

purnesh42H commented Dec 16, 2024

RyanBlaney commented Dec 18, 2024

purnesh42H commented Dec 21, 2024

RyanBlaney commented Dec 21, 2024

RyanBlaney commented Dec 21, 2024

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 Dec 2, 2024 •

edited

Loading

RyanBlaney Dec 17, 2024 •

edited

Loading

RyanBlaney Dec 17, 2024 •

edited

Loading