1241644 - when using --spsProfile, do not upload data to graph server/perfherder

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

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

to avoid posting bad data, we need to not post results to graphserver and perfherder, just finish the job.

Comment 1

[Armen [:armenzg]]

Hi Joel, could you please elaborate what is needed for this project so we can have a contributor look at this? Thanks!

Comment 2

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

this would be a talos specific bug to work on, when we run talos tests we print out specific data to the log file, specifically PERFHERDER_DATA, then we also post data to graph server.  When we run with the --spsProfile option, we do not want data going to any of the official servers, even on try branches as it could confuse developers about a false regression.

talos code is here;
https://dxr.mozilla.org/mozilla-central/search?q=path%3Atesting%2Ftalos%2Ftalos&redirect=true&case=false

I would view this similar to the --develop flag, that is something which prevents posting data at the end.

Comment 3

[Robert Wood [:rwood]]

Attachment: Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review commit: https://reviewboard.mozilla.org/r/50909/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50909/

Comment 4

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

Comment on attachment 8749372 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

https://reviewboard.mozilla.org/r/50909/#review47583

one line I don't understand

::: testing/talos/talos/run_tests.py:164
(Diff revision 1)
>  
>      # results container
>      talos_results = TalosResults()
>  
>      # results links
> -    if not browser_config['develop']:
> +    if not browser_config['develop'] and not config['sps_profile']:

I don't understand this here

Comment 5

[Robert Wood [:rwood]]

https://reviewboard.mozilla.org/r/50909/#review47583

> I don't understand this here

The intention here is to only submit results online when not in --develop mode and not profiling; if either of those flags is on then only record results locally

Comment 6

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

there are a few cases where we don't want to upload:
1) --develop
2) --spsProfile (can be on try without --develop)
3) --develop and --spsProfile

and 1 case where we do want to upload:
1) no --develop, no --spsProfile

I think your case support what I want!

Comment 7

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

Comment on attachment 8749372 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

https://reviewboard.mozilla.org/r/50909/#review47785

all good!

Comment 8

[Robert Wood [:rwood]]

Attachment: Bug 1241644 - when using --spsProfile, do not upload data to graph server;

Review commit: https://reviewboard.mozilla.org/r/51717/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51717/

Comment 9

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

Comment on attachment 8750967 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server;

https://reviewboard.mozilla.org/r/51717/#review48759

nice and simple

Comment 10

[Robert Wood [:rwood]]

Now running with profiling on, causes an error on try:

PERFHERDER_DATA was seen 0 times, expected 1.
# TBPL WARNING # 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e020f7016cdd&selectedJob=20751031

Comment 11

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

oh crap, I forgot about that- maybe we can have an empty PERFHERDER_DATA: {} structure?

Comment 12

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #11)
> oh ****, I forgot about that- maybe we can have an empty PERFHERDER_DATA: {}
> structure?

No, that'll violate the perfherder schema and generate an error. A few options occur to me:

1. Make the talos output parser intelligent enough to know about this case. http://mxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/talos.py#370
2. Add an "ignore" option to the perfherder schema and do not actually ingest the data if it is set to "true".

I think (1) is probably better, if we can make it happen. It seems weird to put an artifact in the log if we're not actually going to use it.

Comment 13

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

would we then hack the mozharness script to look for spsprofile and then not expect perfherder_data?

Comment 14

[Robert Wood [:rwood]]

Comment on attachment 8749372 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50909/diff/1-2/

Comment 15

[Robert Wood [:rwood]]

Comment on attachment 8750967 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51717/diff/1-2/

Comment 16

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

https://reviewboard.mozilla.org/r/51717/#review50948

thanks for the updated code, this looks like it should ignore the failure with spsprofile enabled.

Comment 17

[Robert Wood [:rwood]]

Comment on attachment 8750967 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51717/diff/2-3/

Comment 18

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

https://reviewboard.mozilla.org/r/51717/#review51170

I like this much better.

Comment 19

[Robert Wood [:rwood]]

(In reply to Joel Maher (:jmaher) from comment #18)
> https://reviewboard.mozilla.org/r/51717/#review51170
> 
> I like this much better.

Thanks :jmaher

:wlach, do I get your stamp of approval for this?

https://reviewboard.mozilla.org/r/51717/#review51170

Comment 20

[William Lachance (:wlach)]

(In reply to Robert Wood [:rwood] from comment #19)
> (In reply to Joel Maher (:jmaher) from comment #18)
> > https://reviewboard.mozilla.org/r/51717/#review51170
> > 
> > I like this much better.
> 
> Thanks :jmaher
> 
> :wlach, do I get your stamp of approval for this?
> 
> https://reviewboard.mozilla.org/r/51717/#review51170

lol yup, looks great!

Comment 21

[Robert Wood [:rwood]]

So running on try with --spsProfile works fine, PERFHERDER_DATA is not output and is no longer expected:

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

However running on try without profiling turned on is now failing, with "PERFHERDER_DATA was seen 0 times, expected 1.":

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

:wlach, is there something I have missed here (must be)? If you have a minute to have a look that'd be great, thanks!

Comment 22

[William Lachance (:wlach)]

https://reviewboard.mozilla.org/r/51717/#review51610

::: testing/talos/talos/output.py:141
(Diff revision 3)
> -        LOG.info("PERFHERDER_DATA: %s" % json.dumps(results))
>          if results_scheme in ('file'):
>              json.dump(results, file(results_path, 'w'), indent=2,
>                        sort_keys=True)
> +        else:
> +            LOG.info("PERFHERDER_DATA: %s" % json.dumps(results))

Sorry I missed this earlier (it was an earlier change that was incorrect, and I didn't look at it because of reviewboard), it looks like you're unconditionally NOT printing PERFHERDER_DATA, which isn't going to work (since perfherder relies on this to generate numbers). 

You need to pass some kind of option to Talos to tell it not to output PERFHERDER_DATA *if and only if* sps profiling is enabled (or some equivalent)... looking at the bug history it looks like you might have actually done this already but it somehow got lost?

Comment 23

[Robert Wood [:rwood]]

Attachment: Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review commit: https://reviewboard.mozilla.org/r/55512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55512/

Comment 24

[William Lachance (:wlach)]

Comment on attachment 8756979 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

https://reviewboard.mozilla.org/r/55512/#review52288

This looks fine, you should make sure that PERFHERDER_DATA doesn't get printed when sps profiling is enabled on try (the actual purpose of this change) before landing. :P

Comment 25

[Robert Wood [:rwood]]

Comment on attachment 8749372 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50909/diff/2-3/

Comment 26

[Robert Wood [:rwood]]

Comment on attachment 8750967 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51717/diff/3-4/

Comment 27

[Robert Wood [:rwood]]

Comment on attachment 8756979 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55512/diff/1-2/

Comment 28

[Robert Wood [:rwood]]

Comment on attachment 8749372 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50909/diff/3-4/

Comment 29

[Robert Wood [:rwood]]

Comment on attachment 8750967 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51717/diff/4-5/

Comment 30

[Robert Wood [:rwood]]

Comment on attachment 8756979 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55512/diff/2-3/

Comment 31

[Robert Wood [:rwood]]

Confirmed on try that PERFHERDER_DATA is not printed when spsProfiling is enabled:

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

Comment 32

[Robert Wood [:rwood]]

Also confirmed on try that when spsProfiling is NOT enabled, PERFHERDER_DATA IS still printed:

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

Already have the r+ so going to land.

Comment 33

[Robert Wood [:rwood]]

Comment on attachment 8756979 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55512/diff/3-4/

Comment 34

[Robert Wood [:rwood]]

Squashed commits, landed on try again:

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

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

Comment 35

[Robert Wood [:rwood]]

Comment on attachment 8756979 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55512/diff/4-5/

Comment 36

[Robert Wood [:rwood]]

Comment on attachment 8756979 [details]
Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55512/diff/5-6/

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc819d1feda
when using --spsProfile, do not upload data to graph server/perfherder; r=wlach

Comment 38

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2bc819d1feda