Bug 108577 – Upload results to perf.webkit.org in addition to the one specified by --test-results-server

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 185938 [details]
work in progress

[Ryosuke Niwa]

Created attachment 186127 [details]
Work in progress 2

[Ryosuke Niwa]

Created attachment 187337 [details]
Work in progress 3

[Ryosuke Niwa]

Created attachment 187412 [details]
Work in progress 4

[Ryosuke Niwa]

Created attachment 189670 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

I'd prefer you ask dpranke or ojan for a review.  I think they probably know this section of webkitpy better than I do.

[Ryosuke Niwa]

(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.

[Dirk Pranke]

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 ?

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 189843 [details]
Fixed per comments

[WebKit Review Bot]

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.

[Dirk Pranke]

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)

[Dirk Pranke]

Also, can you make sure that check-webkit-style (and/or lint-webkitpy) is clean, since the style bot seems to be choking?

[Ryosuke Niwa]

Created attachment 189848 [details]
Patch for landing

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Comment on attachment 189848 [details]
Patch for landing

Clearing flags on attachment: 189848

Committed r143833: <http://trac.webkit.org/changeset/143833>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.