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: document server-side stats event ordering and add tests #7824

Open
aranjans opened this issue Nov 11, 2024 · 6 comments
Open

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

aranjans opened this issue Nov 11, 2024 · 6 comments
Assignees
Labels
Area: Server Includes Server, Streams and Server Options. P2 Status: Help Wanted Type: Bug

Comments

@aranjans
Copy link
Contributor

aranjans commented Nov 11, 2024

Description

Currently, the ordering of server-side stats events is not explicitly documented in the stats.Handler interface, nor verified through tests. This can lead to confusion for users implementing stats handlers, as seen in [link-to-user-question-if-available].

The current server-side stats event ordering is:

  1. InHeader (when processing incoming headers)
  2. Begin (when starting RPC processing)
  3. [message events during RPC]
  4. End (when RPC completes)

Proposed Changes

  1. Update the stats.Handler interface documentation to explicitly state the ordering of server-side stats events
  2. Add tests to verify this behavior remains consistent

The current behavior is correct and working as intended, but lacks documentation and test coverage to ensure it remains consistent. This will help users better understand and rely on the stats event ordering when implementing their stats handlers.

@aranjans aranjans added the Area: Server Includes Server, Streams and Server Options. label Nov 11, 2024
@dfawley
Copy link
Member

dfawley commented Nov 11, 2024

The expected server-side stats event ordering is:

This is the current ordering.

We should also discuss options like:

  • Removing "Begin", since the start of every RPC on either side should include the initial headers
  • Omitting "InHeader"/"OutHeader" at RPC start, since the start of every RPC on either side should already include the headers
  • Reordering the events, since logically "Begin" sounds like it should happen first

I'm not wild about the current behavior. But changing things here may be difficult since the API has been around for a long time.

@RyanBlaney
Copy link

I definitely think ordering of both the documentation and the test cases is important. Although intuition may lead some to think it occurs first, simply rephrasing the documentation should be enough. It should be blatantly clear for anyone who skims the docs on hover or a Shift+k. Let me know your thoughts before I create a PR.

Proposal:

// InHeader is the first event handled in the RPC lifecycle.
// It contains stats when the header is received. 
// This event marks the start of processing for incoming RPCs 
// and must be completed before any other events occur.
type InHeader struct {
	// Client is true if this InHeader is from client side.
	Client bool
	// WireLength is the wire length of header.
	WireLength int
	// Compression is the compression algorithm used for the RPC.
	Compression string
	// Header contains the header metadata received.
	Header metadata.MD

	// The following fields are valid only if Client is false.
	// FullMethod is the full RPC method string, i.e., /package.service/method.
	FullMethod string
	// RemoteAddr is the remote address of the corresponding connection.
	RemoteAddr net.Addr
	// LocalAddr is the local address of the corresponding connection.
	LocalAddr net.Addr
}

// 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 begins
// FailFast is only valid if this Begin is from client side.
type Begin struct {
	// Client is true if this Begin is from client side.
	Client bool
	// BeginTime is the time when the RPC attempt begins.
	BeginTime time.Time
	// FailFast indicates if this RPC is failfast.
	FailFast bool
	// IsClientStream indicates whether the RPC is a client streaming RPC.
	IsClientStream bool
	// IsServerStream indicates whether the RPC is a server streaming RPC.
	IsServerStream bool
	// IsTransparentRetryAttempt indicates whether this attempt was initiated
	// due to transparently retrying a previous attempt.
	IsTransparentRetryAttempt bool
}

Test Organization

Grouping tests by RPC type (i.e., Unary, Client Streaming, Server Streaming, Full Duplex) provides better organization and a more intuitive view of the event flow for each type.
Then for each individual test, (checkInHeader, checkBegin, etc.) we can add a hook to track actions. We can make a recordAction function in order to verify the order and consistency.
Simplified Example:

func checkUnaryRPCEvents(t *testing.T, d *gotData, e *expectedData) {
    checkInHeader(t, d, e)
    checkBegin(t, d, e)
    checkInPayload(t, d, e)
    checkOutHeader(t, d, e)
    checkOutPayload(t, d, e)
    checkOutTrailer(t, d, e)
    checkEnd(t, d, e)
}

func TestServerStatsUnaryRPC(t *testing.T) {
    testServerStats(t, &testConfig{compress: ""}, &rpcConfig{success: true, callType: unaryRPC}, []func(t *testing.T, d *gotData, e *expectedData){
        checkUnaryRPCEvents,
    })
}

Personal Note

I would love to become a contributor and implement these test cases if possible. Please let me know there are additional considerations to account for.

@arjan-bal
Copy link
Contributor

@RyanBlaney thank you for taking this up. The proposed changes sound good at a high level. I've started reviewing #7885.

@dfawley
Copy link
Member

dfawley commented Dec 3, 2024

Note that the comments you've added in the above snippets only apply to the server. On the client, I believe we expect this ordering: Begin->OutHeader->{InHeader (at most once)/OutPayload/InPayload (only after InHeader)}->InTrailer.

@RyanBlaney
Copy link

Yes thank you! That clears up a lot. I encountered that sequence for the client during testing and was confused. I clarify in the documentation to address this. I'll most likely add more test cases to cover this.

@RyanBlaney
Copy link

Here's how the documentation looks in Neovim:
image

The single space indent allows for the nice format, let me know if I should remove it for consistency with other functions.

// InHeader contain stats when the header is received.
//  First event in the server side event sequence.
//  Follows last OutPayload for server side events.
// 
// Server Stats Example Event Ordering:
//  *InHeader* -> Begin -> InPayload(s) -> OutHeader -> OutPayload(s) -> OutTrailer -> End 
// 
// Client Stats Example Event Ordering:
//  Begin -> OutHeader -> OutPayload(s) -> *InHeader* -> InTrailer -> InPayload(s) -> End 

Here is Begin:
image

// Begin contains stats when an RPC attempt begins.
//  First event in the client-side event sequence.
//  Follows InHeader for server-side events.
// 
// Server Stats Example Event Ordering:
//  InHeader -> *Begin* -> InPayload(s) -> OutHeader -> OutPayload(s) -> OutTrailer -> End
// 
// Client Stats Example Event Ordering:
//  *Begin* -> OutHeader -> OutPayload(s) -> InHeader -> InTrailer -> InPayload(s) -> End
//
// FailFast is only valid if this Begin is from client side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Server Includes Server, Streams and Server Options. P2 Status: Help Wanted Type: Bug
Projects
None yet
Development

No branches or pull requests

4 participants