891907 - in talos, counter manager instantiation, management, and cleanup is not intuitive or well written

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

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

there is some basic cleanup we can do and probably a series of other actions.  From bug 875914, jhammel has a good recommendation for cleaning up counters:

::: talos/ttest.py
@@ +436,5 @@
>  
> +        except talosRegression, tr:
> +            counter = None
> +            if 'cm' in vars():
> +                counters = cm

counters = vars().get('cm', counters)

That said,

A. this is repeat code
B. the whole thing is a bit of an anti-pattern; AIUI,
  - 'counters' is a list of counters; cm is a counter manager that is instantiated only if counters is non empty: http://hg.mozilla.org/build/talos/file/a11542b55a70/talos/ttest.py#l361
  - so, we strangely look in vars() which is probably wrong anyway since counters could be per test, at least in theory
C. the right way to do this is just to fix cm to be None per test if counters is not none


We should really put a few hours into cleaning up the counters code so it is better integrated and easier to work with in the future.

Comment 1

[Julien Pagès (:parkouss)]

Attachment: 891907.patch

This patch hide the counter complexity. I tried locally by activating linux counters on tresize, this seems to give me the same kind of results.

Note that this patch should be applied on top of the patch in bug 1190376.

Comment 2

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

Comment on attachment 8646471 [details] [diff] [review]
891907.patch

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

this is nice and simple, thanks!

Comment 3

[Julien Pagès (:parkouss)]

Pushed to try:

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

Comment 4

[Julien Pagès (:parkouss)]

All green - landed:

https://hg.mozilla.org/build/talos/rev/7fe8298e0aa2