744411 - remove the csv output from talos harness

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

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

currently we have the option to output the results from a test run into a csv directory with .csv files for each test type.

Now that we have results_url=file:///path, we can use that instead of csv.  To resolve this bug, we need to:
* remove the csv options from the .config files
* remove the csv options from PerfConfigurator.py
* remove the send_to_csv from run_tests.py
* ensure the toscreen works fine
* ensure the amo output is the same
* ensure >1 test (including shutdown=true and counters) works fine with the output file
* allow for an easy switch (run_tests.py -s) to output to talos.out

Comment 1

[Jeff Hammel]

We should really rethink our entire output story but this is a good start

Comment 2

[James Mueller]

I'd be interested in working on this if nobody else is. This is a first time on Mozilla thing for me, so I'm going to have a lot of questions as to the directories and files that are being worked on. Is there a more comprehensive description of the design or should I just head to #ateam with questions?

Comment 3

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

Hi James,  Please come on to irc #ateam and ask for :jmaher or :jhammel.

In addition, you might want to get started with talos:
https://elvis314.wordpress.com/2011/12/21/love-mozilla-love-python-want-to-help-what-next/

That blog post is a little bit outdated, but will get you setup with a lot of the basics.

Comment 4

[James Mueller]

Sure thing. It might take a day or two for me to get everything installed and working correctly, I'll check in at the IRC when I'm ready.

Comment 5

[James Mueller]

Ok, I think I have everything running. I just ran a quick grep to figure out where the csv stuff is in talos and it looks like we have these files.
/Scripts/ts_desktop.yml (pretty sure this was my output file so it might be a false positive)
/talos/addon.config
/talos/bcontroller.py
/talos/cycles.config
/talos/PerfConfigurator.py
/talos/remote.config
/talos/run_tests.py
/talos/sample.2.config
/talos/sample.config
/talos/xperf.config
/talos/xtalos/etlparser.py

I still don't know what any of this does so I'll have to take a look at it. I'd like to have an idea of what's going on tomorrow night. Let me know if there's any issues with what I'm seeing.

Comment 6

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

This is the right direction.  *.yml files are generated during configure/run time, so you can ignore those for now.

the *.config files just have a simple reference to a csv_dir, that will be a painless change.

PerfConfigurator.py and run_tests.py are the core places that provide .csv support and those will have to be modifed and tested.

Please leave bcontroller.py and etlparser.py alone.  We use .csv format for the xperf tracing analysis and that is unrelated to the output format for general talos that we are cleaning up.

Thanks for the due diligence!

Comment 7

[James Mueller]

Going through the code, it looks like there's an issue with the error handling in send_to_csv and send_to_graph. Right now, both raise a talosError if the print_format is not tsformat or tpformat, so it seems likely that the code around line 689 could raise a talosError in send_to_graph due to a bad print format and then raise the same error in send_to_csv.

This is also an issue because send_to_csv is being used to print to stdout which means that deleting it would remove functionality beyond printing to csv files.

The two major issues are:
1) Error handling
2) send_to_csv having functionality beyond sending to csv files

The first seems beyond the scope of this bug, but the two solutions I see for the second issue are either maintaining the functionality of send_to_csv for printing to stdout or altering the output there.

Comment 8

[James Mueller]

It looks like the solution is going to be to replace send_to_csv there with send_to_graph(file:///./results.out), for now at least.

Comment 9

[Jeff Hammel]

(In reply to James Mueller from comment #8)
> It looks like the solution is going to be to replace send_to_csv there with
> send_to_graph(file:///./results.out), for now at least.

to be overly pedantic, probably 'file://%s' % os.path.join(os.getcwd(), 'results.out')

Comment 10

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

Apparently the initial request of this was only half baked.  We want to not print to screen or amo.  The AMO and screen formats can use the send_to_graph(<localfile>).  This gets us to a single code path which is more maintainable and streamlined for future work.

New bugs should be filed for:
* make send_to_graph more robust
* allow for printing a localfile results_url to the screen
* adjust the AMO chain to work with send_to_graph results (will be raw results in the future)

This is starting to have a cleaner definition.

Comment 11

[James Mueller]

I might just be confusing myself, but it sounds like printing to stdout with to_screen and amo is ok to remove and replace with printing to a local file? Specifically at line 668.

Comment 12

[Jeff Hammel]

(In reply to James Mueller from comment #11)
> I might just be confusing myself, but it sounds like printing to stdout with
> to_screen and amo is ok to remove and replace with printing to a local file?
> Specifically at line 668.

Yep, I think that is fine.  We should already have the raw results if you're using the noisy flag anyway

Comment 13

[James Mueller]

I performed the changes and it ran, but it looks like I might have the graph server set up incorrectly or something along those lines since it's erroring out when transmitting the graph. I'll have to check in later with the IRC to figure it out, but the errors aren't related to any code I changed.

Comment 14

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

Yeah, that is confusing.  Really the graph servers are unaccessible unless you are on this special VPN.  They always fail for me as well.  

I would say next steps are to post the patch (hg diff, or if you are using queues, the patch in .hg/patches) to this bug for review and we can also get it pushed to our try server to run all the tests on it for sanity checking.

Comment 15

[James Mueller]

That makes sense. I'll have some questions later but I'll try to run through it now.

Comment 16

[James Mueller]

Should I just copy the output of hg diff here?

Comment 17

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

do a 'hg diff > bug744411.patch'.  Then add that as an attachment to this bug.  It is a bit confusing at first, but becomes second nature after a couple patches.  I switched to mercurial queues to make it a bit easier.

Comment 18

[James Mueller]

Attachment: Potential bug fix

Comment 19

[James Mueller]

Ok, that is pretty nifty how you can see all the changes by clicking on [diff]. Also, this was never assigned to me, what's the policy for assigning bugs?

Comment 20

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

You should be able to edit the 'assigned to' field and put your name in it.  If you cannot, myself or somebody else can edit it for you.  

The general policy is if you plan to work on the bug, assign it to yourself.  Pretty simple.

Comment 21

[James Mueller]

Looks like I don't have permissions. It's not enormously important, I guess, but it'd be nice to have it assigned to me.

Comment 22

[Jeff Hammel]

Also, in general, you should set the feedback or review flag (available when you upload the attachment or at https://bugzilla.mozilla.org/attachment.cgi?id=617353&action=edit ) if you want feedback or review of a patch.  Feedback generally means "The patch is not in final form but I would like input in what I have" where review means "I think the patch is in final form.  Do you think it is in the form to check in?"  For either, you should add the name of someone to do the feedback/review in the text field next to the dropdown (the latter, you should set to '?').

Comment 23

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

Comment on attachment 617353 [details] [diff] [review]
Potential bug fix

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

::: talos/run_tests.py
@@ +271,5 @@
>        times += 1
>        time.sleep(wait_time)
>        wait_time += wait_time
>      else:
> +      # TODO: Change error type

we should either do this in this bug or file a followup bug to handle this.

@@ +609,3 @@
>      except talosError, e:
>        utils.stamped_msg("Failed sending results", "Stopped")
>        #failed to send results, just print to screen and then report graph server error

edit this comment as well.

@@ +629,3 @@
>                      action='store_true', default=False,
>                      help="output CSV to screen")
> +  '''

Either remove this completely, or:
1) file a bug to handle this
2) print to a temp file and then display that file to the screen

Comment 24

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

there is a mention of csv_filters which are still in run_tests.py, other than that, this is looking clean.

Comment 25

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

pushed to try...should have results within 12 hours posted to this bug.

Comment 26

[Jeff Hammel]

Comment on attachment 617353 [details] [diff] [review]
Potential bug fix

+      # TODO: Change error type

Probably don't want the comment without a follow-up bug

       # use filters to approximate what graphserver does:
       # https://github.com/mozilla/graphs/blob/master/server/pyfomatic/collect.py#L212
       # ultimately, this should not be in Talos:
       # https://bugzilla.mozilla.org/show_bug.cgi?id=721902
-      if csv_dir:
-        send_to_csv(csv_dir, {testname : results[testname]}, csv_filters)
-      if options["to_screen"] or options["amo"]:
-        send_to_csv(None, {testname : results[testname]}, csv_filters)
+      

Please remove the comment too as it is no longer valid with the patch

+  # TODO: to_screen's functionality needs to be reworked
+  '''parser.add_option('-s', '--screen', dest='to_screen',
                     action='store_true', default=False,
                     help="output CSV to screen")
+  '''

I would just remove.  AFAIK it isn't used in production (:jmaher?)

Comment 27

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

to_screen is not used in production, only on developer builds if they even know about it.  The direction we are going is to not do that, so we can remove!

Comment 28

[James Mueller]

Attachment: Second patch attempt

Comment 29

[James Mueller]

Attachment: Removed an additional two comments

Comment 30

[cmtalbert]

Comment on attachment 617666 [details] [diff] [review]
Removed an additional two comments

Changing to proper reviewer.

Comment 31

[James Mueller]

Attachment: Was running diff on wrong copy of talos

Comment 32

[Jeff Hammel]

Comment on attachment 617677 [details] [diff] [review]
Was running diff on wrong copy of talos

looks great! thanks for the patch!

Comment 33

[Mozilla RelEng Bot]

Try run for d22506cd4b23 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d22506cd4b23
Results (out of 80 total builds):
    exception: 5
    success: 75
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d22506cd4b23

Comment 34

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

http://hg.mozilla.org/build/talos/rev/6f8e4c3efc62