-
Notifications
You must be signed in to change notification settings - Fork 1.7k
improved grpc implementation + documentation update #2110
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
base: development
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayusjayaswal , Hey can you fix the code quality issues. I will test the PR after that and we will be good to go.
hi! @coolwednesday thanks for the help; the mistakes you pointed out are now corrected :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayusjayaswal, Here’s the screenshot. I noticed the logs are a bit inconsistent.
- Please review and update the logs for streaming client & server as well as unary client & server examples in the GoFr examples folder.
- Also, add the metrics and traces and logs screenshots.
- Once that’s done, we can proceed to merge this PR.
- While updating, make sure to check for any log inconsistencies and fix them.
- Also fix the code quality issues.
Hi @coolwednesday following changes are done
following are some screenshots |
0f6d76d
to
632d080
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey I checked your tests are not passing. Please check that. Also Please also check sending requests through postman if you are able to see their correct traces, logs etc ... based on the changes that you did. Add screenshots of the same.
Thanks ! Post these , we will be good to go to fix these.
@ayusjayaswal ? Any updates on this ? |
@coolwednesday hi I'm working on fixing the registerMetrics function in grpc.go, it is causing problems for the testing. Once this is done all tests should pass properly. |
Sure ! Let me know if you are stuck. You can share it here ! |
59a0bd1
to
3ec30dc
Compare
Hi, I have made most tests pass properly, lints should pass too. But, I couldn't get past this issue (see screenshot 2). @coolwednesday , I'll need some help with the TestGRPC_ServerRun function. ![]() ![]() |
No worries. Will pick this up as soon as I can ...and get this PR done. Thanks for the work you have put Ayush. |
Thanks! @coolwednesday , I’d prefer to complete this PR, whichever feels appropriate to you. |
Go ahead ! Let me know the blocker than. The test I will check and try to fix it tomorrow. Try debugging it till then. |
Hi @coolwednesday following Test function is creating problems func TestGRPC_ServerRun(t *testing.T) {
testCases := []struct {
desc string
port int
expLog string
}{
{"net.Listen() error", 99999, "error in starting gRPC server"}, // Invalid port
{"server.Serve() error", 10000, "error in starting gRPC server"}, // Port occupied
}
for i, tc := range testCases {
f := func() {
c, mocks := container.NewMockContainer(t)
setupGRPCMetricExpectations(mocks.Metrics)
// Add expectations for error scenarios
mocks.Metrics.EXPECT().IncrementCounter(gomock.Any(), "grpc_server_errors_total").AnyTimes()
mocks.Metrics.EXPECT().SetGauge("grpc_server_status", gomock.Any()).AnyTimes()
// If testing "server.Serve() error", occupy the port first
if tc.port == 10000 {
listener, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", fmt.Sprintf(":%d", tc.port))
if err != nil {
t.Fatalf("Failed to occupy port %d: %v", tc.port, err)
}
defer listener.Close() // Ensure cleanup
}
g := &grpcServer{
port: tc.port,
config: getConfigs(t),
}
go func() {
g.Run(c)
}()
// Give some time for the server to attempt startup
time.Sleep(500 * time.Millisecond)
_ = g.Shutdown(t.Context()) // Ensure shutdown
}
out := testutil.StderrOutputForFunc(f)
assert.Contains(t, out, tc.expLog, "TEST[%d], Failed.\n", i)
}
} The invalid port on line 7 works fine but on line 8 port 10000 is not nice, I made it to port 1 and the test passed but 10000 keeps failing silently. This issue started after I added mocks for this test so metrics are available. I'm aware following tests might need some work too but I keep failing to get past this. |
you can use NewServerConfigs function that returns ports that are free.
So the issue is since fatal is called the process immediately exits ... there is no scope of channel reading. Check how are we testing fatal cases across the codebase and use that. If I am able to fix this test, I will push the changes. You can refer to them for understanding. |
@ayusjayaswal, Fixed the tests. Go through it. I will leave the linter's fix to you. |
@coolwednesday Thank you very much for the help, definitely needed that. I have completed the linting. This PR should be ready to merge now : ) |
Contributor: Ayush Jayaswal
Track: Core Framework
@coolwednesday please review this PR.
This Patch does the following:
pkg/gofr/grpc.go
:pkg/gofr/gprc/log.go
:closes #2027