Evaluate Telemetry on global STUN rate limit, and re-activate with reasonable limits

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

Trunk
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32+ fixed, firefox33 fixed, firefox34 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

5 years ago
No description provided.
Assignee

Updated

5 years ago
Depends on: 1012999
Hi Lawrence, I believe you're the release driver for Fx 32.  We want to re-enable throttling of ICE connectivity checks (which we removed to collect metrics) before we go to release.  We'd love to gather some more metrics on Beta if we can.  So we'd like to re-enable the throttling part-way through Beta (roughly 3 weeks in) with 3 weeks to bake.  Are you ok with that timing?   Thanks.
Flags: needinfo?(lmandel)
I'm fine with this plan. Please setup the re-enablement of throttling based on the EARLY_BETA_OR_EARLIER flag, which the relman team will flip at the midpoint of the beta cycle.

EARLY_BETA_OR_EARLIER is defined depending on the corresponding value in build/defines.sh. This file is managed manually by the release management team, with the variable being cleared once we're past the "early beta" point in the release cycle.
Flags: needinfo?(lmandel)
Attachment #8468084 - Flags: review?(docfaraday)
Assignee

Comment 4

5 years ago
Comment on attachment 8468084 [details] [diff] [review]
re-enable STUN throttling in mid-beta and later

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

We need to do this for the long-term violations as well: http://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#553

We probably also want to wrap the assert, and we'll need to increase the short-term limit since we do see it being exceeded occasionally (we've never seen the long-term limit broken on telemetry). I'm pretty sure 16K would do the trick.
Attachment #8468084 - Flags: review?(docfaraday) → review-
EARLY_BETA_OR_EARLIER is going to be preffed off in beta 6 (today). Do you have enough information to reenable throttling this week?
Flags: needinfo?(mreavy)
Assigning to Randell as he seems to be working on this.
Assignee: nobody → rjesup
Attachment #8468084 - Attachment is obsolete: true
Attachment #8471123 - Flags: review?(docfaraday)
Assignee

Comment 8

5 years ago
Comment on attachment 8471123 [details] [diff] [review]
re-enable STUN throttling in mid-beta and later

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

Looks good with one nit.

::: media/mtransport/nr_socket_prsock.cpp
@@ +516,5 @@
>    if (nr_is_stun_request_message((UCHAR*)msg, len)) {
>      // Global rate limiting for stun requests, to mitigate the ice hammer DoS
>      // (see http://tools.ietf.org/html/draft-thomson-mmusic-ice-webrtc)
>  
>      // Tolerate rate of 8k/sec, for one second.

Nit: Update comment.
Attachment #8471123 - Flags: review?(docfaraday) → review+
Flags: needinfo?(mreavy)
Comment on attachment 8471123 [details] [diff] [review]
re-enable STUN throttling in mid-beta and later

Adding beta approval to the bug for consistency.
Attachment #8471123 - Flags: approval-mozilla-beta+
Assignee

Comment 11

5 years ago
Aren't we going to check this into central? We need to raise the rate limit there too so we can monitor how the new limit does.
Flags: needinfo?(rjesup)
Assignee

Updated

5 years ago
Assignee: rjesup → docfaraday
Status: NEW → ASSIGNED
Assignee

Comment 13

5 years ago
Comment on attachment 8477029 [details] [diff] [review]
Double the global STUN long-term rate limit.

This landed in the wrong place.
Attachment #8477029 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Flags: needinfo?(rjesup)
Keywords: checkin-needed
What about Aurora33? Presumably we need an approval request? :)
Target Milestone: mozilla32 → ---
Assignee

Comment 16

5 years ago
Comment on attachment 8471123 [details] [diff] [review]
re-enable STUN throttling in mid-beta and later

Approval Request Comment

Already approved for beta, needs to go to aurora too.
Attachment #8471123 - Flags: approval-mozilla-aurora?
Assignee

Updated

5 years ago
Blocks: 1057096
https://hg.mozilla.org/mozilla-central/rev/4bd8c4b749e1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Attachment #8471123 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.