1514372 - Consider dropping TELEMETRY_MEMORY_REPORTER_MS probe

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Kris Maglione [:kmag]]

After bug 1505522, the numbers reported by this probe are trivial. The median is 0, the 75th percentile is under 2ms, and the 95th percentile is around 10ms.

I suppose it's possible we may at some point in the future accidentally add an expensive probe to the main thread section of the reporter, but that seems not super likely. And the timing of this code is now much less interesting than lots of other code sections that we don't collect telemetry for.

Comment 1

[Jan-Erik Rediger [:janerik]]

To help Mozilla out with this bug, here's the steps:

0) Comment here on the bug that you want to volunteer to help. I (or someone else) will assign it to you.
1) Download and build the Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build.
- If you have any problems, please ask on IRC (https://wiki.mozilla.org/Irc) in the #introduction channel. They're there to help you get started.
- You can also read the Developer Guide, which has answers to most development questions: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction
2) Start working on this bug. You will need to remove all mentions and code around TELEMETRY_MEMORY_REPORTER_MS.
1. Remove the definition in toolkit/components/telemetry/Histograms.json: https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/toolkit/components/telemetry/Histograms.json#8248-8256
2. Remove it from toolkit/components/telemetry/histogram-whitelists.json: https://searchfox.org/mozilla-central/rev/413dd3a1fab3446749f3690d93ee17e3196f8c37/toolkit/components/telemetry/histogram-whitelists.json#1080
3. Remove the recording code: https://searchfox.org/mozilla-central/rev/413dd3a1fab3446749f3690d93ee17e3196f8c37/xpcom/base/MemoryTelemetry.cpp#234,328-330

- If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #telemetry channel on IRC (https://wiki.mozilla.org/Irc) most hours of most days.
3) Build your change with `mach build` and test your change with `mach test toolkit/components/telemetry/tests/`. Also check your changes for adherence to our style guidelines by using `mach lint`
4) Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
- Here's the guide: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
5) After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
6) ...now you get to think about what kind of bug you'd like to work on next. Let me know what you're interested in and I can help you find your next contribution.

Comment 2

[lshobith0]

I would like to work on this bug. Please assign this to me.

Comment 3

[lshobith0]

I got the following error. All I did was build Firefox using `mach build` and test it using `mach test toolkit/components/telemetry/tests`. I didn't even change the source code. Is it a bug or did I do something wrong?

Error running mach:

    ['test', 'toolkit/components/telemetry/tests/']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

TypeError: coercing to Unicode: need string or buffer, NoneType found

  File "/home/sai/fox/src/mozilla-central/testing/mach_commands.py", line 169, in test
    argv=extra_args, test_objects=tests, **kwargs)
  File "/home/sai/fox/src/mozilla-central/python/mach/mach/registrar.py", line 138, in dispatch
    return self._run_command_handler(handler, context=context, **kwargs)
  File "/home/sai/fox/src/mozilla-central/python/mach/mach/registrar.py", line 95, in _run_command_handler
    result = fn(**kwargs)
  File "/home/sai/fox/src/mozilla-central/python/mach_commands.py", line 99, in python_test
    return self.run_python_tests(*args, **kwargs)
  File "/home/sai/fox/src/mozilla-central/python/mach_commands.py", line 112, in run_python_tests
    self.activate_pipenv(pipfile=None, populate=True, python=python)
  File "/home/sai/fox/src/mozilla-central/python/mozbuild/mozbuild/base.py", line 775, in activate_pipenv
    self.virtualenv_manager.activate_pipenv(pipfile, populate, python)
  File "/home/sai/fox/src/mozilla-central/python/mozbuild/mozbuild/virtualenv.py", line 634, in activate_pipenv
    stderr=subprocess.STDOUT, env=env)
  File "/usr/lib/python2.7/subprocess.py", line 185, in check_call
    retcode = call(*popenargs, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 172, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 394, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1047, in _execute_child
    raise child_exception

Comment 4

[Chris H-C :chutten]

Hm, that's an interesting one I haven't seen before. Let me get a fresh build and try this myself. In the meantime, have you asked about this on the #introduction IRC channel? They're usually quite good at figuring these sorts of things out.

(putting a needinfo on myself to remember to get back to this after my build completes.)

Comment 5

[Chris H-C :chutten]

I've reproduced the failure in a new build and have filed bug 1518200 to look into it.

Turns out the telemetry python tests aren't playing well with mach test at the moment. So instead of mach test toolkit/components/telemetry/tests/ instead run mach test toolkit/components/telemetry/tests/unit

That's a shorter series of tests but should still catch most problems that might crop up while fixing this bug.

Please let me know if you have any other problems.

Comment 6

[Bren Louis Surio]

I'm currently looking into this. Should I submit patch before this is assigned to me?

Comment 7

[Bren Louis Surio]

I'm having problems with building. I was able to build it twice for initial build and testing it, however my visual studio updated so now it's having error like this:

0:07.02 config/external/ffi
0:07.02 process_begin: CreateProcess(NULL, D:/dev/visualstudio/2019/community/VC/Tools/MSVC/14.20.27508/bin/HostX64/x64/ml64.exe -Fowin64.obj -Z7 -c f:/repos/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32/config/external/ffi/win64.asm, ...) failed.
0:07.02 make (e=2): The system cannot find the file specified.
0:07.02 mozmake.EXE[4]: *** [f:/repos/mozilla-source/mozilla-central/config/rules.mk:765: win64.obj] Error 2
0:07.02 mozmake.EXE[3]: *** [f:/repos/mozilla-source/mozilla-central/config/recurse.mk:74: config/external/ffi/target] Error 2
0:07.02 mozmake.EXE[3]: *** Waiting for unfinished jobs....
0:07.03 [0m[0m[1m[32m Finished[0m release [optimized] target(s) in 1.93s
0:07.12 mozmake.EXE[2]: *** [f:/repos/mozilla-source/mozilla-central/config/recurse.mk:34: compile] Error 2
0:07.12 mozmake.EXE[1]: *** [f:/repos/mozilla-source/mozilla-central/config/rules.mk:404: default] Error 2
0:07.12 mozmake.EXE: *** [client.mk:125: build] Error 2
0:07.19 235 compiler warnings present.

14.20.27508 is my previous version, now it's 14.21.27702. How do I update the path that mach build takes? I already tried mach configure and mach bootstrap again. Thank you.

Comment 8

[Bren Louis Surio]

I was able to run the test using mach test toolkit/components/telemetry/tests/unit. But now I have a failed tests:

Ran 144 checks (96 subtests, 48 tests)
Expected results: 142
Unexpected results: 2
test: 1 (1 fail)
subtest: 1 (1 fail)

Unexpected Results

toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
FAIL toolkit/components/telemetry/tests/unit/test_TelemetrySession.js - xpcshell return code: 0
FAIL test_saveLoadPing - [test_saveLoadPing : 365] false == true
f:/repos/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:checkPayload:365
f:/repos/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:test_saveLoadPing:577

Do I have to edit the test to match my changes? Thank you.

Comment 9

[Chris H-C :chutten]

As far as I know no tests should need to be changed to remove TELEMETRY_MEMORY_REPORTER_MS. Did you remove other memory instrumentation or perhaps turn off memory reporting altogether?

Comment 10

[Bren Louis Surio]

Thank you. I have accidentally removed some codes. It is passed now. I'm about to submit a patch. I'm still configuring how to submit to Phabricator.

Comment 11

[Chris H-C :chutten]

Sounds great. Please let me know if you have any questions.

Comment 12

[Bren Louis Surio]

Thank you. I already followed instruction on how to setup arc and phabricator. However when I enter arc diff --trace, I only get this error so I can't patch. Is there something I'm missing out? I already did the authentication and arc install-certificate also, I can login to phabricator website

[2019-05-29 10:54:02] EXCEPTION: (Exception) Failed to passthru proc_open(): proc_open(): CreateProcess failed, error code - 2 at [<phutil>\src\future\exec\PhutilExecPassthru.php:103]
arcanist(head=master, ref.master=7329bc7c32b9), phutil(head=master, ref.master=86ee6e90797c)
  #0 PhutilExecPassthru::execute() called at [<phutil>\src\future\exec\execx.php:50]
  #1 phutil_passthru(string, PhutilCommandString) called at [<phutil>\src\console\PhutilInteractiveEditor.php:148]
  #2 PhutilInteractiveEditor::invokeEditor(string, string, integer) called at [<phutil>\src\console\PhutilInteractiveEditor.php:77]
  #3 PhutilInteractiveEditor::editInteractively() called at [<arcanist>\src\workflow\ArcanistDiffWorkflow.php:1658]
  #4 ArcanistDiffWorkflow::getCommitMessageFromUser() called at [<arcanist>\src\workflow\ArcanistDiffWorkflow.php:1501]
  #5 ArcanistDiffWorkflow::buildCommitMessage() called at [<arcanist>\src\workflow\ArcanistDiffWorkflow.php:469]
  #6 ArcanistDiffWorkflow::run() called at [<arcanist>\scripts\arcanist.php:394]

Comment 13

[Chris H-C :chutten]

Wow, that's something I've never seen before. The Mozilla Phabricator User Guide says the #phabricator channel on IRC should be populated with people who can help. Are you able to ask over there?

Comment 14

[Bren Louis Surio]

I think I found out the reason, it's because I'm not updated. I read the manual, but I'm afraid I might mess something up. What would be next commands I use? How do I combine my commit with the latest? No one is answering on IRC, probably timezone reasons.

$ hg head
changeset:   476441:a73077366144
tag:         tip
fxtree:      central
user:        Arnold Iakab <[e-mail]>
date:        Fri May 31 18:12:45 2019 +0000
summary:     Bug 1512607 Intermittent Windows raptor [taskcluster:error] Task aborted - max run time exceeded r=rwood,pe

changeset:   475794:3286e63d8a2a
user:        Bren Louis <[e-mail]>
date:        Tue May 28 20:57:12 2019 +0800
summary:     Bug 1514372 - Removed time record regarding TELEMETRY_MEMORY_REPORTER_MS

Comment 15

[Chris H-C :chutten]

It shouldn't matter if your source code isn't up-to-date. You should still be able to push for review (the problem would come later when we try to land it after review, if any problem were to come of it).

Since the problem is preventing you from using Phabricator, to get you unstuck we can conduct a review on the patch directly. You can use hg export to generate a patch file and then you can upload it here to the bug and mark me as a reviewer. Specifically, I think (I'm not sure, I use git to access mozilla-central via git-cinnabar) you should only need to hg export -r 475794:3286e63d8a2a -o bug1514372.patch which should generate a file called "bug1514372.patch". Then here on the bug you can click on the link "Attach a file" just under the bug details near the top of this page. Attach the patch file and change the review flag to "?" and then put me in the box (search for chutten) (this way I get an email and it shows up in red that I have something I need to review).

Do you think that will work?

Comment 16

[Bren Louis Surio]

Attachment: bug1514372.patch

Hi. I was able to export the patch. Please check. I just removed the timing part. I was wondering if I should also removed the RECORD macro in MemoryTelemetry.cpp.

Comment 17

[Chris H-C :chutten]

Comment on attachment 9070289 [details] [diff] [review]
bug1514372.patch

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

This is exactly what we need, thank you!

Removing the `RECORD` macro from MemoryTelemetry would probably be a bad idea since it is used to condense what would be a very verbose brick of accumulate calls and memory manager calls into a more-or-less-readable brick of `RECORD` calls.

Thank you for working through the tooling problems. I'll mark this as checkin-needed so a Sheriff will add this to the tree. Then it will start shipping to Firefox users worldwide!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/696ec62765c7
Removed time record regarding TELEMETRY_MEMORY_REPORTER_MS r=chutten

Comment 19

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/696ec62765c7