Closed Bug 971357 Opened 6 years ago Closed 4 years ago

Ensure that STUN traffic is logged to the RLogRingbuffer

Categories

(Core :: WebRTC: Networking, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed
Blocking Flags:

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(1 file, 3 obsolete files)

Right now, STUN traffic is logged at DEBUG, but setting the log level to DEBUG causes the RLogRingbuffer to be rapidly overwritten by media traffic logging.
QA Contact: docfaraday
It may be good enough just to log that we've received a STUN response. We probably also want to throttle down the media packet logging.
One possible fix.
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Attachment #8374560 - Flags: review?(ekr)
Unrot and improve.
Attachment #8374560 - Attachment is obsolete: true
Attachment #8374560 - Flags: review?(ekr)
Attachment #8438064 - Flags: review?(ekr)
Byron -- Can you unrot and revector this to Nils when you are both back from PTO?
backlog: --- → webRTC+
Rank: 45
Flags: needinfo?(docfaraday)
Priority: -- → P4
Comment on attachment 8438064 [details] [diff] [review]
Log STUN responses at INFO instead of DEBUG.

Review of attachment 8438064 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag. Byron, please feel free to re-r? and I will review.
Attachment #8438064 - Flags: review?(ekr)
Unrot.
Attachment #8438064 - Attachment is obsolete: true
Comment on attachment 8627732 [details] [diff] [review]
Log STUN responses at INFO instead of DEBUG

Review of attachment 8627732 [details] [diff] [review]:
-----------------------------------------------------------------

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0635073f2c29
Attachment #8627732 - Flags: review?(ekr)
Byron, could you put this on reviewboard?
Bug 971357: Log STUN responses at INFO instead of DEBUG.
Comment on attachment 8627734 [details]
MozReview Request: Bug 971357: Log STUN responses at INFO instead of DEBUG.

Bug 971357: Log STUN responses at INFO instead of DEBUG.
Attachment #8627734 - Flags: review?(ekr)
Attachment #8627732 - Attachment is obsolete: true
Flags: needinfo?(docfaraday)
Attachment #8627732 - Flags: review?(ekr)
https://reviewboard.mozilla.org/r/12265/#review10767

::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c:457
(Diff revision 1)
> -    r_log(NR_LOG_STUN,LOG_DEBUG,"STUN-CLIENT(%s): Received check response (my_addr=%s,peer_addr=%s)",ctx->label,ctx->my_addr.as_string,peer_addr->as_string);
> +    r_log(NR_LOG_STUN,LOG_DEBUG,"STUN-CLIENT(%s): Inspecting STUN response (my_addr=%s,peer_addr=%s)",ctx->label,ctx->my_addr.as_string,peer_addr->as_string);

Suggest a comma between addresses here.

::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c:739
(Diff revision 1)
> +      r_log(NR_LOG_STUN,LOG_WARNING,"STUN-CLIENT(%s): Error %d processing response, stun error code %d.", ctx->label, _status, (int)ctx->error_code);

Use nr_strerror() to translate \_status?
r=me with or without these changes.
Attachment #8627734 - Flags: review?(ekr) → review+
Attachment #8627734 - Flags: review+ → review?(ekr)
Comment on attachment 8627734 [details]
MozReview Request: Bug 971357: Log STUN responses at INFO instead of DEBUG.

Bug 971357: Log STUN responses at INFO instead of DEBUG.
https://reviewboard.mozilla.org/r/12265/#review10767

> Suggest a comma between addresses here.

I'm going to guess you meant whitespace, since there's already a comma.
Comment on attachment 8627734 [details]
MozReview Request: Bug 971357: Log STUN responses at INFO instead of DEBUG.

Carry forward r=ekr
Attachment #8627734 - Flags: review?(ekr) → review+
https://reviewboard.mozilla.org/r/12265/#review10893

I don't know why this won't let me mark it ship it, but LGTM
https://hg.mozilla.org/mozilla-central/rev/740bf479671a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.