816634 - output datazilla URLs to TBPL for each talos run

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

TBPL should display the datazilla URL. The existing TBPL output is
something like this:


RETURN: <a
href='http://graphs.mozilla.org/graph.html#tests=[[224,94,14]]'>tsvgr:
4047.32</a>
RETURN: <a
href='http://graphs.mozilla.org/graph.html#tests=[[225,94,14]]'>tsvgr_opacity:
98.0</a>
NOISE: Outputting datazilla results to
https://datazilla.mozilla.org/talos
NOISE: datazilla: https//datazilla.mozilla.org/talos; oauth=True

We should also output the datazilla URL(s) for these tests.

Talos will need the revision and branch name (which it already has
from application.ini)

Comment 1

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

Here is what example output would look like:

https://datazilla.mozilla.org/talos/summary/Mozilla-Inbound/1e2b9cdc486b
https://datazilla.mozilla.org/talos/summary/Mozilla-Inbound-Non-PGO/1e2b9cdc486b

https://datazilla.mozilla.org/talos/summary/Services-Central/1e2b9cdc486b
https://datazilla.mozilla.org/talos/summary/Services-Central-Non-PGO/1e2b9cdc486b

Comment 2

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

I would really like to see links to individual tests, i.e.:
https://datazilla.mozilla.org/talos/summary/Mozilla-Inbound/1e2b9cdc486b/tp5n

Comment 3

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

right now we format the output to tbpl as:
tresize: <a href="">11.0</a>

where would datazilla go?  I am leaning towards:
tresize: <a href="">11.0</a> <a href="https://datazilla.mozilla.org/talos/...">datazilla</a>

If we did that, then we would need to modify the talos graph server output bit to hack in a link to datazilla.  That is doable, but we would never know if datazilla uploading succeeded or not.

Another solution might be to create a browser addon which does this for you, I really don't like that idea.

Comment 4

[Ed Morley [:emorley]]

(In reply to Joel Maher (:jmaher) from comment #3)
> If we did that, then we would need to modify the talos graph server output
> bit to hack in a link to datazilla.  That is doable, but we would never know
> if datazilla uploading succeeded or not.

I'd say that's fine.

We can always get the later upload build step to output an error (which I'm guessing it already does), so people looking at the results know why the URL leads to a 404.

Comment 5

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #3)
> right now we format the output to tbpl as:
> tresize: <a href="">11.0</a>
> 
> where would datazilla go?  I am leaning towards:
> tresize: <a href="">11.0</a> <a
> href="https://datazilla.mozilla.org/talos/...">datazilla</a>
> 
> If we did that, then we would need to modify the talos graph server output
> bit to hack in a link to datazilla.  That is doable, but we would never know
> if datazilla uploading succeeded or not.

I am more inclined to modify the master results class so that the TBPL prints can be serialized together.  I can work on this if you want.

> Another solution might be to create a browser addon which does this for you,
> I really don't like that idea.

No, I don't like that either

Comment 6

[Jeff Hammel]

The current methodology is that we get a list of links from the graphserver after we post there:

http://hg.mozilla.org/build/talos/file/e00b0636f9f3/talos/output.py#l327

Unfortunately, this means its hard to correllate tests with what graphserver is giving back.

I'll probably need some example results from graphserver as ABICT this will be hard to test.

Comment 7

[Mozilla RelEng Bot]

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

Comment 8

[Jonathan Eads ( :jeads )]

(In reply to Joel Maher (:jmaher) from comment #1)
> Here is what example output would look like:
> 
> https://datazilla.mozilla.org/talos/summary/Mozilla-Inbound/1e2b9cdc486b
> https://datazilla.mozilla.org/talos/summary/Mozilla-Inbound-Non-PGO/
> 1e2b9cdc486b
> 
> https://datazilla.mozilla.org/talos/summary/Services-Central/1e2b9cdc486b
> https://datazilla.mozilla.org/talos/summary/Services-Central-Non-PGO/
> 1e2b9cdc486b

I think we're going to also need to specify the product name and potentially version in the url.  

https://datazilla.mozilla.org/talos/summary/Mozilla-Inbound/1e2b9cdc486b?product=Firefox&branch_version=16.0a1

The same revision can occur on the same branch in multiple products (Firefox, Fennec).  I'm not sure if the same revision/branch/product combination could also have multiple versions, if so then we will need that as well. If you're trying to build a url to a particular set of test results, the user interface will need to know which to load.  This data is present in the JSON structures that Talos sends so hopefully this is not too difficult.  If it's helpful I can make sensible defaults, if there are sensible defaults to make...  I could dynamically determine what the most current product version is so that could be used as a default.

I think I'm also going to need to present additional links in the UI for all of the product/version combinations that are available for the revision/branch of interest.  Still thinking about ways of doing this at the moment...  

Also, from the irc discussion with edmorley and jmaher, I've added test and platform to the url as possible key/value pairs to use for application initialization: 

https://datazilla.mozilla.org/talos/summary/Mozilla-Inbound/1e2b9cdc486b?product=Firefox&branch_version=16.0a1&test=Talos tp5n&platform=linux fedora 12 arm

The test/platform names will need to match those used by talos or we will need a mapping to the names used by TBPL.  I can set that up if we need it but I'm not sure if we do yet...

Comment 9

[Ed Morley [:emorley]]

Ok, thinking about this bug and bug 794901 more:
* The easiest way for us to get the correct URLs is just to get Talos to work them out, rather than trying to do in the TBPL UI. This also avoids situations where add/remove/rename testsuites in TBPL and branch talos results no longer display properly - since each branch's version of Talos will automatically be in sync with what we are running there.
* That said, markup in logs is a god-awful idea.

-> I propose Talos/buildbot output the results in a human-readable format (even something as simple as TinderboxPrint: TalosResult: 123.4 TestName: foo DatazillaURL: bar) - I don't mind what, so long as it *isn't* HTML markup. 

-> Over in bug 794901 I adjust the scrape function for talos to support this new non-markup format (ensuring TBPL still supports the old one, until everything works it's way through the trains).

Sound ok?

Comment 10

[Ed Morley [:emorley]]

json wfm too, just not markup! :-)

Comment 11

[Jeff Hammel]

So as a proposal, how about this:
TinderboxPrint: [
TinderboxPrint: {'ts':
TinderboxPrint: {'datazilla': 'http://datazilla.mozilla.org/talos/whatever',
TinderboxPrint:  'graphserver': 'http://graphs.mozilla.org/whatever'
TinderboxPrint: }},
TinderboxPrint: # other tests would go here
TinderboxPrint: ]

Comment 12

[Jeff Hammel]

(also, I'm still mostly blindly guessing at what is desired from a TBPL perspective, so suggestions welcome)

Comment 13

[Ed Morley [:emorley]]

(In reply to Jeff Hammel [:jhammel] from comment #11)
> So as a proposal, how about this:
> TinderboxPrint: [
> TinderboxPrint: {'ts':
> TinderboxPrint: {'datazilla': 'http://datazilla.mozilla.org/talos/whatever',
> TinderboxPrint:  'graphserver': 'http://graphs.mozilla.org/whatever'
> TinderboxPrint: }},
> TinderboxPrint: # other tests would go here
> TinderboxPrint: ]

As long as JSON.parse() can copy with it then anything wfm really. 

The only thing I can think of, is adding a more obvious start marker so we can differentiate between the results json and prior/later buildbot based TinderboxPrints and/or tell the old format from the new (though I guess we could handle the latter by catching the JSON.parse() exception). 

Whilst Talos runs don't currently have any other tinderboxPrints (apart from the results), other suites do (eg revision used for the test run etc), so maybe we need to presume there may be some in the future?

Comment 14

[Jeff Hammel]

Any suggestions?  To me this sounds eerily close to markup, e.g.

<report>
[{'ts': ...}]
</report>

Comment 15

[Ed Morley [:emorley]]

That's a bit of a strawman argument, I was thinking more of "### Talos Result Start ###" ;-)

Or we could just stick it on one line with a prefix, eg:
TinderboxPrint: TalosResult: [{'ts': ...}]

We could also always do what mozharness does, and have a CLI option for tbpl, such that with it, we'll output the json on one line as above, and without, something non-json human readable?

Comment 16

[Mozilla RelEng Bot]

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

Comment 17

[Jeff Hammel]

(In reply to Ed Morley [UTC+0; email:edmorley@moco] from comment #15)
> That's a bit of a strawman argument, I was thinking more of "### Talos
> Result Start ###" ;-)
> 
> Or we could just stick it on one line with a prefix, eg:
> TinderboxPrint: TalosResult: [{'ts': ...}]
> 
> We could also always do what mozharness does, and have a CLI option for
> tbpl, such that with it, we'll output the json on one line as above, and
> without, something non-json human readable?

I'm for TinderboxPrint: TalosResult: [] on one line.  If you have datazilla or graphserver URLs these should be present.  If not...it shouldn't ;)

Comment 18

[Jeff Hammel]

So what graphserver actually gives us back:

"""
tp5n_paint	graph.html#tests=[[206,113,14]]
tp5n_paint	376.68	graph.html#tests=[[206,113,14]]
tp5n_main_rss_paint	graph.html#tests=[[209,113,14]]
tp5n_main_rss_paint	119728242.69	graph.html#tests=[[209,113,14]]
tp5n_pbytes_paint	graph.html#tests=[[207,113,14]]
tp5n_pbytes_paint	461008181.58	graph.html#tests=[[207,113,14]]
tp5n_xres_paint	graph.html#tests=[[211,113,14]]
tp5n_xres_paint	7821104.05	graph.html#tests=[[211,113,14]]
tp5n_shutdown_paint	graph.html#tests=[[215,113,14]]
tp5n_shutdown_paint	1069.00	graph.html#tests=[[215,113,14]]
"""

A fairly annoying format but what can you do? :(

Comment 19

[Ed Morley [:emorley]]

(In reply to Jeff Hammel [:jhammel] from comment #17)
> I'm for TinderboxPrint: TalosResult: [] on one line.  If you have datazilla
> or graphserver URLs these should be present.  If not...it shouldn't ;)

sgtm

Comment 20

[Mozilla RelEng Bot]

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

Comment 21

[Jeff Hammel]

Deconflating the individual graphserver responses, one obtains:

['tsvgr\tgraph.html#tests=[[224,113,14]]\ntsvgr\t2965.75\tgraph.html#tests=[[224,113,14]]\n',
 'tsvgr_opacity\tgraph.html#tests=[[225,113,14]]\ntsvgr_opacity\t98.00\tgraph.html#tests=[[225,113,14]]\n']

Comment 22

[Jeff Hammel]

So the JSON format I propose in comment 11 won't really suffice, as it is missing the result # for graphserver.  Maybe something like:

[{'ts': {'graphserver': {'url': 'http://graphs.m.o/...', 'result': 123.4}}, ...]

So the reason that the outer layer is (python terms) a list and not a dict is to preserve the order of the tests.  If we don't care about this, we can put all of the tests in one dict.

Comment 23

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

I don't believe we care about the order of the tests.

Comment 24

[Ed Morley [:emorley]]

(In reply to Joel Maher (:jmaher) from comment #23)
> I don't believe we care about the order of the tests.

Agreed

Otherwise looks good :-)

Comment 25

[Jeff Hammel]

Attachment: restructure TBPL output thingy

This converts the TBPL output to a JSON struct.  Currently this is only done for graphserver URLs, since ABICT the URLs needed for datazilla don't yet exist.

There will be a bit of the ole deployment switcheroo required here too.  TBPL will need to parse the new (and old!) format before this can be deployed -> talos.

Comment 26

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

Comment on attachment 689295 [details] [diff] [review]
restructure TBPL output thingy

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

a few questions/comments.  I would like this to:
1) Support both formats in the interim
2) Support graphserver and datazilla urls from the first version.

::: talos/output.py
@@ +312,5 @@
>                  wait_time *= 2
>              else:
>                  raise utils.talosError("Graph server unreachable (%d attempts)\n%s" % (self.retries, msg))
>  
> +        # add TBPL output

wouldn't we want to keep the old results_from_graph code and call it here as well?  This way we can land this now and worry about tbpl when this is deployed.

@@ +332,5 @@
>  
> +            # parse the response
> +            lines = [line.strip() for line in response.strip().splitlines()]
> +            assert len(lines) == 2
> +            testname, result, path = lines[-1].split()

why linese[-1] ?

@@ +337,5 @@
> +            if self.isMemoryMetric(testname):
> +                result = filesizeformat(result)
> +
> +            # find a place to put it
> +            test_dict.setdefault(testname, {})['graphserver'] = {'url': url_format % (scheme, server, path),

url_format seems overkill here.

@@ +338,5 @@
> +                result = filesizeformat(result)
> +
> +            # find a place to put it
> +            test_dict.setdefault(testname, {})['graphserver'] = {'url': url_format % (scheme, server, path),
> +                                                                 'result': result}

where do you set tbpl_output or assign it a value?

Comment 27

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

what is next on this bug?

Comment 28

[Jeff Hammel]

Sorry, I thought I responded to this about a week ago :)  Suffice it to say, still being worked on, more details inline.

(In reply to Joel Maher (:jmaher) from comment #26)
> Comment on attachment 689295 [details] [diff] [review]
> restructure TBPL output thingy
> 
> Review of attachment 689295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a few questions/comments.  I would like this to:
> 1) Support both formats in the interim
> 2) Support graphserver and datazilla urls from the first version.

Yep, I agree.  As soon as we can get rid of the legacy format from TBPL, we should.

> ::: talos/output.py
> @@ +312,5 @@
> >                  wait_time *= 2
> >              else:
> >                  raise utils.talosError("Graph server unreachable (%d attempts)\n%s" % (self.retries, msg))
> >  
> > +        # add TBPL output
> 
> wouldn't we want to keep the old results_from_graph code and call it here as
> well?  This way we can land this now and worry about tbpl when this is
> deployed.

I kinda morphed the poorly documented and cargo-culted results_from_graph code to the new function.  They basically deal with the same information.  In the WIP patch, which I'll post following, I do use the same function for both outputs.

> @@ +332,5 @@
> >  
> > +            # parse the response
> > +            lines = [line.strip() for line in response.strip().splitlines()]
> > +            assert len(lines) == 2
> > +            testname, result, path = lines[-1].split()
> 
> why linese[-1] ?

Damned if I know ;)  Graphserver gives responses like:

 'tsvgr\tgraph.html#tests=[[224,113,14]]\ntsvgr\t2965.75\tgraph.html#tests=[[224,113,14]]\n'

So thius is basically the same information except that the second line also has the graphserver-computed metric.  IMHO an awful format, but I don't care about changing graphserver at this point.

> @@ +337,5 @@
> > +            if self.isMemoryMetric(testname):
> > +                result = filesizeformat(result)
> > +
> > +            # find a place to put it
> > +            test_dict.setdefault(testname, {})['graphserver'] = {'url': url_format % (scheme, server, path),
> 
> url_format seems overkill here.

Taken straight from http://hg.mozilla.org/build/talos/file/7217b5b18353/talos/output.py#l332 except I now use the actual scheme that we give it vs insisting on http:// .

> @@ +338,5 @@
> > +                result = filesizeformat(result)
> > +
> > +            # find a place to put it
> > +            test_dict.setdefault(testname, {})['graphserver'] = {'url': url_format % (scheme, server, path),
> > +                                                                 'result': result}
> 
> where do you set tbpl_output or assign it a value?

Should be in results.py

Comment 29

[Jeff Hammel]

Attachment: WIP

Just updating what I have so far

Comment 30

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #27)
> what is next on this bug?

So there are a few things:

- the WIP should support both the legacy case and the new case for graphserver URLs

- datazilla URLs TBD; I'll wire these up in the form of comment 8; that said, they won't link to anything until datazilla gets the uplift to support these. If we want to deploy before this, we should be able to comment out a few lines of code and call it a day

- TBPL has to be changed to parse the new data structure (if present) and display according to that (and obviously be redeployed)

I *think* that is it

Comment 31

[Jeff Hammel]

> - datazilla URLs TBD; I'll wire these up in the form of comment 8; that said, they won't link to anything until datazilla gets the uplift to support these. If we want to deploy before this, we should be able to comment out a few lines of code and call it a day

I'm going to throw this in to talos for now but this should be upstreamed to datazilla_client. Let's not close until that's done unless someone wants to file a follow-up

Comment 32

[Jeff Hammel]

Another thing to thing about:  the way we currently have this structured, the datazilla URLs aren't really test-specific.  We had talked about in the past having an anchor for the testname, e.g. instead of 

TinderboxPrint: TalosResult: {"ts": {"datazilla": {"url": "https://datazilla.allizom.org/test/summary/mozilla-central/634180132e68?product=Firefox&branch_version=20.0a1"}}}

having

TinderboxPrint: TalosResult: {"ts": {"datazilla": {"url": "https://datazilla.allizom.org/test/summary/mozilla-central/634180132e68?product=Firefox&branch_version=20.0a1#ts"}}}

Are we doing this?  Are we recording URLs per test as per half of the comments in this bug, or are we reconsidering?

Comment 33

[Jeff Hammel]

Attachment: noneedforatitle

TBPL won't pick this up yet, but it is something.  Currently I set the same datazilla URL for all tests, so we should figure out what we want to do there.

Comment 34

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

Comment on attachment 691600 [details] [diff] [review]
noneedforatitle

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

overall this looks like a cleaned up version of the last one, are we running green on try server with this?

::: talos/run_tests.py
@@ +25,5 @@
>  here = os.path.dirname(os.path.realpath(__file__))
>  
>  def browserInfo(browser_config, devicemanager=None):
> +  """Get the buildid and sourcestamp from the application.ini (if it exists)"""
> +  # XXX this should probably be moved to PerfConfigurator.py

when running in a 2 stage setup, perfconfigurator & run_tests.py, we want to make sure that we are reporting the correct browser.  On production environments it is all the same location, just refreshing the directory.

Comment 35

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #34)
> Comment on attachment 691600 [details] [diff] [review]
> noneedforatitle
> 
> Review of attachment 691600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> overall this looks like a cleaned up version of the last one, are we running
> green on try server with this?

I have yet to push; will do so shortly.

> ::: talos/run_tests.py
> @@ +25,5 @@
> >  here = os.path.dirname(os.path.realpath(__file__))
> >  
> >  def browserInfo(browser_config, devicemanager=None):
> > +  """Get the buildid and sourcestamp from the application.ini (if it exists)"""
> > +  # XXX this should probably be moved to PerfConfigurator.py
> 
> when running in a 2 stage setup, perfconfigurator & run_tests.py, we want to
> make sure that we are reporting the correct browser.  On production
> environments it is all the same location, just refreshing the directory.

in run_tests.py, we already get the browser_info from perfconfigurator: http://hg.mozilla.org/build/talos/file/1eab02b9475e/talos/run_tests.py#l177 ; I mostly think the method should be moved there or what not; it should still be called from run_tests.py, and not from PerfConfigurator when doing the 2 stage thing.

Comment 36

[Jeff Hammel]

Rereading comment 8 , :jeads has implemented the per test *suite* name locator.  However, this doesn't exactly play in to the existing format:

{'tsvg': {'graphserver': 'value': 12345., 'url': 'graphs.m.o/something/something'},
 'tsvg_opacity': {...}
}

There are two reasons this won't work for datazilla:

1. Looking at comment 8 and the datazilla output from Talos, we prepend the string 'Talos ' to any test we have: http://hg.mozilla.org/build/talos/file/7217b5b18353/talos/output.py#l410 ; There is no reason that I'm aware of *why* we do this, per se. We can certainly stop doing this

2. As you may infer from the code and previous comment, we currently munge all of the "tests", as reported to TBPL, into a single results object.  As such, what we have is incompatible

I'm not really sure what to do about this :( Sadly, I didn't anticipate this going into this code

Comment 37

[Jeff Hammel]

Attachment: Example datazilla json output for tsvg

On TBPL we display these separately as tsvg and tsvg_opacity; in datazilla these are currently "Talos tsvg" as one combined result

Comment 38

[Jeff Hammel]

Thinking about this a bit, I think we have two options:

1. Reconcile Talos to do what we do on TBPL: that is, e.g. split tsvg/tsvg_opacity into two separate categories.  IIRC, we specifically took this out for datazilla.

2. Keep e.g. tsvg/tsvg_opacity as one test and do something new for datazilla.  Unfortunately, this probably means scrapping the format we've agreed to for TBPL.  I'm open to ideas there.

IMHO, the test name as specified to talos, tsvg in this case, should be used as what we're specifying to TBPL.  In other words, option 2.  I also think we should take out the prepended 'Talos ' that we currently do: the URL https://datazilla.mozilla.org/talos/ already tells us that we're dealing with talos data.  However, I could be persuaded on this point.

Comment 39

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

We should remove the 'Talos ', string we send along.

Comment 40

[Ed Morley [:emorley]]

Sorry for the delay replying here; today is the first in a week I've managed to get under double digit un-dealt with bugmails :-/ (whilst philor is away the PST mice will play, or something like that hehe).

(In reply to Jeff Hammel [:jhammel] from comment #38)
> 2. Keep e.g. tsvg/tsvg_opacity as one test and do something new for
> datazilla.  Unfortunately, this probably means scrapping the format we've
> agreed to for TBPL.  I'm open to ideas there.

What's wrong with just displaying them as one test in TBPL?

I'm not too attached to anything we do at the moment, so happy to switch to whatever you think best.

Comment 41

[Jeff Hammel]

So edmorley is on PTO and we need a decision for the Talos side. What I currently proposed is being keyed off of test

{'tsvg': {'graphserver': 'value': 12345., 'url': 'graphs.m.o/something/something'},
 'tsvg_opacity': {...}
}

Since we have no interest in trying to have a 1:1 map between what graphserver does and datazilla does, I'm instead thinking the top-level key should be off of format:

{'graphserver': {'tsvg': {'url': ..., 'value'}},
 'datazilla': {'tsvg': {...}}

Comment 42

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

I am fine with graphserver and datazilla being top level identifiers.

Comment 43

[Ed Morley [:emorley]]

(In reply to Jeff Hammel [:jhammel] from comment #41)
> So edmorley is on PTO 

I didn't realise a response was outstanding?
My thoughts in comment 40 still stand - happy with whatever format you pick for talos :-)

Comment 44

[Jeff Hammel]

(In reply to Ed Morley (Away 20th Dec-2nd Jan) [UTC+0; email:edmorley@moco] from comment #40)
> Sorry for the delay replying here; today is the first in a week I've managed
> to get under double digit un-dealt with bugmails :-/ (whilst philor is away
> the PST mice will play, or something like that hehe).
> 
> (In reply to Jeff Hammel [:jhammel] from comment #38)
> > 2. Keep e.g. tsvg/tsvg_opacity as one test and do something new for
> > datazilla.  Unfortunately, this probably means scrapping the format we've
> > agreed to for TBPL.  I'm open to ideas there.
> 
> What's wrong with just displaying them as one test in TBPL?

ABICT, this is not possible with the way things are reported in graphserver.  I could be wrong

> I'm not too attached to anything we do at the moment, so happy to switch to
> whatever you think best.

Comment 45

[Jeff Hammel]

(In reply to Ed Morley (Away 20th Dec-2nd Jan) [UTC+0; email:edmorley@moco] from comment #43)
> (In reply to Jeff Hammel [:jhammel] from comment #41)
> > So edmorley is on PTO 
> 
> I didn't realise a response was outstanding?
> My thoughts in comment 40 still stand - happy with whatever format you pick
> for talos :-)

Sorry, wasn't waiting for your response.  We just had a meeting and were trying to pick out a new format, just noting that you, as TBPLmaster, wouldn't be around to tell us we were off our proverbial rockers.

Comment 46

[Jeff Hammel]

Attachment: proposed fix

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=a74a5a591de7

One big disadvantage of this patch is that if you use more than one graphserver or datazilla url...this will break.  My personal feeling is that this should be ticketed if this patch lands as is.  Otherwise we can figure it out here (though the answer will almost definitely be yet-another-change-in-format).

Comment 47

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

Comment on attachment 695851 [details] [diff] [review]
proposed fix

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

quick look is good, I want to do a second pass.  Does this pass on try server?  We don't have a need for >1 url for each type.  If we do have a need, it will be in the future when we have a stable format that tbpl can parse and only datazilla output instead of graph server.

Comment 48

[Jeff Hammel]

As of 2012-12-26 15:11:20 PST noted in comment 46, it was running on try. As of now it is still running.

Comment 49

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

Comment on attachment 695851 [details] [diff] [review]
proposed fix

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

great stuff.  It moves us in the right direction.

Comment 50

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

this fails on try server:
https://tbpl.mozilla.org/php/getParsedLog.php?id=18296535&tree=Try&full=1

Comment 51

[Jeff Hammel]

Boo!  Good catch though.

This is a counter-related bug:

NOISE: Posting result 0 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 1 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 2 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 3 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 4 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
Traceback (most recent call last):
  File "run_tests.py", line 326, in <module>
RETURN: <a href='http://graphs.mozilla.org/graph.html#tests=[[206,23,24]]'>tp5n_paint: 180.93</a>
RETURN: <a href='http://graphs.mozilla.org/graph.html#tests=[[207,23,24]]'>tp5n_pbytes_paint: 3.5GB</a>
RETURN: <a href='http://graphs.mozilla.org/graph.html#tests=[[209,23,24]]'>tp5n_main_rss_paint: 232.1MB</a>
    main()
  File "run_tests.py", line 323, in main
    run_tests(parser)
  File "run_tests.py", line 299, in run_tests
    talos_results.output(results_urls, **results_options)
  File "/builds/slave/talos-slave/talos-data/talos/results.py", line 84, in output
    _output.output(results, url, tbpl_output)
  File "/builds/slave/talos-slave/talos-data/talos/output.py", line 65, in output
    self.post(results, results_server, results_path, results_scheme, tbpl_output)
  File "/builds/slave/talos-slave/talos-data/talos/output.py", line 319, in post
    self.add_tbpl_output(links, tbpl_output, server, scheme)
  File "/builds/slave/talos-slave/talos-data/talos/output.py", line 345, in add_tbpl_output
    assert len(lines) == 2
AssertionError

I'll have to dig in more deeply to figure out what is going on here :/  Suffice it to say....we need to add those counters

Comment 52

[Jeff Hammel]

Attachment: WIP

Comment 53

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #52)
> Created attachment 701959 [details] [diff] [review]
> WIP

Testing on try, https://tbpl.mozilla.org/?tree=Try&rev=69182ee49d8d , I see failures:

NOISE: Posting result 0 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 1 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 2 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 3 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
NOISE: Posting result 4 of 5 to http://graphs.mozilla.org/server/collect.cgi, attempt 0
RETURN: <a href='http://graphs.mozilla.org/graph.html#tests=[[206,23,21]]'>tp5n_paint: 268.12</a>
RETURN: <a href='http://graphs.mozilla.org/graph.html#tests=[[207,23,21]]'>tp5n_pbytes_paint: 3.5GB</a>
RETURN: <a href='http://graphs.mozilla.org/graph.html#tests=[[209,23,21]]'>tp5n_main_rss_paint: 193.0MB</a>
Traceback (most recent call last):
  File "run_tests.py", line 326, in <module>
    main()
  File "run_tests.py", line 323, in main
    run_tests(parser)
  File "run_tests.py", line 299, in run_tests
    talos_results.output(results_urls, **results_options)
  File "/Users/cltbld/talos-slave/talos-data/talos/results.py", line 84, in output
    _output.output(results, url, tbpl_output)
  File "/Users/cltbld/talos-slave/talos-data/talos/output.py", line 65, in output
    self.post(results, results_server, results_path, results_scheme, tbpl_output)
  File "/Users/cltbld/talos-slave/talos-data/talos/output.py", line 319, in post
    self.add_tbpl_output(links, tbpl_output, server, scheme)
  File "/Users/cltbld/talos-slave/talos-data/talos/output.py", line 345, in add_tbpl_output
    assert len(lines) == 2, "Expected 2 lines, got: %s" % lines
AssertionError: Expected 2 lines, got: ['tp5n_responsiveness_paint\t99.00\tgraph.html#tests=[[216,23,21]]']

In the current code, this wouldn't show up as a failure, due to the weird and very weird logic here: http://hg.mozilla.org/build/talos/file/70be2aca088e/talos/output.py#l338

In short, I have interpreted that code as "We assume data to come in as `tsvgr\tgraph.html#tests=[[224,113,14]]\ntsvgr\t2965.75\tgraph.html#tests=[[224,113,14]]\n`; However, The first line doesn't actually tell us anything, so use some weird logic to continue the loop in that case.  We want the second line."  However, I actually have no idea what graphserver is "supposed" to send back.

I think I'll modify the code to work more like the old way, but probably the correct thing to do would be to figure out what graphserver is actually supposed to be sending back and make sure that that thing is happening.

Comment 54

[Robert Helmer [:rhelmer]]

(In reply to Jeff Hammel [:jhammel] from comment #53)
> (In reply to Jeff Hammel [:jhammel] from comment #52)
> > Created attachment 701959 [details] [diff] [review]
> > WIP
> 
> Testing on try, https://tbpl.mozilla.org/?tree=Try&rev=69182ee49d8d , I see
> failures:
> 
> NOISE: Posting result 0 of 5 to
> http://graphs.mozilla.org/server/collect.cgi, attempt 0
> NOISE: Posting result 1 of 5 to
> http://graphs.mozilla.org/server/collect.cgi, attempt 0
> NOISE: Posting result 2 of 5 to
> http://graphs.mozilla.org/server/collect.cgi, attempt 0
> NOISE: Posting result 3 of 5 to
> http://graphs.mozilla.org/server/collect.cgi, attempt 0
> NOISE: Posting result 4 of 5 to
> http://graphs.mozilla.org/server/collect.cgi, attempt 0
> RETURN: <a
> href='http://graphs.mozilla.org/graph.html#tests=[[206,23,21]]'>tp5n_paint:
> 268.12</a>
> RETURN: <a
> href='http://graphs.mozilla.org/graph.html#tests=[[207,23,
> 21]]'>tp5n_pbytes_paint: 3.5GB</a>
> RETURN: <a
> href='http://graphs.mozilla.org/graph.html#tests=[[209,23,
> 21]]'>tp5n_main_rss_paint: 193.0MB</a>
> Traceback (most recent call last):
>   File "run_tests.py", line 326, in <module>
>     main()
>   File "run_tests.py", line 323, in main
>     run_tests(parser)
>   File "run_tests.py", line 299, in run_tests
>     talos_results.output(results_urls, **results_options)
>   File "/Users/cltbld/talos-slave/talos-data/talos/results.py", line 84, in
> output
>     _output.output(results, url, tbpl_output)
>   File "/Users/cltbld/talos-slave/talos-data/talos/output.py", line 65, in
> output
>     self.post(results, results_server, results_path, results_scheme,
> tbpl_output)
>   File "/Users/cltbld/talos-slave/talos-data/talos/output.py", line 319, in
> post
>     self.add_tbpl_output(links, tbpl_output, server, scheme)
>   File "/Users/cltbld/talos-slave/talos-data/talos/output.py", line 345, in
> add_tbpl_output
>     assert len(lines) == 2, "Expected 2 lines, got: %s" % lines
> AssertionError: Expected 2 lines, got:
> ['tp5n_responsiveness_paint\t99.00\tgraph.html#tests=[[216,23,21]]']
> 
> In the current code, this wouldn't show up as a failure, due to the weird
> and very weird logic here:
> http://hg.mozilla.org/build/talos/file/70be2aca088e/talos/output.py#l338
> 
> In short, I have interpreted that code as "We assume data to come in as
> `tsvgr\tgraph.html#tests=[[224,113,14]]\ntsvgr\t2965.75\tgraph.
> html#tests=[[224,113,14]]\n`; However, The first line doesn't actually tell
> us anything, so use some weird logic to continue the loop in that case.  We
> want the second line."  However, I actually have no idea what graphserver is
> "supposed" to send back.
> 
> I think I'll modify the code to work more like the old way, but probably the
> correct thing to do would be to figure out what graphserver is actually
> supposed to be sending back and make sure that that thing is happening.

Hmm so looks like graphserver does e.g.:
"""RETURN\t%s\tgraph.html#tests=[[%d,%d,%d]]""" % (metadata.test_name, metadata.test_id, metadata.branch_id, metadata.os_id)

http://hg.mozilla.org/graphs/file/8884ef9418bf/server/pyfomatic/collect.py#l272

However if you look at AMO-specific results, you can see that it explicitly prints "success" or "failure" which is nice, I wonder if we could get away with adding that to the above without breaking talos :)

http://hg.mozilla.org/graphs/file/8884ef9418bf/server/pyfomatic/collect.py#l257

Comment 55

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #54)
> (In reply to Jeff Hammel [:jhammel] from comment #53)
> > In short, I have interpreted that code as "We assume data to come in as
> > `tsvgr\tgraph.html#tests=[[224,113,14]]\ntsvgr\t2965.75\tgraph.
> > html#tests=[[224,113,14]]\n`; However, The first line doesn't actually tell
> > us anything, so use some weird logic to continue the loop in that case.  We
> > want the second line."  However, I actually have no idea what graphserver is
> > "supposed" to send back.

I think just looking for "RETURN\t" before that URL would be enough to work with the current code.

Comment 56

[Jeff Hammel]

Thanks :rhelmer, that helps a lot :)

As for why we're getting one line here, it now "makes perfect sense" in the sense that everything in Talos is less than straight-forward..  ABICT, for the 'VALUES' case, I should receive two lines (like I have observed in previous testing): one from http://hg.mozilla.org/graphs/file/8884ef9418bf/server/pyfomatic/collect.py#l274 and one from http://hg.mozilla.org/graphs/file/8884ef9418bf/server/pyfomatic/collect.py#l277 , the first sans the average and the second with.  However, we run tpn with responsiveness: http://k0s.org:8080/?show=active#tpn . This means that we send AVERAGE values: http://hg.mozilla.org/build/talos/file/70be2aca088e/talos/output.py#l249 .  So what we're actually sending to graphserver from tpn is a single number: http://hg.mozilla.org/build/talos/file/70be2aca088e/talos/output.py#l249 .

So... ;) I will:

- either better check for AVERAGE vs VALUES or, failing that, fall back on behaviour similar to what we do currently

- document this in code comments so that human beings can understand what we're doing (even if it still is a pile of spaghetti)

If we wanted to revise the graphserver format, I would be cool with that too.  TBH, the line at http://hg.mozilla.org/build/talos/file/70be2aca088e/talos/output.py#l249 I don't find helpful at all since we hit http://hg.mozilla.org/graphs/file/8884ef9418bf/server/pyfomatic/collect.py#l277 regardless.  (I'd also, of course, prefer JSON.)  But its not really important for this bug.

Comment 57

[Jeff Hammel]

Also, as implied but I'll state it explicitly for posterity, a bunch of the code for all of these special cases is, to be quite frank, a load of crap that has been forward-ported ad infinitum.  We should really clean this mess up.  I've mostly been waiting on datazilla since if bug 824814 is done most of this can be a straight-forward (well, close enough) deletion of code. That said, as-is we could do much better than now if it is worth the effort.

In general, and I won't file a bug just quite yet as I'd like to be more specific, the test specific things should be embedded in the test definitions, currently in test.py (see also: bug 770744).  That is, VALUES vs AVERAGE and "knowing" if the test is a responsiveness test or not should be explicit, not implicit as it currently is.  I think if we put the special cases close to where they should live it will make our lives much easier.

Again, this is an aside and not the best forum for discussion, but even if we don't decide to fix this before bug 824814, we should in general try to do this going forward or we will soon be in much the same boat of spaghetti code we are now (which isn't painful the 95% of the time you aren't looking on it, and very painful that 5%).

Unrelatedly, if we do change the graphserver format, we'll have to be careful to ensure that graphserver and Talos can both work simultaneously with the old format and new....

Comment 58

[Jeff Hammel]

:rhelmer: if you'd like to remove http://hg.mozilla.org/graphs/file/8884ef9418bf/server/pyfomatic/collect.py#l274 from graphserver I don't think Talos actually uses it.  I can file a separate bug if desired

Comment 59

[Jeff Hammel]

Attachment: round next

this seems to green up try: https://tbpl.mozilla.org/?tree=Try&rev=5cf19bb084a4 ; jmaher, lemme know if you want any additional testing before checkin

Comment 60

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

Comment on attachment 702964 [details] [diff] [review]
round next

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

if we are green on try, lets get this in.

Comment 61

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/d8e2300a127d

Still to do:

- deploy a new talos.zip
- fix TBPL to actually read the results

Comment 62

[Ed Morley [:emorley]]

The '"TinderboxPrint: TalosResult: {"datazilla": ' line on the try run contains entries for every single pageset. I'm presuming this wasn't intentional?

Comment 63

[Ed Morley [:emorley]]

s/pageset/page in the test run/

Comment 64

[Jeff Hammel]

Beh, certainly not.  I hate talos.

I will look into this.

Comment 65

[Jeff Hammel]

Attachment: follow up

This gives: 

NOISE: Datazilla results at https://datazilla.mozilla.org/test/summary/foobar/d8be4bc4fba8?product=Firefox&branch_version=21.0a1
TinderboxPrint: TalosResult: {"datazilla": {"tsvg": {"url": "https://datazilla.mozilla.org/test/summary/foobar/d8be4bc4fba8?product=Firefox&branch_version=21.0a1"}}}

Comment 66

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

Comment on attachment 703089 [details] [diff] [review]
follow up

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

awesome!

Comment 67

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #66)
> Comment on attachment 703089 [details] [diff] [review]
> follow up
> 
> Review of attachment 703089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> awesome!

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=74794a471859

Comment 68

[Jeff Hammel]

Green, pushed to talos: http://hg.mozilla.org/build/talos/rev/d34468dcc1f8

This gives us the following from try:

{"datazilla": {"tp5n": {"url": "https://datazilla.mozilla.org/talos/summary/Try/74794a471859?product=Firefox&branch_version=21.0a1"}}, "graphserver": {"tp5n_shutdown_paint": {"url": "http://graphs.mozilla.org/graph.html#tests=[[215,23,22]]", "result": "386.00"}, "tp5n_pbytes_paint": {"url": "http://graphs.mozilla.org/graph.html#tests=[[207,23,22]]", "result": "3.4GB"}, "tp5n_main_rss_paint": {"url": "http://graphs.mozilla.org/graph.html#tests=[[209,23,22]]", "result": "212.7MB"}, "tp5n_paint": {"url": "http://graphs.mozilla.org/graph.html#tests=[[206,23,22]]", "result": "268.72"}, "tp5n_responsiveness_paint": {"url": "http://graphs.mozilla.org/graph.html#tests=[[216,23,22]]", "result": "90.00"}}}