-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add binary logger option for client and server #5675
Conversation
ad8e520
to
074c7da
Compare
test/end2end_test.go
Outdated
rpcDone := make(chan struct{}) | ||
go func() { | ||
for { | ||
_, err := stream.Recv() | ||
if err == io.EOF { | ||
close(rpcDone) | ||
return | ||
} | ||
} | ||
}() | ||
stream.CloseSend() | ||
<-rpcDone |
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.
Does this not work?
stream.CloseSend()
var err error
for err != io.EOF {
_, err = stream.Recv()
}
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.
Switched. You don't even need the for err != io.EOF {}, I just added if _, err = stream.Recv(); err != io.EOF { t.Fatal(...) }
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.
CloseSend
shouldn't be returning any error here anyway, right? Not sure what I was thinking.
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.
Well it'll return an EOF (through ending of the stream with no error in the streams lifetime as you explained to me Tues).
if csbl.mml.events != 9 { | ||
t.Fatalf("want 9 client side binary logging events, got %v", csbl.mml.events) | ||
} | ||
if ssbl.mml.events != 8 { | ||
t.Fatalf("want 8 server side binary logging events, got %v", ssbl.mml.events) | ||
} |
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.
Why aren't these the same?
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.
This is WAI. I looked over my diff (and found some correctness issues - which you can see in the diff of the commit I'm about to push out). However, my diff from master really keeps the exact same number of logging calls in each operation/over the stream lifetime.
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.
Can you explain the difference, though? It doesn't block this PR, but we need to understand this and make sure it's correct.
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.
Yeah, I'm testing this exact scenario in o11y e2e, and writing out the exact atoms of events emitted that we expect like we discussed, hopefully if there's any glaring correctness issues it'll show up then, or we can see it's WAI, and we can look into it further.
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.
We can also print them here really easily, right? Why not just add a couple t.Log
s and let's see what it is.
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.
Thanks for the comments boss man :D! Hope your ankle isn't too messed up.
test/end2end_test.go
Outdated
rpcDone := make(chan struct{}) | ||
go func() { | ||
for { | ||
_, err := stream.Recv() | ||
if err == io.EOF { | ||
close(rpcDone) | ||
return | ||
} | ||
} | ||
}() | ||
stream.CloseSend() | ||
<-rpcDone |
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.
Switched. You don't even need the for err != io.EOF {}, I just added if _, err = stream.Recv(); err != io.EOF { t.Fatal(...) }
if csbl.mml.events != 9 { | ||
t.Fatalf("want 9 client side binary logging events, got %v", csbl.mml.events) | ||
} | ||
if ssbl.mml.events != 8 { | ||
t.Fatalf("want 8 server side binary logging events, got %v", ssbl.mml.events) | ||
} |
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.
This is WAI. I looked over my diff (and found some correctness issues - which you can see in the diff of the commit I'm about to push out). However, my diff from master really keeps the exact same number of logging calls in each operation/over the stream lifetime.
* Add binary logger option for client and server
…ersion to 1.50.1 (#5722) * Add binary logger option for client and server (#5675) * Add binary logger option for client and server * gcp/observability: implement public preview config syntax, logging schema, and exposed metrics (#5704) * Fix o11y typo (#5719) * o11y: Fixed o11y bug (#5720) * update version to 1.50.1
This PR adds a binary logger option for client and server. This is needed due to the observability config iteration of a partition between client and server events.
RELEASE NOTES: N/A