Closed Bug 1241644 Opened 8 years ago Closed 8 years ago

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

Categories

(Testing :: Talos, defect)

Product:
Testing
Component:
Talos
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: jmaher, Assigned: rwood)

References

Details

(Whiteboard: [talos_wishlist])

Attachments

(1 file, 2 obsolete files)

Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;
58 bytes, text/x-review-board-request
wlach
: review+
Details 
to avoid posting bad data, we need to not post results to graphserver and perfherder, just finish the job.
Hi Joel, could you please elaborate what is needed for this project so we can have a contributor look at this? Thanks!
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.
Blocks: 1088251
Whiteboard: [talos_wishlist]
Assignee: nobody → rwood
Status: NEW → ASSIGNED
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
Attachment #8749372 - Flags: review?(jmaher)
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
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 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!
Attachment #8749372 - Flags: review+
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
Attachment #8750967 - Flags: review?(jmaher) → review+
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
oh crap, I forgot about that- maybe we can have an empty PERFHERDER_DATA: {} structure?
Flags: needinfo?(wlachance)
(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.
Flags: needinfo?(wlachance)
would we then hack the mozharness script to look for spsprofile and then not expect perfherder_data?
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 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/
https://reviewboard.mozilla.org/r/51717/#review50948

thanks for the updated code, this looks like it should ignore the failure with spsprofile enabled.
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/
(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
Flags: needinfo?(wlachance)
(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!
Flags: needinfo?(wlachance)
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!
Flags: needinfo?(wlachance)
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?
Flags: needinfo?(wlachance)
Attachment #8756979 - Flags: review?(wlachance) → review+
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 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 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 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/
Depends on: 1276915
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/
Attachment #8749372 - Attachment description: MozReview Request: Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder; r?jmaher → Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;
Attachment #8750967 - Attachment description: MozReview Request: Bug 1241644 - when using --spsProfile, do not upload data to graph server; r?jmaher → Bug 1241644 - when using --spsProfile, do not upload data to graph server;
Attachment #8756979 - Attachment description: MozReview Request: Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder; r?wlach → Bug 1241644 - when using --spsProfile, do not upload data to graph server/perfherder;
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 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/
Confirmed on try that PERFHERDER_DATA is not printed when spsProfiling is enabled:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecd17ede248c
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 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/
Attachment #8749372 - Attachment is obsolete: true
Attachment #8750967 - Attachment is obsolete: true
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 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/
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc819d1feda
when using --spsProfile, do not upload data to graph server/perfherder; r=wlach
https://hg.mozilla.org/mozilla-central/rev/2bc819d1feda
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: