1186076 - adjust talosdata json blob to include summaries

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

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

right now we upload the raw replicates via the talosdata:
TALOSDATA: [{"test_machine": {"platform": "x86", "osversion": "Ubuntu 12.04", "os": "linux", "name": "talos-linux32-ix-006"}, "test_build": {"name": "Firefox", "version": "42.0a1", "id": "20150721080450", "branch": "Mozilla-Inbound-Non-PGO", "revision": "c004f0d1c60b"}, "testrun": {"date": 1437494193, "suite": "tp5o", "options": {"responsiveness": true, "tpmozafterpaint": true, "tpchrome": true, "tppagecycles": 25, "tpcycles": 1, "tprender": false, "shutdown": false, "cycles": 1, "rss": true}}, "results": {"tudou.com": [...], "uol.com.br": [...]}, "talos_counters": {"XRes": {"max": "28524509.00", "mean": "14403432.33"}, "Private Bytes": {"max": "560222208.00", "mean": "542248181.01"}, "Main_RSS": {"max": "186503168.00", "mean": "163624128.60"}}}]

what we have in the json is currently:
"results": {"page": [replicates], ...}

one thought I had was to modify it to look like this:
"results": {"summary": [page_summaries], "page": [replicates], ...}
"summary": {"summary": val, "page": val, ...}

then we can just take the ingestion for the replications and switch to data['summary']['page']

likewise for the summary series, we would have them in data['results']['summary'] and that would roll up into data['summary']['summary']

thoughts?

Comment 1

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #0)
> right now we upload the raw replicates via the talosdata:
> TALOSDATA: [{"test_machine": {"platform": "x86", "osversion": "Ubuntu
> 12.04", "os": "linux", "name": "talos-linux32-ix-006"}, "test_build":
> {"name": "Firefox", "version": "42.0a1", "id": "20150721080450", "branch":
> "Mozilla-Inbound-Non-PGO", "revision": "c004f0d1c60b"}, "testrun": {"date":
> 1437494193, "suite": "tp5o", "options": {"responsiveness": true,
> "tpmozafterpaint": true, "tpchrome": true, "tppagecycles": 25, "tpcycles":
> 1, "tprender": false, "shutdown": false, "cycles": 1, "rss": true}},
> "results": {"tudou.com": [...], "uol.com.br": [...]}, "talos_counters":
> {"XRes": {"max": "28524509.00", "mean": "14403432.33"}, "Private Bytes":
> {"max": "560222208.00", "mean": "542248181.01"}, "Main_RSS": {"max":
> "186503168.00", "mean": "163624128.60"}}}]
> 
> what we have in the json is currently:
> "results": {"page": [replicates], ...}
> 
> one thought I had was to modify it to look like this:
> "results": {"summary": [page_summaries], "page": [replicates], ...}
> "summary": {"summary": val, "page": val, ...}
> 
> then we can just take the ingestion for the replications and switch to
> data['summary']['page']
> 
> likewise for the summary series, we would have them in
> data['results']['summary'] and that would roll up into
> data['summary']['summary']
> 
> thoughts?

Sounds good to me. Maybe it should be `"summary": {"suite": val, "page": val, ...}`?

Comment 2

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

yeah, the summary for suite and page sounds good.  any concerns with the summary series needing specific data?  or do we just need the summary suite value?

Comment 3

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

Attachment: print out summaries in talosdata blob (1.0)

:parkouss, I would like your general thoughts on this as well.

here is an example of tart data from a local run:
"summary": {"newtab-open-preload-no.half.TART": 2.7654970941089445, "iconFade-open-DPI2.error.TART": 4.345000000002983, "icon-close-DPI2.all.TART": 1.904098204858323, "simple-close-DPI1.half.TART": 1.7159162351732404, "icon-close-DPI1.all.TART": 1.7452358866259106, "simple-close-DPI1.all.TART": 1.9560971486066239, "newtab-open-preload-yes.all.TART": 2.9000984633510765, "icon-open-DPI2.all.TART": 3.721956011083284, "icon-open-DPI2.half.TART": 2.6405409357764507, "iconFade-close-DPI2.half.TART": 1.8133813968253514, "simple-close-DPI1.error.TART": 23.29500000000189, "iconFade-open-DPI2.half.TART": 2.5204260776088026, "suite": 5.476306850972606, "iconFade-close-DPI2.all.TART": 1.8004974538112726, "newtab-open-preload-no.all.TART": 3.949255613841821, "icon-open-DPI1.error.TART": 33.99750000000495, "icon-close-DPI1.half.TART": 1.647813827722845, "simple-open-DPI1.error.TART": 34.395000000004075, "icon-open-DPI1.all.TART": 3.9996053225411066, "newtab-open-preload-yes.half.TART": 2.624972380983709, "icon-open-DPI2.error.TART": 32.86999999999898, "simple-open-DPI1.all.TART": 4.0397371200414804, "newtab-open-preload-yes.error.TART": 37.8424999999952, "simple-open-DPI1.half.TART": 2.8153769312281502, "newtab-open-preload-no.error.TART": 33.50500000000102, "iconFade-open-DPI2.all.TART": 2.475937725361624, "icon-open-DPI1.half.TART": 2.8459648475414365, "icon-close-DPI2.half.TART": 1.6503121238792144, "iconFade-close-DPI2.error.TART": 5.544999999998254, "icon-close-DPI1.error.TART": 24.337500000001455, "icon-close-DPI2.error.TART": 23.902499999996508}

and a full blob of a reduced tresize locally:
INFO : TALOSDATA: [{"talos_counters": {}, "results": {"tresize": [16.53371666666667, 16.992316666666667, 16.654333333333305]}, "summary": {"suite": 16.72573004574022, "tresize": 16.654333333333305}, "test_machine": {"platform": "x86_64", "osversion": "Ubuntu 14.04", "os": "linux", "name": "qm-pxp01"}, "testrun": {"date": 1438039054, "suite": "tresize", "options": {"responsiveness": false, "cycles": 3, "tpmozafterpaint": true, "shutdown": false, "rss": false}}, "test_build": {"name": "Firefox", "version": "42.0a1", "id": "20150727080509", "branch": "", "revision": "effb41253228"}}]

Comment 4

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

Attachment: a real patch that changes talosdata to include summaries (1.0)

the right patch this time.

Comment 5

[William Lachance (:wlach)]

Comment on attachment 8639812 [details] [diff] [review]
a real patch that changes talosdata to include summaries (1.0)

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

Seems like it's on the right track, there are some changes I'd like to either see or discuss.

::: talos/output.py
@@ +144,5 @@
>  
> +    @classmethod
> +    def geomean(cls, val_list):
> +        results = [i for i, j in val_list]
> +        mozlog.info("geomeatric mean benchmark")

geomeatric -> geometric

Also, not sure if this logging is particularly useful. See comment below.

@@ +152,5 @@
> +    def mean(cls, val_list):
> +        results = [i for i, j in val_list]
> +        mozlog.info("geomeatric mean benchmark")
> +        return filter.mean(results)
> +

I'm honestly not sure if having separate methods for all the different metrics really adds any value compared to just doing the calculations inline. I assume we're only using them in this one place.

If you are going to keep them, I'd shorten them into oneliners. e.g.:

@classmethod
def mean(cls, val_list):
    return filter.mean([i for i, j in val_list])

@@ +508,5 @@
>                                       for extension in test.extensions]
>          return options
>  
> +    def construct_results(self, vals, testname):
> +        average = None

average doesn't seem like right value here. probably the easiest thing is to just remove the temporary and just return the value directly

i.e.

return self.responsiveness_Metric([val for (val, page) in vals])

@@ +579,5 @@
>                              results.setdefault(page, []).extend(val)
> +
> +                suite_summary = self.construct_results(vals,
> +                                                       testname=test.name())
> +                summary['suite'] = suite_summary

I think I'd prefer a structure like this:

"summary": { 
  "suite": <suite summary value>
  "subtests: {
     "subtest1": <subtest1 summary value>,
     ...
  }
}

Inside Perfherder we're going to need to handle the suite summary seperately from the subtest summaries anyway, so I think this structure will make that a bit more clear and understandable. Also, it allows us to have subtests called "suite". :)

Comment 6

[Julien Pagès (:parkouss)]

Well, clearing the NI - Will provided a thorough review here I think. :)

Comment 7

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

Attachment: a real patch that changes talosdata to include summaries (1.1)

updated with feedback, tested locally:
INFO : TALOSDATA: [{"talos_counters": {}, "results": {"tresize": [16.49304999999997, 16.814783333333324, 16.828950000000017]}, "summary": {"suite": 16.711579102160158, "subtests": {"tresize": 16.828950000000017}}, "test_machine": {"platform": "x86_64", "osversion": "Ubuntu 14.04", "os": "linux", "name": "qm-pxp01"}, "testrun": {"date": 1438199437, "suite": "tresize", "options": {"responsiveness": false, "cycles": 3, "tpmozafterpaint": true, "shutdown": false, "rss": false}}, "test_build": {"name": "Firefox", "version": "42.0a1", "id": "20150729070337", "branch": "", "revision": "6b745227506b"}}]

and:
INFO : TALOSDATA: [{"talos_counters": {}, "results": {"newtab-open-preload-no.half.TART": [11.579244613647461, 2.723386235015337, 2.287874778111776], "iconFade-open-DPI2.error.TART": [3.514999999999418, 3.9400000000023283, 5.5900000000037835], "icon-close-DPI2.all.TART": [1.4369320483768688, 1.829973362275024, 1.8967982609142628], "simple-close-DPI1.half.TART": [1.4588609541518778, 1.774559523509099, 1.5848854094335478], "icon-close-DPI1.all.TART": [1.5917497737841173, 1.606426779740776, 1.8044541565810932], "simple-close-DPI1.all.TART": [1.5545839853347487, 1.715478240604132, 1.5885289590080063], "newtab-open-preload-yes.all.TART": [3.3353459722809977, 3.809172357338062, 3.842582429664722], "icon-open-DPI2.all.TART": [2.5704866544558453, 2.9510621658780356, 2.9339906395821087], "icon-open-DPI2.half.TART": [2.161922593911489, 2.4052372465328293, 2.462279053444558], "iconFade-close-DPI2.half.TART": [1.2718118140985677, 1.651925676209586, 1.362154483795166], "simple-close-DPI1.error.TART": [17.475000000000364, 17.06000000000131, 18.30500000000029], "iconFade-open-DPI2.half.TART": [2.4107773527503014, 2.4056976065039635, 2.2425516912570367], "newtab-open-preload-yes.half.TART": [2.2861578207389983, 2.5635032342827837, 2.530984121820201], "iconFade-close-DPI2.all.TART": [1.3164700394296376, 1.6449812633890502, 1.421768670765365], "newtab-open-preload-no.all.TART": [10.46609662771225, 4.163976004199376, 3.311995287354176], "icon-open-DPI1.error.TART": [36.47999999999956, 30.05000000000291, 29.2699999999968], "icon-close-DPI2.error.TART": [18.100000000002183, 19.029999999998836, 18.764999999999418], "icon-close-DPI1.half.TART": [1.468282721465147, 1.5378524859746296, 1.5478524271647136], "simple-open-DPI1.error.TART": [26.774999999999636, 28.8550000000032, 29.24500000000262], "icon-open-DPI1.all.TART": [2.6888812118106418, 2.762841445334414, 2.97658259721323], "icon-open-DPI2.error.TART": [37.784999999998035, 30.534999999999854, 32.455000000001746], "icon-close-DPI2.half.TART": [1.3042262712221466, 1.6950430940179264, 1.8647036667793029], "newtab-open-preload-yes.error.TART": [27.395000000000437, 33.38500000000204, 35.779999999998836], "simple-open-DPI1.half.TART": [2.1652529416260897, 2.3367353796958925, 2.193949098856944], "newtab-open-preload-no.error.TART": [32.05500000000029, 32.75, 28.810000000004948], "iconFade-open-DPI2.all.TART": [2.1997755844638034, 2.271159814399423, 2.198632388471443], "icon-open-DPI1.half.TART": [2.2126868619368625, 2.2641069843218875, 2.6936087220214135], "simple-open-DPI1.all.TART": [2.3955099852842707, 2.4640040221668427, 2.5380791498749864], "icon-close-DPI1.error.TART": [18.960000000000946, 19.43999999999869, 19.529999999998836], "iconFade-close-DPI2.error.TART": [3.095000000001164, 3.4949999999989814, 3.1450000000040745]}, "summary": {"suite": 4.84552927729184, "subtests": {"newtab-open-preload-no.half.TART": 2.5056305065635565, "iconFade-open-DPI2.error.TART": 4.765000000003056, "icon-close-DPI2.all.TART": 1.8633858115946436, "simple-close-DPI1.half.TART": 1.6797224664713233, "icon-close-DPI1.all.TART": 1.7054404681609348, "simple-close-DPI1.all.TART": 1.6520035998060691, "newtab-open-preload-yes.all.TART": 3.825877393501392, "icon-open-DPI2.all.TART": 2.9425264027300724, "icon-open-DPI2.half.TART": 2.4337581499886936, "iconFade-close-DPI2.half.TART": 1.507040080002376, "simple-close-DPI1.error.TART": 17.6825000000008, "iconFade-open-DPI2.half.TART": 2.3241246488805, "newtab-open-preload-yes.half.TART": 2.547243678051492, "iconFade-close-DPI2.all.TART": 1.5333749670772074, "newtab-open-preload-no.all.TART": 3.737985645776776, "icon-open-DPI1.error.TART": 29.659999999999854, "icon-close-DPI1.half.TART": 1.5428524565696717, "simple-open-DPI1.error.TART": 29.05000000000291, "icon-open-DPI1.all.TART": 2.869712021273822, "icon-open-DPI2.error.TART": 31.4950000000008, "simple-open-DPI1.all.TART": 2.5010415860209143, "newtab-open-preload-yes.error.TART": 34.58250000000044, "simple-open-DPI1.half.TART": 2.265342239276418, "newtab-open-preload-no.error.TART": 30.780000000002474, "iconFade-open-DPI2.all.TART": 2.234896101435433, "icon-open-DPI1.half.TART": 2.4788578531716503, "icon-close-DPI2.half.TART": 1.7798733803986146, "iconFade-close-DPI2.error.TART": 3.320000000001528, "icon-close-DPI1.error.TART": 19.484999999998763, "icon-close-DPI2.error.TART": 18.897499999999127}}, "test_machine": {"platform": "x86_64", "osversion": "Ubuntu 14.04", "os": "linux", "name": "qm-pxp01"}, "testrun": {"date": 1438199554, "suite": "tart", "options": {"responsiveness": false, "tpmozafterpaint": false, "tpchrome": true, "tppagecycles": 3, "tpcycles": 1, "tprender": false, "shutdown": false, "cycles": 1, "rss": false}}, "test_build": {"name": "Firefox", "version": "42.0a1", "id": "20150729070337", "branch": "", "revision": "6b745227506b"}}]

Comment 8

[William Lachance (:wlach)]

Comment on attachment 8640679 [details] [diff] [review]
a real patch that changes talosdata to include summaries (1.1)

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

This looks great! Please run through try to make sure things don't break (I don't think they will). Once we have this working we can start adjusting perfherder to use this. The sooner the better, I suppose, as this change will need to ride the trains and we probably want to maintain backwards compatibility.

Comment 9

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

landed:
http://hg.mozilla.org/build/talos/rev/43a0112be4ed

testing on try in detail and will work on landing.

Comment 10

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

Attachment: resolve custom metrics and measurements for subtests (1.0)

Comment 11

[William Lachance (:wlach)]

Comment on attachment 8644929 [details] [diff] [review]
resolve custom metrics and measurements for subtests (1.0)

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

In general I think this is ok. Be sure to give it lots of testing. :)

Let's file a followup to improve how we handle filters in the future.

::: talos/results.py
@@ +156,5 @@
>              page = result['page']
>              data = result['runs']
> +            data_summary = {}
> +            remaining_filters = []
> +

I agree that it would be nice to clean this stuff up a bit -- right now there's a lot of special casing which is rather confusing.

@@ +169,5 @@
> +            data_summary['min'] = min(data)
> +            data_summary['max'] = max(data)
> +            data_summary['mean'] = filter.mean(data)
> +            data_summary['median'] = filter.median(data)
> +            data_summary['std'] = filter.stddev(data)

I would just define data_summary here as:

data_summary = {
  'min': min(data),
  ...
}

and remove the definition above.

Comment 12

[William Lachance (:wlach)]

Comment on attachment 8644929 [details] [diff] [review]
resolve custom metrics and measurements for subtests (1.0)

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

::: talos/results.py
@@ +178,5 @@
> +                    # for v8_subtest we need to page for reference data
> +                    data = filter.v8_subtest(data, page)
> +                else:
> +                    data = f.apply(data)
> +            data_summary['filtered'] = data

One additional comment: I think we should use the term "value" instead of "filtered" here. "filtered" is internal nomenclature, "value" more strongly indicates what we're doing (providing an arbitrary value which we think is representative of the subtest suite)

Comment 13

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

per irc conversation we agreed that 'value' would be sent to perfherder if and only if there was a custom calculation for the subtest.  filtered is the value that we use for graph server, and all the other tests use median (and kraken uses mean).

Comment 14

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

https://hg.mozilla.org/build/talos/rev/2181cef265e7

Comment 15

[Julien Pagès (:parkouss)]

Comment on attachment 8644929 [details] [diff] [review]
resolve custom metrics and measurements for subtests (1.0)

Thanks for asking for feedback!

From a code-only point of view, I would be really happy with a follow up bug as proposed by Will to avoid doing strange things with the filters. I feel like this is an abuse of what it was designed for, so it needs to be redesigned.

That being said, it is great - as far as I can understand that work will help for generating good talos alerts by getting more data out of the tests and this is good news!