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

Log cleanup pass #2285

Merged
merged 3 commits into from
Dec 2, 2023
Merged

Log cleanup pass #2285

merged 3 commits into from
Dec 2, 2023

Conversation

davidzhao
Copy link
Member

Demoted a bunch of logs to DEBUG, consolidated logs.

Demoted a bunch of logs to DEBUG, consolidated logs.
RequestSink: reqSink,
ResponseSource: resSource,
NodeID: nodeID,
}, nil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it now returns nodeID, so RTCService would have that information for consolidated logging

t.params.Logger.Errorw("error handling event", err, "event", event.String())
if onNegotiationFailed := t.getOnNegotiationFailed(); onNegotiationFailed != nil {
onNegotiationFailed()
if !t.isClosed.Load() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are getting errors that operations couldn't be completed due to PeerConnection has closed. so handling that race better

"lc (filtered)", t.filteredLocalCandidates.Get(),
"rc (filtered)", t.filteredRemoteCandidates.Get(),
"lc_filtered", t.filteredLocalCandidates.Get(),
"rc_filtered", t.filteredRemoteCandidates.Get(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces in fields are harder to parse. temporary for now, I'll be changing how we handle ice candidates in a separate PR

@@ -376,7 +382,7 @@ func (s *RTCService) ServeHTTP(w http.ResponseWriter, r *http.Request) {
websocket.CloseNormalClosure,
websocket.CloseNoStatusReceived,
) {
pLogger.Infow("exit ws read loop for closed connection", "connID", cr.ConnectionID, "wsError", err)
closedByClient.Store(true)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid a separate line, but include reason for closure in the same entry

if rtpStats != "" {
d.params.Logger.Infow("rtp stats", "direction", "downstream", "mime", d.mime, "ssrc", d.ssrc, "stats", rtpStats)
}
d.params.Logger.Debugw("rtp stats",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry Raja 😢

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go well my trusted friend!!!

@@ -111,7 +111,7 @@ func (u *UpTrackManager) SetPublishedTrackMuted(trackID livekit.TrackID, muted b
track.SetMuted(muted)

if currentMuted != track.IsMuted() {
u.params.Logger.Infow("publisher mute status changed", "trackID", trackID, "muted", track.IsMuted())
u.params.Logger.Debugw("publisher mute status changed", "trackID", trackID, "muted", track.IsMuted())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one has been useful. But can get it from analytics events.

@@ -1297,7 +1297,7 @@ func (s *StreamAllocator) initProbe(probeGoalDeltaBps int64) {
s.channelObserver = s.newChannelObserverProbe()
s.channelObserver.SeedEstimate(s.lastReceivedEstimate)

s.params.Logger.Infow(
s.params.Logger.Debugw(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to make analytics events for these.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I left the other one alone because they contained information about the change. but seems like notifying that we are starting to probe is only useful in limited situations. would be great to have congestion_started and congestion_ended events

@davidzhao
Copy link
Member Author

@boks1971 @paulwe I've added a few more fixes here after the review. merging it in now.

@davidzhao davidzhao merged commit 3fe124c into master Dec 2, 2023
2 checks passed
@davidzhao davidzhao deleted the log-pass branch December 2, 2023 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants