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.

Sign up for GitHub

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

Jump to bottom

stats: peer is not populated for all HandleRPC client callbacks #6947

Closed
pellared opened this issue Jan 29, 2024 · 7 comments · Fixed by #7096
Closed

stats: peer is not populated for all HandleRPC client callbacks #6947

pellared opened this issue Jan 29, 2024 · 7 comments · Fixed by #7096
Assignees
@aranjans
Labels
Type: Bug

Comments

@pellared
Copy link
Contributor

pellared commented Jan 29, 2024 •
edited

What version of gRPC are you using?

v1.61.0

What version of Go are you using (go version)?

go version go1.21.4 windows/amd64

What operating system (Linux, Windows, …) and version?

go version go1.21.4 windows/amd64 (was not working on Linux as well)

What did you do?

package main_test

import (
	"context"
	"net"
	"testing"

	"google.golang.org/grpc"
	"google.golang.org/grpc/credentials/insecure"
	"google.golang.org/grpc/interop"
	"google.golang.org/grpc/interop/grpc_testing"
	"google.golang.org/grpc/peer"
	"google.golang.org/grpc/stats"
)

func TestPeerForClientStatsHandler(t *testing.T) {
	spy := &handlerSpy{}

	// Start server.
	l, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		t.Fatal(err)
	}
	grpcServer := grpc.NewServer()
	grpc_testing.RegisterTestServiceServer(grpcServer, interop.NewTestServer())
	errCh := make(chan error)
	go func() {
		errCh <- grpcServer.Serve(l)
	}()
	t.Cleanup(func() {
		grpcServer.Stop()
		if err := <-errCh; err != nil {
			t.Error(err)
		}
	})

	// Create client with stats handler and do some calls.
	conn, err := grpc.Dial(
		l.Addr().String(),
		grpc.WithBlock(),
		grpc.WithTransportCredentials(insecure.NewCredentials()),
		grpc.WithStatsHandler(spy),
	)
	if err != nil {
		t.Fatal(err)
	}
	t.Cleanup(func() {
		if err := conn.Close(); err != nil {
			t.Error(err)
		}
	})

	client := grpc_testing.NewTestServiceClient(conn)
	interop.DoEmptyUnaryCall(client)
	interop.DoLargeUnaryCall(client)
	interop.DoClientStreaming(client)
	interop.DoServerStreaming(client)
	interop.DoPingPong(client)

	// Assert if peer is populated for each stats type.
	for _, callbackArgs := range spy.Args {
		if callbackArgs.Peer == nil {
			t.Errorf("peer not populated for: %T", callbackArgs.RPCStats)
		} else {
			t.Logf("passed for: %T", callbackArgs.RPCStats)
		}
	}
}

type peerStats struct {
	RPCStats stats.RPCStats
	Peer     *peer.Peer
}

type handlerSpy struct {
	Args []peerStats
}

func (h *handlerSpy) TagRPC(ctx context.Context, info *stats.RPCTagInfo) context.Context {
	return ctx
}

func (h *handlerSpy) HandleRPC(ctx context.Context, rs stats.RPCStats) {
	p, _ := peer.FromContext(ctx)
	h.Args = append(h.Args, peerStats{rs, p})
}

func (h *handlerSpy) TagConn(ctx context.Context, info *stats.ConnTagInfo) context.Context {
	return ctx
}

func (h *handlerSpy) HandleConn(context.Context, stats.ConnStats) {
}

What did you expect to see?

According to #6897 (comment):

It works in every HandleRPC call, since it is populated before.

Therefore, the test should pass.

What did you see instead?

$ go test
--- FAIL: TestPeerForClientStatsHandler (0.01s)
    grpc_test.go:63: peer not populated for: *stats.Begin
    grpc_test.go:65: passed for: *stats.OutHeader
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:65: passed for: *stats.InHeader
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:65: passed for: *stats.InTrailer
    grpc_test.go:63: peer not populated for: *stats.End
    grpc_test.go:63: peer not populated for: *stats.Begin
    grpc_test.go:65: passed for: *stats.OutHeader
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:65: passed for: *stats.InHeader
    grpc_test.go:65: passed for: *stats.InTrailer
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:63: peer not populated for: *stats.End
    grpc_test.go:63: peer not populated for: *stats.Begin
    grpc_test.go:65: passed for: *stats.OutHeader
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:65: passed for: *stats.InHeader
    grpc_test.go:65: passed for: *stats.InTrailer
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:63: peer not populated for: *stats.End
    grpc_test.go:63: peer not populated for: *stats.Begin
    grpc_test.go:65: passed for: *stats.OutHeader
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:65: passed for: *stats.InHeader
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:65: passed for: *stats.InTrailer
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:63: peer not populated for: *stats.End
    grpc_test.go:63: peer not populated for: *stats.Begin
    grpc_test.go:65: passed for: *stats.OutHeader
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:65: passed for: *stats.InHeader
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:63: peer not populated for: *stats.OutPayload
    grpc_test.go:63: peer not populated for: *stats.InPayload
    grpc_test.go:65: passed for: *stats.InTrailer
    grpc_test.go:63: peer not populated for: *stats.End
FAIL
exit status 1
@pellared pellared mentioned this issue Jan 29, 2024
Closed
@zasweq
Copy link
Contributor

zasweq commented Feb 8, 2024

grpc-go/internal/transport/http2_client.go

Line 356 in d41b01d

t.ctx = peer.NewContext(t.ctx, t.getPeer())
this populates the peer at transport creation time. All the stats handler callouts happen derived from this context. I ran a local test and every stats handler callout had the peer.

@zasweq zasweq closed this as completed Feb 8, 2024
@pellared
Copy link
Contributor Author

pellared commented Feb 8, 2024 •
edited

@zasweq Do you mean that the test passed for you on main?

@pellared
Copy link
Contributor Author

pellared commented Feb 8, 2024 •
edited

@zasweq I have the same problem on main. I just had to add context.Background() to interop. calls.

EDIT: I created #6971

pellared added a commit to pellared/grpc-go that referenced this issue Feb 8, 2024
@pellared
Reproduce grpc#6947
1094be8
@pellared pellared mentioned this issue Feb 8, 2024
Closed
@dfawley dfawley reopened this Feb 14, 2024
@arvindbr8
Copy link
Member

@aranjans -- Ping; This might something to look at after issue you are currently looking at.

@aranjans
Copy link
Collaborator

aranjans commented Feb 29, 2024 via email

@aranjans
Copy link
Collaborator

@pellared We are able to reproduce the issue and would be working on the fix. We would be able to fix all the InPayload and OutPayload, however there is no peer available at Begin and End might not have the peer populated in the case where the RPC was ended even before we got a peer. So we should be able to get End wherever possible.

@pellared
Copy link
Contributor Author

Makes sense

@aranjans aranjans mentioned this issue Apr 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aranjans aranjans

Labels
Type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

grpc: update clientStreamAttempt context with transport context aranjans/grpc-go
5 participants
@pellared @arvindbr8 @dfawley @zasweq @aranjans