Closed
Bug 1275707
Opened 9 years ago
Closed 9 years ago
Filter out numbers from message manager size telemetry
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(3 files, 1 obsolete file)
4.02 KB,
patch
|
billm
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.48 KB,
patch
|
billm
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
2.68 KB,
patch
|
baku
:
review+
benjamin
:
feedback+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
There are a huge number of different message manager messages with names of the form "ublock0:sb:{N}", where {N} is some number from 1 to over 1000. Having so many different keys seems to cause problems for telemetry and makes it harder to tell how many messages of each type there really are, so this patch combines them by eliminating any digits. It will also help for the webdev tools that use channels with names like "debug:server1.conn5.child1:packet". This will create some ambiguity (eg there are some messages of the form "ublock:sb:{N}"), but that should be a minor issue.
Assignee | ||
Comment 1•9 years ago
|
||
This seems pretty low risk, but I'll do a try run before landing just in case.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3abc64c7e9fe
Attachment #8756529 -
Flags: review?(amarchesini)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8756530 -
Flags: review?(amarchesini)
Assignee | ||
Updated•9 years ago
|
tracking-e10s:
--- → ?
![]() |
||
Updated•9 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8756529 [details] [diff] [review]
part 1 - Factor out a common method for message manager size telemetry.
Maybe you could review these two patches, Bill? They are super simple and I expect baku is gone for the weekend already.
Attachment #8756529 -
Flags: review?(amarchesini) → review?(wmccloskey)
Assignee | ||
Updated•9 years ago
|
Attachment #8756530 -
Flags: review?(amarchesini) → review?(wmccloskey)
Assignee | ||
Comment 5•9 years ago
|
||
I'm also asking for the telemetry data review thing as part of this review.
Attachment #8756975 -
Flags: review?(benjamin)
Attachment #8756529 -
Flags: review?(wmccloskey) → review+
Attachment #8756530 -
Flags: review?(wmccloskey) → review+
Comment on attachment 8756975 [details] [diff] [review]
part 3 - Rename MESSAGE_MANAGER_MESSAGE_SIZE.
Review of attachment 8756975 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/telemetry/Histograms.json
@@ +10731,4 @@
> "bug_numbers": [1260908],
> "expires_in_version": "55",
> "kind": "exponential",
> "high": 8000000,
One thing I regretted with these histograms is not specifying "low": 8192. Can you try doing that? It just makes it a little easier to read the graphs.
Assignee | ||
Updated•9 years ago
|
Attachment #8756975 -
Attachment is obsolete: true
Attachment #8756975 -
Flags: review?(benjamin)
Updated•9 years ago
|
Attachment #8757505 -
Flags: review?(benjamin) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8757505 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 8•9 years ago
|
||
(f? for telemetry data approval)
Updated•9 years ago
|
Attachment #8757505 -
Flags: feedback?(benjamin) → feedback+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6115c96a8701
part 1 - Factor out a common method for message manager size telemetry. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef70956cd95c
part 2 - Remove numbers from the message manager message names. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/5885ad0c0a1d
part 3 - Rename MESSAGE_MANAGER_MESSAGE_SIZE. r=baku
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6115c96a8701
https://hg.mozilla.org/mozilla-central/rev/ef70956cd95c
https://hg.mozilla.org/mozilla-central/rev/5885ad0c0a1d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 11•9 years ago
|
||
I'll nominate this for uplift in a few days after I've confirmed that data has started coming in and is non-spammy.
Flags: needinfo?(continuation)
Assignee | ||
Comment 12•9 years ago
|
||
I've confirmed that there are only around 20 categories now, and the ublock and Adblock stuff is all being merged together.
Flags: needinfo?(continuation)
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8756529 [details] [diff] [review]
part 1 - Factor out a common method for message manager size telemetry.
This approval request is for all three patches.
Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: One kind of e10s OOM-related telemetry was disabled because it was causing telemetry server issues, this makes a small tweak and reenables it.
[Describe test coverage new/current, TreeHerder]: this code runs frequently
[Risks and why]: low, it just makes a small tweak to telemetry (the first part just moves code around)
[String/UUID change made/needed]: none
Attachment #8756529 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•9 years ago
|
Attachment #8757505 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•9 years ago
|
Attachment #8756530 -
Flags: approval-mozilla-beta?
Assignee | ||
Updated•9 years ago
|
status-firefox47:
--- → wontfix
status-firefox48:
--- → affected
Comment 14•9 years ago
|
||
Comment on attachment 8757505 [details] [diff] [review]
part 3 - Rename MESSAGE_MANAGER_MESSAGE_SIZE.
Improve telemetry, taking it.
Should be in 48 beta 2
Attachment #8757505 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•9 years ago
|
Attachment #8756530 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•9 years ago
|
Attachment #8756529 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 15•9 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•