-
Notifications
You must be signed in to change notification settings - Fork 881
ci: Improve peer logging to help identify race #93
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #93 +/- ##
==========================================
+ Coverage 68.87% 71.87% +2.99%
==========================================
Files 85 89 +4
Lines 3396 3612 +216
Branches 55 55
==========================================
+ Hits 2339 2596 +257
+ Misses 860 793 -67
- Partials 197 223 +26
Continue to review full report at Codecov.
|
c.opts.Logger.Debug(context.Background(), "ice gathering state updated", | ||
slog.F("state", iceGatherState)) |
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 adding this additional logging!
c.opts.Logger.Debug(context.Background(), "flushing remote candidate") | ||
hash := sha256.Sum224([]byte(pendingCandidate.Candidate)) | ||
c.opts.Logger.Debug(context.Background(), "flushing buffered remote candidate", | ||
slog.F("hash", hash), |
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.
Neat idea to log this, so we can track if a particular ICE candidate made it across
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 concerned that somehow the ICE candidate isn't getting across the pipe, so at least now we'll know whether it does or not!
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.
Speaking of that - we could also add logging when we set up the channels in the peer/conn_test.go
, too!
case c := <-channel1.LocalCandidate():
_ = channel2.AddRemoteCandidate(c)
Line 304 in f9e594f
case c := <-channel2.LocalCandidate(): |
And it might make sense to assert no error on that AddRemoteCandidate
call? I don't think it should in the local network case (and if it does, might give us extra clues)
No description provided.