1518313 - Remove --geckoProfile from talos-test mach command now that we have --gecko-profile

Status: RESOLVED FIXED

(Testing :: Talos, enhancement, P3)

Description

[Mike Conley (:mconley) (:⚙️)]

Confusingly, it seems like profiling a Talos test can be invoked from the command line either with --gecko-profile or --geckoProfile:

https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/testing/talos/talos/cmdline.py#97-105

and

https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/testing/talos/talos/cmdline.py#91-96

The latter ones are suppressed from --help output. We should probably just remove them at some point.

Comment 1

[Shriram (irc: Mavericks)]

Attachment: bug1518313.patch

Comment 2

[Ionuț Goldan [:igoldan]]

Handing this over to Alexandru, as I don't have the cycles.

Comment 3

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Shriram, please do a search after geckoProfile related code and make sure you covered all the relevant files. You missed fixing:

• talos.py
• you need to double check if the changes in test_config.py

Might be other things. Also, right before submitting the package please do a rebase if needed.

Comment 4

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Comment on attachment 9076180 [details] [diff] [review]
bug1518313.patch

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

Shriram, please do a [search after geckoProfile](https://searchfox.org/mozilla-central/search?q=geckoProfile%7CgeckoProfileInterval%7CgeckoProfileEntries&regexp=true&path=testing%2Ftalos) related code and make sure you covered all the relevant files. You missed fixing:
- [talos.py](https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/talos.py)
- you need to double check if the changes in [test_config.py](https://searchfox.org/mozilla-central/source/testing/talos/talos/unittests/test_config.py)

Might be other things. Also, right before submitting the package please do a rebase if needed.

Comment 5

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Comment on attachment 9076180 [details] [diff] [review]
bug1518313.patch

Shriram, please do a [search after geckoProfile](https://searchfox.org/mozilla-central/search?q=geckoProfile%7CgeckoProfileInterval%7CgeckoProfileEntries&regexp=true&path=testing%2Ftalos) related code and make sure you covered all the relevant files. You missed fixing:
- [talos.py](https://searchfox.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/talos.py)
- you need to double check if the changes in [test_config.py](https://searchfox.org/mozilla-central/source/testing/talos/talos/unittests/test_config.py)

Might be other things. Also, right before submitting the package please do a rebase if needed.

Comment 6

[Shriram (irc: Mavericks)]

Thank you Ionuț and Alexandru.
Alexandru, I will come up with another patch that addresses the issues in your comment.

Comment 7

[Shriram (irc: Mavericks)]

Attachment: Removes geckoProfile argument usage from Talos testing related code

The latest diff should cover all relevant talos files as per comment #3

Comment 8

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Shriram, I didn't forget this patch, I'm looking into it as I'm investigating some consistency issues.

Comment 9

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Ionut, there are some failed gecko profile jobs for talos, because those --geckoProfile options are going up to taskcluster >>>
[task 2020-02-04T10:28:02.201Z] executing ['/usr/bin/python2.7', '-u', 'mozharness/scripts/talos_script.py', '--cfg', 'mozharness/configs/talos/linux_config.py', '--suite=g5', '--use-talos-json', '--setpref=media.peerconnection.mtransport_process=false', '--setpref=network.process.enabled=false', '--download-symbols', 'ondemand', '--geckoProfile']

Raptor has this camelCase to dash-case also, and I would recommend doing this replace for both raptor and talos at once. Otherwise we might get conflicts. Taskgraph is using the same cameCase form of the option for all the tasks, if I read the code correctly.

Dave, what do you say?

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Alexandru Ionescu :alexandrui (needinfo me) from comment #9)

Ionut, there are some failed gecko profile jobs for talos, because those --geckoProfile options are going up to taskcluster >>>
[task 2020-02-04T10:28:02.201Z] executing ['/usr/bin/python2.7', '-u', 'mozharness/scripts/talos_script.py', '--cfg', 'mozharness/configs/talos/linux_config.py', '--suite=g5', '--use-talos-json', '--setpref=media.peerconnection.mtransport_process=false', '--setpref=network.process.enabled=false', '--download-symbols', 'ondemand', '--geckoProfile']

Raptor has this camelCase to dash-case also, and I would recommend doing this replace for both raptor and talos at once. Otherwise we might get conflicts. Taskgraph is using the same cameCase form of the option for all the tasks, if I read the code correctly.

Dave, what do you say?

If Raptor supports hyphen-case then it should be safe to modify taskgraph to use this for both Talos and Raptor. I don't think we need to extend this to removing camelCase from Raptor at this time though.

Comment 11

[Ionuț Goldan [:igoldan]]

Agree with Dave.

Comment 12

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Shriram, you need to make one more update to the patch. Taskcluster is still using the camel case --geckoProfile option to trigger the gecko profiles:
[taskcluster 2020-02-04T10:27:00.773Z] Executing command 1: './run-task' -- '/usr/bin/python2.7' -u 'mozharness/scripts/talos_script.py' --cfg 'mozharness/configs/talos/linux_config.py' --suite=g1 --use-talos-json '--setpref=media.peerconnection.mtransport_process=false' '--setpref=network.process.enabled=false' --download-symbols ondemand --geckoProfile
So in order to avoid the inconsistency, the taskcluster related code needs to be updated. This way the gecko profile jobs failing for the moment to pass. Please add to the patch the replacement of --geckoProfile with --gecko-profile occurrences you find in this search.

Comment 13

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Comment on attachment 9122945 [details] [diff] [review]
Removes geckoProfile argument usage from Talos testing related code

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

Please add to the patch the replacement of --geckoProfile with --gecko-profile occurrences you find in [this search](https://searchfox.org/mozilla-central/search?q=--geckoProfile&case=false&regexp=true&path=taskcluster%2Ftaskgraph%2F).

Comment 14

[Shriram (irc: Mavericks)]

Attachment: bug1518313v3.diff

Comment 15

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Comment on attachment 9128387 [details] [diff] [review]
bug1518313v3.diff

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

Looks good. You just need to remove `import argparse` from `testing/mozharness/mozharness/mozilla/testing/talos.py:12`
Also, by bad that I didn't told you earlier: to submit the patch for review and then to be landed you need to install [moz-phab](https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html). This time I can submit the patch on your behalf if you want, but next time you'll have to do this yourself.
You anyway need to do one more change, so I would suggest doing this now.

Comment 16

[Shriram (irc: Mavericks)]

Attachment: Bug 1518313 - Removes --geckoProfile from talos-test mach command and replaces it with --gecko-profile; r=AlexandruIonescu

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d73ddaef8723
Removes --geckoProfile from talos-test mach command and replaces it with --gecko-profile; r=AlexandruIonescu,perftest-reviewers

Comment 18

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Congrats Shriram, your patch just landed!

Comment 19

[Noemi Erli[:noemi_erli]]

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

Comment 20

[Shriram (irc: Mavericks)]

(In reply to Alexandru Ionescu (needinfo me) [:alexandrui] from comment #18)

Congrats Shriram, your patch just landed!

Thank you Alexandru!
Also thanks Dave and Ionut

Comment 21

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Shriram, if you want some more tasks just stay around, I'll ni? you as soon as I find something interesting.

Comment 22

[Shriram (irc: Mavericks)]

Sure Alexandru. I'll take them up. Thank you so much!