1226196 - Does MEMORY_RESIDENT need to have 200 buckets?

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Chris H-C :chutten]

By Betteridge's Law of Headlines, the answer is 'No'. 

Unfortunately, we can't just change histograms like this. So we'll need to rename it so that everything thinks it's new.

This is a good second bug.

0) Read https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe 
1) Remove MEMORY_RESIDENT from toolkit/components/telemetry/bucket_whitelist.json - this makes it subject to normal bucket size rules
2) In toolkit/components/telemetry/Histograms.json, rename MEMORY_RESIDENT (to MEMORY_RESIDENT_FAST?), lower its bucket count to 100, remove the "extended_statistics_ok" line, add a "bug_numbers" line with this bug's number.
3) In toolkit/components/telemetry/TelemetrySession.jsm, rename MEMORY_RESIDENT

Comment 1

[Tyler Steiman]

Attachment: bug1226196-MEMORY_RESIDENT-buckets.diff

Hi Chris,

I've attached a patch file for this bug. If anything needs to be changed please let me know!!! Thanks, and I've assigned this bug to myself just to give you a heads up.

Comment 2

[Chris H-C :chutten]

Comment on attachment 8690223 [details] [diff] [review]
bug1226196-MEMORY_RESIDENT-buckets.diff

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

Looks good to me. Thanks for the patch!

Comment 3

[Chris H-C :chutten]

Would you like any help finding something to work on next?

Comment 4

[Chris H-C :chutten]

Ack, prematurely added checkin-needed. Fairly certain we'll want an r= in that commit message first :)

Comment 5

[Tyler Steiman]

I would love help finding something next to work on!

I will update the commit message, should it be r=chutten or what? Thanks!!

Comment 6

[Chris H-C :chutten]

r=chutten on the first line so that if anything goes wrong, the sheriffs know who to hunt down :)

Then we should make sure it works. How did you test that the patch worked? A record of any testing you performed would be a helpful comment to include on this bug.

Comment 7

[Tyler Steiman]

Well I'm not too savvy on testing as I'm a beginner, the only thing I can tell you is it builds and runs cleanly :P is there anything else I should do to test it? Thanks for the help.

Comment 8

[Chris H-C :chutten]

I'm glad you asked!

Well, what your patch does is remove and add (or, rename) a histogram from Telemetry, Firefox's metrics system. As with seemingly everything, there's an about page for it: about:telemetry. However, if telemetry isn't turned on, most of the data (including your new histogram) will be missing.

Turning telemetry on in a developer build requires some environment variables to trick the build into thinking it's official. (In general, we don't want developer builds to report metrics. It adds noise to the data) But, temporarily, you'll need it on, so edit your .mozconfig and add:
export MOZILLA_OFFICIAL=1
export MOZ_TELEMETRY_REPORTING=1

(.mozconfig is the build configuration file located in the root of your gecko source directory. You probably already have one.)

Then you'll need to rebuild: ./mach build

Once that's done we can test. A good test would be to take a look in about:telemetry and make sure that your current data shows a Histogram called MEMORY_RESIDENT_FAST and doesn't show a histogram called MEMORY_RESIDENT. (these MEMORY_* histograms are collected just after start and periodically throughout a session, so they should always have data)

Comment 9

[Tyler Steiman]

After adding the above to my .mozconfig and building/running the Histogram in about:telemetry is indeed showing `MEMORY_RESIDENT_FAST`! Does that mean I can go ahead and update the patch comment and resubmit?

Comment 10

[Chris H-C :chutten]

Yes, please! Feel free to review+ your reupload as the commit message has the appropriate r=. Then, set 'checkin-needed' as a keyword so that the sheriffs will get your patch included in the next merges.

And then I guess you'll be needing a new bug!
Do you like SQL? bug 1226461 has an SQL statement that's just a few steps from readable
Do you like Android Java? bug 1216344 identifies an abstraction layer that doesn't need to exist
Do you like Python? bug 1226381 is about giving more informative log messages

(these and so many more you can find and tailor to your strengths using "Bugs Ahoy": http://www.joshmatthews.net/bugsahoy/?unowned=1&simple=1 )

Comment 11

[Tyler Steiman]

Attachment: bug1226196-MEMORY_RESIDENT-buckets.diff

Comment 12

[Tyler Steiman]

(In reply to Chris H-C :chutten from comment #10)
> Yes, please! Feel free to review+ your reupload as the commit message has
> the appropriate r=. Then, set 'checkin-needed' as a keyword so that the
> sheriffs will get your patch included in the next merges.

I have done so. Thanks, I will be taking on another bug from the list!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be19277b423f

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be19277b423f

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be19277b423f