1190376 - move the sps profilling stuff in another module

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Julien Pagès (:parkouss)]

Right now the sps profiling feature is coded into the TTest._runTest method.

We should move that code in his own file, to clearly separate concerns.

See the discussion in bug 1190270 around this.

Comment 1

[Julien Pagès (:parkouss)]

Attachment: sps_profile.patch

This move the sps_profile code in his own module.

Works good locally, with and without --spsProfile option.

pushed a try without sps here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dbe8cf64c09

Maybe we should push one try with sps also ?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8643314 [details] [diff] [review]
sps_profile.patch

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

I like this patch in general, but I don't like the:
from talos import utils
from talos.utils import TalosError
from talos.sps_profile import SpsProfile

I didn't think we had talos as a module?

Comment 3

[Julien Pagès (:parkouss)]

We have now, with bug 1189021. :)

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

I forgot!  How will this work locally?

Comment 5

[Julien Pagès (:parkouss)]

Just fine, since talos is installed (so the package talos exists locally), and "import talos" already worked there.

This module thing was only an issue with harness, since we are not installing talos - but the dependencies on harness. But this has been fixed, so we can use real nice imports now.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

does this imply that you run talos from a specific directory?  I think it is ok, but I want to confirm before changing to a r+.

Also have you done a push to try with --spsProfiling enabled?

Comment 7

[Julien Pagès (:parkouss)]

(In reply to Joel Maher (:jmaher) from comment #6)
> does this imply that you run talos from a specific directory?  I think it is
> ok, but I want to confirm before changing to a r+.

Well, no, this is not required. Not for me at least, as I use the talos 'binary' that is provided. Also no just if the user uses a virtualenv (like what is done using INSTALL.py). So I think this is safe from that point of view.

> 
> Also have you done a push to try with --spsProfiling enabled?

Not yet! I was wondering what would be the appropriate syntax. Maybe exactly the one you provided in bug 1190270 ? Also the try here is pretty unreadable, because of the reasons explained by mail. The fix in bug 1190907 should do the trick for now, so I am waiting for it to be landed before repushing to try here - I think a bit with and whithout --spsProfiling (two trys).

Comment 8

[Julien Pagès (:parkouss)]

Attachment: sps_profile.patch

Asking again for review here. Same patch as before, just a bit more of clean up.

This is working file locally (missingsymbols.zip and profile_tresize.sps.zip are created when using --spsProfile and that MOZ_UPLOAD_DIR is defined), I'll push to try with sps_profile later.

Comment 9

[Julien Pagès (:parkouss)]

Pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e8adbff2ecb

Same with sps profile option:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1a31131f27e

Comment 10

[Julien Pagès (:parkouss)]

Oh, I tried to build the syntax manually for spsProfile, seems like a failure. Canceled, and repushed to try (using trychooser this time):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb12c8fea1af

Comment 11

[Julien Pagès (:parkouss)]

Hmm, looks pretty good but I'm a bit worried with the two red jobs in:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb12c8fea1af

Does not look related to my change to me, but maybe I should try again the job without my change to be sure ?

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

I would try again, it could have been a bad base you started with.

Comment 13

[Julien Pagès (:parkouss)]

Right, pushed to try again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27e3de759a63

Comment 14

[Julien Pagès (:parkouss)]

Joel, I have a bunch of errors that seems unrelated:

send failed, graph server says:
11:20:19     INFO -  RETURN:to determine geomean from 'test_run_values' for  50795710 - local variable 'values' referenced before assignment
11:20:19     INFO -  RETURN:  File "/var/www/html/graphs/server/pyfomatic/collect.py", line 295, in handleRequest
11:20:19     INFO -  RETURN:    geomean = valuesReader(databaseCursor, databaseModule, inputStream, metadata)
11:20:19     INFO -  RETURN:  File "/var/www/html/graphs/server/pyfomatic/collect.py", line 238, in valuesReader
11:20:19     INFO -  RETURN:    raise DatabaseException("to determine geomean from 'test_run_values' for  %s - %s" % (metadata.test_run_id, str(x)))
11:20:19     INFO -  RETURN:
11:20:19     INFO -  RETURN:

Also I still have two win8 talos chrome busted. Will retry those without my patch to be sure.

Comment 15

[Julien Pagès (:parkouss)]

So tried without my patch, one busted, the other green. Will put back my patch an try again some more.

Comment 16

[Julien Pagès (:parkouss)]

put pack my changes, on 4 jobs two busted, two success. It does not seems related to my change. I think we are good landing this, but maybe we should investigate what is this intermittent bug first ?

As you want Joel - I would go to land that patch, since this does not seems related an that it only appears when the spsProfile option is activated on try (I suspect I tried again on a bad base as you suggested). Then we should try again with spsProfile, and maybe fill a bug.

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

lets land the patch.  I suspect this is a specific test causing troubles.

Comment 18

[Julien Pagès (:parkouss)]

Landed:

https://hg.mozilla.org/build/talos/rev/e8854f96ade3