Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

RyanBlaney
Copy link

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.
@RyanBlaney 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
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.08%. Comparing base (4c07bca) to head (19b4974).
Report is 29 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7885      +/-   ##
==========================================
+ Coverage   81.84%   82.08%   +0.23%     
==========================================
  Files         377      379       +2     
  Lines       38120    38261     +141     
==========================================
+ Hits        31201    31405     +204     
+ Misses       5603     5551      -52     
+ Partials     1316     1305      -11     
Files with missing lines Coverage Δ
stats/stats.go 68.42% <ø> (ø)

... and 44 files with indirect coverage changes

@RyanBlaney RyanBlaney marked this pull request as ready for review December 2, 2024 03:13
@arjan-bal 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 arjan-bal self-requested a review December 2, 2024 09:25
@arjan-bal arjan-bal self-assigned this Dec 2, 2024
@arjan-bal arjan-bal added the Type: Documentation Documentation or examples label Dec 2, 2024
@arjan-bal arjan-bal added this to the 1.69 Release milestone Dec 2, 2024
Comment on lines 153 to 156
func (s *testServer) UnaryCall(
ctx context.Context,
in *testpb.SimpleRequest,
) (*testpb.SimpleResponse, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

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

type event struct {
eventType string
timestamp time.Time
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

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.
@arjan-bal arjan-bal assigned arjan-bal and unassigned RyanBlaney Dec 3, 2024
@purnesh42H purnesh42H assigned purnesh42H and unassigned arjan-bal Dec 3, 2024
@arjan-bal arjan-bal requested review from purnesh42H and arjan-bal and removed request for arjan-bal December 3, 2024 08:01
@purnesh42H purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024
stats/stats.go Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Got it!

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I'll get right to it! Thank you!

@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

// Server Stats
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

@RyanBlaney
Copy link
Author

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.

stats/stats.go Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: InHeader

Copy link
Author

Choose a reason for hiding this comment

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

Got it! Cleaning this up in the next commit.

stats/stats.go Outdated
// InHeader contain stats when the header is received.
//
// First event in the server side event sequence.
// Follows last OutPayload for server side events.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

func (s *InHeader) isRPCStats() {}


Copy link
Contributor

Choose a reason for hiding this comment

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

nix new line

Copy link
Author

@RyanBlaney RyanBlaney Dec 17, 2024

Choose a reason for hiding this comment

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

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.

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

type connCtxKey struct{}
type rpcCtxKey struct{}
type (
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

My bad. Remnant of an gofmt mistake.

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

@@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

@RyanBlaney RyanBlaney Dec 17, 2024

Choose a reason for hiding this comment

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

Remnant of an autoformatting mistake. Correcting now.

@purnesh42H
Copy link
Contributor

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

@purnesh42H purnesh42H assigned RyanBlaney and unassigned purnesh42H Dec 16, 2024
@RyanBlaney
Copy link
Author

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 purnesh42H assigned purnesh42H and unassigned RyanBlaney Dec 21, 2024
@purnesh42H
Copy link
Contributor

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

The vet errors should be fixed now. Happy holidays!

@RyanBlaney
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Documentation or examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants