Bug 108577

Summary: Upload results to perf.webkit.org in addition to the one specified by --test-results-server
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Tools / TestsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, barraclough, dpranke, eric, mjs, mrowe, ojan, pdr, slewis, webkit.review.bot, wsiegrist
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77037    
Attachments:
Description Flags
work in progress
none
Work in progress 2
none
Work in progress 3
none
Work in progress 4
none
Patch
none
Fixed per comments
none
Patch for landing none

Description Ryosuke Niwa 2013-01-31 21:35:22 PST
We will be launching new performance monitoring on perf.webkit.org soon.

Since we would like to pre-populate new monitoring app with useful data, make run-perf-tests submit results to perf.webkit.org in addition to webkit-perf.appspot.com specified by buildbot.
While we could be modifying buildbot configuration to specify both, I'm going to just add a hack in run-perf-tests since this is only a temporary solution during the transition.
Once we've completed the transition, we'll stop uploading results to webkit-perf.appspot.com and simply specify perf.webkit.org on --test-results-server.
Comment 1 Ryosuke Niwa 2013-01-31 21:37:38 PST
Created attachment 185938 [details]
work in progress
Comment 2 Ryosuke Niwa 2013-02-01 13:46:19 PST
Created attachment 186127 [details]
Work in progress 2
Comment 3 Ryosuke Niwa 2013-02-08 10:39:39 PST
Created attachment 187337 [details]
Work in progress 3
Comment 4 Ryosuke Niwa 2013-02-08 21:55:02 PST
Created attachment 187412 [details]
Work in progress 4
Comment 5 Ryosuke Niwa 2013-02-21 20:43:59 PST
Created attachment 189670 [details]
Patch
Comment 6 WebKit Review Bot 2013-02-21 20:45:26 PST
Attachment 189670 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/common/checkout/scm/git.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py', u'Tools/Scripts/webkitpy/common/checkout/scm/svn.py', u'Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py', u'Tools/Scripts/webkitpy/layout_tests/port/base.py', u'Tools/Scripts/webkitpy/layout_tests/port/chromium.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner_integrationtest.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py']" exit_code: 1
Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py:167:  [MainTest.test_upload_json.MockFileUploader.__init__] Method should have "self" as first argument  [pylint/E0213] [5]
Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py:172:  [MainTest.test_upload_json.MockFileUploader.upload_single_text_file] Method should have "self" as first argument  [pylint/E0213] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Eric Seidel (no email) 2013-02-22 14:07:04 PST
Comment on attachment 189670 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189670&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:367
> -        repos.append(('chromium', self.path_from_chromium_base('build')))
> +        repos.append(('Chromium', self.path_from_chromium_base('build')))

These are paths?  Are you sure you wnat to capitalize here?

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:281
> +                url = 'http://trac.webkit.org/browser/trunk/PerformanceTests/' + '/'.join(path[0:i + 1])

You might separate this out into another function.  I think common.config.urls already has this anyway. :)
Comment 8 Eric Seidel (no email) 2013-02-22 14:07:27 PST
I'd prefer you ask dpranke or ojan for a review.  I think they probably know this section of webkitpy better than I do.
Comment 9 Ryosuke Niwa 2013-02-22 14:31:14 PST
(In reply to comment #7)
> (From update of attachment 189670 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189670&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium.py:367
> > -        repos.append(('chromium', self.path_from_chromium_base('build')))
> > +        repos.append(('Chromium', self.path_from_chromium_base('build')))
> 
> These are paths?  Are you sure you wnat to capitalize here?

No, they're repository names.
Comment 10 Dirk Pranke 2013-02-22 14:54:56 PST
Comment on attachment 189670 [details]
Patch

Mostly minor comments ... I'd like to review it again briefly once you can get things cleaned up?

View in context: https://bugs.webkit.org/attachment.cgi?id=189670&action=review

> Tools/ChangeLog:12
> +        a svn checkout or a git clone. This information is embedded in JSON submitted to perf.webkit.org.

I saw there was some discussion of this on #irc, but can you add a bit more as to *why* you need/want the timestamp?

Also, reflecting what we discussed on #webkit, can you put in something about why we need to support two formats, and what the transition plan is? this patch makes things pretty ugly, so it would be good to clarify that this is hopefully temporary.

> Tools/ChangeLog:16
> +        of this feature has been updated to explicitly lowercase the names.

Is this actually needed as a part of this change? AFAIK, these repo names aren't really user-visible and this kinda seems like busy work. I'm not saying you should remove it, but I wouldn't have made the changes unless they were actually needed, especially since you're having to work-around the changes to keep from breaking clients.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:257
> +    def svn_timestamp(self, path):

Why is this "svn_timestamp" rather than just "timestamp" or "timestamp_of_latest_commit"?

Why do you need '-25' rather than just '-1' ? It doesn't look like this is actually filtering for git-svn-id: or anything to find the svn-specific revisions?  

Do you need to do anything here to ensure you're not pulling timestamps off a branch?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:1100
> +        By default it returns a list that only contains a ('WebKit', <webkitRepossitoryPath>) tuple."""

typo: webkitRepossitory . I know you didn't add it, but might as well fix it while you're here :).

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:367
>> +        repos.append(('Chromium', self.path_from_chromium_base('build')))
> 
> These are paths?  Are you sure you wnat to capitalize here?

The first arg is just an identifier, not a path.

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:208
>      def _generate_and_show_results(self):

this function is too long and doing too much now ... can you add some FIXMEs to break it up to be easier to follow (and test)?

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:265
> +            result = self._results[metric_full_name]

nit: for metric_full_name, result in self._results.iteritems(): ?

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:267
> +            # We can't reprot results without indivisual measurements.

typos.

> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:327
> +    def _generate_output_files(self, output_json_path, perf_webkit_json_path, results_page_path, output, perf_webkit_output):

seems like here you could just call _generate_output_files twice, once with the old output, and then the second time w/ the new output and results_page_path=None ?
Comment 11 Ryosuke Niwa 2013-02-22 15:11:19 PST
(In reply to comment #10)
>
> > Tools/ChangeLog:12
> > +        a svn checkout or a git clone. This information is embedded in JSON submitted to perf.webkit.org.
> 
> I saw there was some discussion of this on #irc, but can you add a bit more as to *why* you need/want the timestamp?
> 
> Also, reflecting what we discussed on #webkit, can you put in something about why we need to support two formats, and what the transition plan is? this patch makes things pretty ugly, so it would be good to clarify that this is hopefully temporary.

Done.
 
> > Tools/ChangeLog:16
> > +        of this feature has been updated to explicitly lowercase the names.
> 
> Is this actually needed as a part of this change? AFAIK, these repo names aren't really user-visible and this kinda seems like busy work. I'm not saying you should remove it, but I wouldn't have made the changes unless they were actually needed, especially since you're having to work-around the changes to keep from breaking clients.

Yes. These names are recorded and exposed on perf.webkit.org as a part of its UI.

> > Tools/Scripts/webkitpy/common/checkout/scm/git.py:257
> > +    def svn_timestamp(self, path):
> 
> Why is this "svn_timestamp" rather than just "timestamp" or "timestamp_of_latest_commit"?

Renamed.

> Why do you need '-25' rather than just '-1' ? It doesn't look like this is actually filtering for git-svn-id: or anything to find the svn-specific revisions?  

Oops, changed it to -1.

> Do you need to do anything here to ensure you're not pulling timestamps off a branch?

I don't really care what happens on beaches at least for now since none of our bots use branches.

> > Tools/Scripts/webkitpy/layout_tests/port/base.py:1100
> > +        By default it returns a list that only contains a ('WebKit', <webkitRepossitoryPath>) tuple."""
> 
> typo: webkitRepossitory . I know you didn't add it, but might as well fix it while you're here :).

Fixed.

> > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:208
> >      def _generate_and_show_results(self):
> 
> this function is too long and doing too much now ... can you add some FIXMEs to break it up to be easier to follow (and test)?

Added.

> > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:265
> > +            result = self._results[metric_full_name]
> 
> nit: for metric_full_name, result in self._results.iteritems(): ?

Fixed.

> > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:267
> > +            # We can't reprot results without indivisual measurements.
> 
> typos.

Fixed.

> > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:327
> > +    def _generate_output_files(self, output_json_path, perf_webkit_json_path, results_page_path, output, perf_webkit_output):
> 
> seems like here you could just call _generate_output_files twice, once with the old output, and then the second time w/ the new output and results_page_path=None ?

No. results page should only be generated with the old JSON format for now. I'll update the results page's template to use the new format once this patch is in.
Comment 12 Ryosuke Niwa 2013-02-22 15:14:07 PST
Created attachment 189843 [details]
Fixed per comments
Comment 13 WebKit Review Bot 2013-02-22 15:16:24 PST
Attachment 189843 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/common/checkout/scm/git.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py', u'Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py', u'Tools/Scripts/webkitpy/common/checkout/scm/svn.py', u'Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py', u'Tools/Scripts/webkitpy/layout_tests/port/base.py', u'Tools/Scripts/webkitpy/layout_tests/port/chromium.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner_integrationtest.py', u'Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py']" exit_code: 1
Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py:167:  [MainTest.test_upload_json.MockFileUploader.__init__] Method should have "self" as first argument  [pylint/E0213] [5]
Tools/Scripts/webkitpy/performance_tests/perftestsrunner_unittest.py:172:  [MainTest.test_upload_json.MockFileUploader.upload_single_text_file] Method should have "self" as first argument  [pylint/E0213] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Dirk Pranke 2013-02-22 15:18:26 PST
Comment on attachment 189843 [details]
Fixed per comments

View in context: https://bugs.webkit.org/attachment.cgi?id=189843&action=review

r+ with the typos fixed.

> Tools/ChangeLog:9
> +        --test-results-server. The new format is needed to provide of extra information perf.webkit.org

nit: s/provide of/provide/

> Tools/ChangeLog:12
> +        webkit-perf.appspot.com will be deleted once the transition is completed.

webkit-perf.appspot.com can be deleted.

> Tools/ChangeLog:14
> +        This patch adds scm.svn_timestamp to obtain the timestamp of the latest commit present in

rename to timestamp_of_latest_commit() in the Changelog?

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:263
> +        # Manually modify the timezone since Git doesn't have an optiton to show it in UTC.

typo: "optiton"

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:270
> +        return time_wihout_timezone.strftime('%Y-%m-%dT%H:%M:%SZ')

typo: "wihout" (both lines)
Comment 15 Dirk Pranke 2013-02-22 15:19:33 PST
Also, can you make sure that check-webkit-style (and/or lint-webkitpy) is clean, since the style bot seems to be choking?
Comment 16 Ryosuke Niwa 2013-02-22 15:29:50 PST
Created attachment 189848 [details]
Patch for landing
Comment 17 Ryosuke Niwa 2013-02-22 15:37:06 PST
(In reply to comment #16)
> Created an attachment (id=189848) [details]
> Patch for landing

I'm going to land this patch manually. I need to prepare both perf.webkit.org and webkit-perf.appsot.com for this patch so please don't land this patch until I do tonight.
Comment 18 Ryosuke Niwa 2013-02-22 20:30:25 PST
Comment on attachment 189848 [details]
Patch for landing

Clearing flags on attachment: 189848

Committed r143833: <http://trac.webkit.org/changeset/143833>
Comment 19 Ryosuke Niwa 2013-02-22 20:30:32 PST
All reviewed patches have been landed.  Closing bug.