Cleanup of page_measurement_results object

Cleanup of page_measurement_results object

This patch introduces telemetry.value, which factors out the different kinds
of data collected by telemetry benchmarks, currently scalar values, lists of scalar values, and histograms. This factoring provides mechanisms for combining together values --- value summarization, which was previously the job of the results objects, is now part of the value system.

The actual interface between the page_measurement_results object and the
rest of telemetry is unchanged. A values_backcompat module exists to
convert the old interface to the new interface. A followup patch can remove
this backcompat path. 

BUG=306944

[nduca, 2013-10-16 08:41:50]

Very WIP. The key concept i'm playing around with here is there being a Value
class hiearchy that restates the specific handling of the different perf result
options.

I have shimmed the old Add() interface to do create the new types. This will go
away eventually.

Right now, Values() dont have page, unit, importance until they're added to the
results object. I think i may switch this to be more direct:
   results.Add(ScalarValue('foo', 'ms', 33, important=False))
   results.AddSummary(ScalarValue('foo', 'ms', 33, important=False))
Or possibly with the page also tied up:
   results.Add(ScalarPageSpecificResult(page, 'foo', 'ms', 33, important=False))
   results.Add(ScalarResult('foo', 'ms', 33, important=False))

Problably will rename Value->Results.

Not at a stage where nits can really be listened to. This is all very hacky
still. Big structrual feedback is really all its ready for.

I want to move most of the 'histogram' specific handling into the Value type.
Aim is for buildbot results object will be type agnostic by the time I'm done
with it.

Some warts around how PrintPerf wants chart name vs trace name. Will figure out
how to fix.

</braindump> have fun :)

[nduca, 2013-10-21 09:31:59]

Ready for review. Wow that was a painful slog.

[qyearsley, 2013-10-21 16:15:08]

On 2013/10/21 09:31:59, nduca wrote:
> Ready for review. Wow that was a painful slog.

Excellent! I'll also review! (Although today I'm learning the perf sheriff
routine, so I'll probably get around to reviewing most of it by tomorrow or
Wednesday.)

Initial note: values.py should probably be called value.py, since it contains
the class Value, and subclasses called  *Value.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page/values.py (right):

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:83: """
Docstring formatting nit:

In the example in the style guide
(http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Commen...):
 - The ending triple-quote goes on its own line, with no blank line above it.
Docstrings in this file are inconsistent.
 - The summary line ideally should be less than 1 line ("A doc string should be
organized as a summary line (one physical line)...")

[qyearsley, 2013-10-23 01:06:55]

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page/values.py (right):

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:9: def _Mean(l):
Since this is a function, it should be moved to after the global constants. It
could go at the bottom with the other module-level functions. Also, lower-case l
is a slightly iffy variable name (looks like numeral 1).

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:13: PICK_FIRST = 'pick-first'
Comment suggestion: Note that these are the two possible choices for "same page
merging policy".

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:17: SUMMARY_RESULT_OUTPUT_INTENT =
'summary-result-output-intent'
The term "result output intent" isn't very clear when I first read this. (I
suspect that it will become clearer when I look at the rest of the related code,
but if possible, a comment here would be good.)

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:21: """telemetry.page.values and its
associated Value hierarchy create
This summary looks like it's for a module-level docstring. The docstring for
class Value should just explain that Value is a base class which represents one
result value that can be added to a PageMeasurementResults object.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:41: """A generic Value object.
The class Value is just an abstract base class, it shouldn't actually be
instantiated right?

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:43: Note: Page may be optional"""
When would page be optional? Or, when might the page attribute be None?

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:49: def IsEventuallyMergableWith(self,
that):
What does this check? Does the term "eventually" imply that there may be some
time delay before merging?

Why isn't it called IsMergableWith?

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:54: def
GetKeyForMergingBySamePage(self):
What is this "key" used for? Is it a key into some dict, and if so, what dict?

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:61: def
MergeLikeValuesFromSamePage(cls, values):
Is the argument 'values' here a list of Value objects? Is the return value a
Value object?

Could add "Args:" and "Returns:" sections to your docstrings.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:94: def GetBuildbotDataType(self,
output_intent):
Comment suggestion: Note that this returns a string describing a "type" of
result, like "default", "histogram", or "important". But what does this have to
do with buildbot again?

Also, what does output_intent represent?

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:99: """Returns the buildbot's
equivalent value."""
What's meant by buildbot's "equivalent" data_type or value?

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:115: def
GetRepresentativeScalarValue(self):
Name suggestion: What about ToScalar (or ToNumber)?

The current name is long and suggests that it would return a ScalarValue
instance, when in fact it returns a number.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:120: def
GetRepresentativeStringValue(self):
Same naming suggestion as above -- this could be called ToString.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:125: class ScalarValue(Value):
The term "scalar" here just means single number (as opposed to list of numbers),
right?

It'd be good to have a class docstring noting that it represents a single
numerical value (with associated units etc).

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:148: # print_perf_results likes to get
lists only.
What (and where) is print_perf_results?

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:169: v0 = values[0]
Not a very meaningful variable name. Might as well just use values[0] below.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:176: class ListOfScalarValues(Value):
Possible alternative name: ScalarValueList.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:202: mp)
Minor nit: I'd prefer the name merge_policy over mp, even if it's just a little
temporary variable with limited range.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:312: # produced is stable acrsos
different systems.
Spelling: across.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:443: """Mangles a value name plus
optional chart name into a standard string.
Maybe "combines" rather than "mangles"... :-P

[nduca, 2013-10-23 07:08:55]

dtu, tonyg any comments before i go fixing nits?

[qyearsley, 2013-10-26 19:05:40]

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page/values.py (right):

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:399: def
_MergeLikeValuesImpl(all_values, key_func, merge_func):
1. Curious: What exactly does the "impl" signify here? I'm not very aware of
naming conventions e.g. in C++ code.

2. Docstring would be helpful here to explain the arguments.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:409: # that the returned array is in a
stable order, group by grouop.
Spelling

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values.py:443: """Mangles a value name plus
optional chart name into a standard string.
I think the -FromTraceAndChartName part of the name is redundant, since the two
arguments are called trace_name and chart_name.

Also, the first line in the docstring is confusing since this method is supposed
to return a "value name", not combine a "value name" with something else.

Really, this whole business with "chart name" and "trace name" being combined
into "value name" (previously called "measurement name" in
PageMeasurementResults) is a little bit confusing. Especially since, if I
understand right, they're split up in the buildbot results output, printed as
"RESULT chart_name: trace_name= value unit", then combined again before being
sent to the dashboard, in //tools/build/scripts/slave/results_dashboard.py.

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page/values_unittest.py (right):

https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/values_unittest.py:26: class
MergeValueTest(TestBase):
These tests look fairly good to me.

On the matter of naming: Some of the other tests in Telemetry use the
testCamelCaseFunctionName format, since functions in general are supposed to be
AllCapsIncludingFirstWord according to the Chromium Python style guide, but the
python unittest module requires test functions to start with lower-case
"test"...

Either way, Chromium style guide isn't super clear on test function naming, but
testCamelCase might fit in better with the non-test modules.

[nduca, 2013-10-26 21:35:04]

@qyearsly, I've done a pass at the nits you provided. I had a few disagreements
but most I took into account...

> tools/telemetry/telemetry/page/values.py:99: """Returns the buildbot's
> equivalent value."""
> What's meant by buildbot's "equivalent" data_type or value?

Telemetry uses the word "buildbot" to refer to the PrintPerfResults ecosystem in
build/. It isn't well named, so we use that monikker. Thus, buildbot data type
referes to data types consumed by that system, etc. With respect, I think
changing that naming convention is out of scope of this patch. The primary
accomplishment of this patch is to make visible the somewhat-lacking quality of
that downstream system.

> GetRepresentativeStringValue(self):
> Same naming suggestion as above -- this could be called ToString.
Long names are not bad. Autocomplete is our friend. ToString could be conflated
with __str__, which isn't the intent. This is specific to the value hierarchy. I
am leaving this as is, although I did update the Scalar one to be Number to deal
with the valid ambiguous return type concern you raised.

> tools/telemetry/telemetry/page/values.py:169: v0 = values[0]
> Not a very meaningful variable name. Might as well just use values[0] below.
The code is more concise with the temp var. I left it as is.

>
https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
> tools/telemetry/telemetry/page/values.py:176: class ListOfScalarValues(Value):
> Possible alternative name: ScalarValueList.
Shrug. Bikeshed.

> tools/telemetry/telemetry/page/values.py:443: """Mangles a value name plus
> optional chart name into a standard string.
> Maybe "combines" rather than "mangles"... :-P

Bikeshed. http://en.wikipedia.org/wiki/Name_mangling


> 1. Curious: What exactly does the "impl" signify here? I'm not very aware of
> naming conventions e.g. in C++ code.

I used Common as a suffix to improve clarity.

>
https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
> tools/telemetry/telemetry/page/values.py:443: """Mangles a value name plus
> optional chart name into a standard string.
> I think the -FromTraceAndChartName part of the name is redundant, since the
two
> arguments are called trace_name and chart_name.
You don't see the argument names when you call it, remember? ValueName("boo",
"blah") which is the chart name and which is the trace name? Yes we could beg
people
to say ValueName(chart_name="boo", trace_name="blah") but python doesn't have a
way to force kwarg-passing as near as I can tell... This is my solution to that
issue.


> Also, the first line in the docstring is confusing since this method is
supposed
> to return a "value name", not combine a "value name" with something else.
This was a typo.

> Really, this whole business with "chart name" and "trace name" being combined
> into "value name" (previously called "measurement name" in
> PageMeasurementResults) is a little bit confusing. Especially since, if I
> understand right, they're split up in the buildbot results output, printed as
> "RESULT chart_name: trace_name= value unit", then combined again before being
> sent to the dashboard, in //tools/build/scripts/slave/results_dashboard.py.

Again, this is more an issue with the build scripts system. This patch makes the
messiness of the build system more visible. This was previously scattered in
many tight loops of the buildbot results object, and masked. Fixing that is
beyond the scope of this work, but emphatically definitely worth doing. I hope
that there will be cleanup patches landed after this that push the value concept
further down, and stop us doing this insane context-specific toStringing in the
intermediate stages. But thats not this patch.

[qyearsley, 2013-10-26 22:29:39]

On 2013/10/26 21:35:04, nduca wrote:
> > tools/telemetry/telemetry/page/values.py:99: """Returns the buildbot's
> > equivalent value."""
> > What's meant by buildbot's "equivalent" data_type or value?
> 
> Telemetry uses the word "buildbot" to refer to the PrintPerfResults ecosystem
in
> build/. It isn't well named, so we use that monikker. Thus, buildbot data type
> referes to data types consumed by that system, etc. With respect, I think
> changing that naming convention is out of scope of this patch. The primary
> accomplishment of this patch is to make visible the somewhat-lacking quality
of
> that downstream system.
> 

Yeah. I think was mainly confused about the term 'equivalent'. But one can see
by
looking below that it returns one of 'default', 'histogram', 'unimportant', or
'unimportant-histogram'.


>
https://codereview.chromium.org/27486002/diff/4001/tools/telemetry/telemetry/...
> > tools/telemetry/telemetry/page/values.py:176: class
ListOfScalarValues(Value):
> > Possible alternative name: ScalarValueList.
> Shrug. Bikeshed.

First time I've heard this term. Interesting. This comment it was just a passing
thought
that I had while reading through.

 
> > tools/telemetry/telemetry/page/values.py:443: """Mangles a value name plus
> > optional chart name into a standard string.
> > Maybe "combines" rather than "mangles"... :-P
> 
> Bikeshed. http://en.wikipedia.org/wiki/Name_mangling

Also interesting. Learned something.


Looks good -- I shall continue looking through the other modules :-)

[nduca, 2013-10-31 09:46:03]

@dtu, you didn't post your comments. Guys, c'mon...

[qyearsley, 2013-10-31 16:15:34]

On 2013/10/31 09:46:03, nduca wrote:
> @dtu, you didn't post your comments. Guys, c'mon...

Yep, after we get this in we can then start getting that 'General Value' class
in there.

Although, I suppose the next CL after this is to change
PageMeasurementResults.Add* methods and change all of the metrics/measurements
to use those?

[dtu, 2013-10-31 23:24:29]

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/docs/tel...
File tools/telemetry/docs/telemetry.page.page_measurement_results.html (right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/docs/tel...
tools/telemetry/docs/telemetry.page.page_measurement_results.html:1: 
Get rid of this.

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page.py (right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page.py:37: return self.url < other.url
Compare on page.name, then page.url.

[dtu, 2013-11-01 00:16:31]

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/block_page_measurement_results.py (right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/block_page_measurement_results.py:23:
values_by_value_name = {}
You're doing this same value sorting operation in multiple places. Maybe write a
helper function to do it, or maybe PageMeasurementResults should take care of
maintaining or returning a sorted Value list.

Additionally, this could just be:

for value in sorted(values, key=lambda value: value.name):
  lines.append

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_measurement_results.py (right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_measurement_results.py:26:
pages.difference_update(self.pages_that_had_errors_or_failures)
You never use pages_that_had_errors_or_failures directly. Maybe you should get a
list of successes instead. Or do [value.page for value in
self._all_page_specific_values if value.page.url in self.successes]

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_measurement_results.py:53: if value.page not
in pages_that_had_errors_or_failures]
use self.pages_that_succeeded (defined above), instead.

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/value.py (right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/value.py:140: def
GetRepresentativeStringValue(self):
nit: GetRepresentativeString, to avoid confusion with our new definition of
"Value." Same with GetRepresentativeNumber.

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/value_backcompat.py (right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/value_backcompat.py:6: # This module helps
convert the old PageMeasurementResults API into the new
nit: convert to a docstring.

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/value_backcompat.py:17: value, important=True)
nit: indents here and below.

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/value_backcompat.py:39: raise
Exception('Unrecognized data type %s', data_type)
nit: ValueError

[tonyg, 2013-11-04 20:15:03]

This change is both All-Kinds-Of-Awesome and terrifying at the same time. I'd be
pretty surprised if we can land this without breaking something on the perf
dashboard. So if you have ideas or are willing to do things in smaller chunks, I
think that would help us land it more safely. But I'll leave that up to you.

The overall structure lg2m, but I'd like to review again after you've addressed
all the comments. Will try to respond faster this time.

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/block_page_measurement_results.py (right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/block_page_measurement_results.py:16: if
len(values) == 0:
if not values:

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/buildbot_page_measurement_results.py
(right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/buildbot_page_measurement_results.py:31:
[page.display_name for page in self.pages_that_succeeded])
Whether we use url or display_name in this list, the important thing is that it
matches whatever we output in the page-specific trace name. Not sure if that is
already the case or not, just making a note to double check.

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py
(right):

https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py:83:
'*RESULT a: a= [7,3] seconds\nAvg a: 5.000000seconds\n' +
I think we need to keep these in the order they were run.

[nduca, 2013-11-05 03:43:29]

>
https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
>
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py:83:
> '*RESULT a: a= [7,3] seconds\nAvg a: 5.000000seconds\n' +
> I think we need to keep these in the order they were run.

Why?

[tonyg, 2013-11-05 03:47:53]

On 2013/11/05 03:43:29, nduca wrote:
> >
>
https://codereview.chromium.org/27486002/diff/134001/tools/telemetry/telemetr...
> >
>
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py:83:
> > '*RESULT a: a= [7,3] seconds\nAvg a: 5.000000seconds\n' +
> > I think we need to keep these in the order they were run.
> 
> Why?

A couple of reasons:

The buildbot's PageCyclerGraphingLogProcess tries to make determinations about
which indices in the results correspond to which pages based on the "Pages"
output. I'm not *sure* this will break if you sort them or even that
functionality is necessary. I just know the buildbot code is brittle and it
would be best if we could land a large change like this with no expected
functional differences so it will be easier to troubleshoot if something goes
wrong.

Manfred added a line plot view of the results to the HTML results page. It is
often useful to see the results in order so that you know if, say, the first run
or the last run had different performance characteristics.

[nduca, 2013-11-05 04:11:19]

The order within a page is not changed. The order of runs across pages is what
changes. The problem here is that buildbot wants the values printed in
alphabetical order, but whats their internal values sorted in insertion order.
You simply can't mix and match those in this architecture, not without a whole
new level of complexity. If you want that, i can investigate it. Or we can relax
the requirement that buildbot print in alphabetical order... but having both is
really hard.

[tonyg, 2013-11-05 04:48:43]

tl;dr

RESULT a_by_url: http___www.bar.com_= 7 seconds
RESULT a_by_url: http___www.foo.com_= 3 seconds
vs.
RESULT a_by_url: http___www.foo.com_= 3 seconds
RESULT a_by_url: http___www.bar.com_= 7 seconds
is OK.

[7,3] vs [3,7] is not.


Details:

Here's a link to the buildbot code I'm trying to keep happy:
https://code.google.com/p/chromium/codesearch#chromium/tools/build/scripts/sl...

I'm a little confused by your comment, I thought what we want (and the
current behavior) for lists everywhere is to keep them in the order in
which they are executed. I'm not aware of any buildbot expectations on
alphabetization.

I am sure that the buildbot doesn't care in which order the RESULT
lines are output. We can change that all we like. The only reason they
are alphabetical right now is so that measurements with many results
(like loading_trace) are more user-friendly.

What I don't think we should change is making any list output in any
order other than execution order. That would break the ability to
interpret those spark lines and line charts on the HTML results page.
Most tests have a slower first run than subsequent runs, and some
tests display patterns like getting slower or faster throughout the
runs. Sometimes a previous page affects a subsequent page. The user
needs to be able to see those patterns.

On Mon, Nov 4, 2013 at 8:11 PM,  <nduca@chromium.org> wrote:
> The order within a page is not changed. The order of runs across pages is
> what
> changes. The problem here is that buildbot wants the values printed in
> alphabetical order, but whats their internal values sorted in insertion
> order.
> You simply can't mix and match those in this architecture, not without a
> whole
> new level of complexity. If you want that, i can investigate it. Or we can
> relax
> the requirement that buildbot print in alphabetical order... but having both
> is
> really hard.
>
> https://codereview.chromium.org/27486002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nduca, 2013-11-05 05:01:46]

That helps, I think I can make it work if we relax the alphabetization. I
thought the alphabetical part was relevant.

[tonyg, 2013-11-05 05:02:35]

On 2013/11/05 05:01:46, nduca wrote:
> That helps, I think I can make it work if we relax the alphabetization. I
> thought the alphabetical part was relevant.

Nice :)

[nduca, 2013-11-16 07:47:33]

Sorry for the long delay folks. I hate it when they make me do my day job ^_^.
Tony, the concerns about ordering are, I believe, addressed. There should be
pretty solid unit test coverage for it.

[tonyg, 2013-11-16 16:53:25]

Looking really close, a few more comments.

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py
(right):

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/buildbot_page_measurement_results_unittest.py:252:
'*RESULT a: a= [3,4,7,8] seconds\n' +
This still seems wrong. Any ideas?

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_test_results_unittest.py (right):

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test_results_unittest.py:1: # Copyright (c)
2012 The Chromium Authors. All rights reserved.
2013

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/__init__.py (right):

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/__init__.py:5: The Value hierarchy provide a way
of representing the values measurements
s/provide/provides/

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/histogram.py (right):

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:57: # More buildbot insanity:
perf_tests_results_helper requires the histogram
If you like, I suspect we could fix that file in this CL. We can definitely
change anything we want about that class with a chrome/src only change and a
grep over chrome/src.

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:62: # This has to hand-JSONify the
histogram to ensure the order of keys
This doesn't sound right. There's no way key-order matters in a dict because
ordering is not guaranteed. Can you explain more?

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/histogram.py:90: return None
Just curious, should this be NotImplemented?

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/scalar.py (right):

https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/scalar.py:36: # Buildbot's print_perf_results
method likes to get lists for all values,
Should we fix this here or just punt?

[nduca, 2013-11-16 22:33:23]

Thanks for the weekend review! Will dig into the actual value difference on that
one test you spotted, that does look fishy.

> If you like, I suspect we could fix that file in this CL. We can definitely
> change anything we want about that class with a chrome/src only change and a
> grep over chrome/src.
If its cool, I'd love to hold that off. I worry already about this patch having
complications when I land it. If I understand,
@qyearsly is thinking these days of making telemetry output json directly to
buildbot too, which may bypass this mess fully?

>
https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/histogram.py:62: # This has to hand-JSONify
the
> histogram to ensure the order of keys
> This doesn't sound right. There's no way key-order matters in a dict because
> ordering is not guaranteed. Can you explain more?

I updated the comment to explain more, but its actually because the buildbot
results unittests check string equality on the result of runs with
histograms. I could in theory write some code to parse the result of the
buildbot in order to test it, if you wanted, though per the comments before, I
also feel a bit like this whole system is marked for death anyway?

> 
>
https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/histogram.py:90: return None
> Just curious, should this be NotImplemented?
Actually great question. Updated the strings a bit to clarify, but if we threw
NotImplemented, then tests that had histograms would crash. The
MergeLikeValuesFromDifferentPages api allows you to return None indicating that
data is not semantically mergable across pages. This would be the case for trace
data, for instance. If we someday start using histograms more heavily, we could
come back and implement this further.


>
https://codereview.chromium.org/27486002/diff/484001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/scalar.py:36: # Buildbot's print_perf_results
> method likes to get lists for all values,
> Should we fix this here or just punt?

Similar thought as before. On balance, what would you recommend? If you think
its worth doing the fixes despite my comments, thats totally cool and I can dig
in a bit more.

[nduca, 2013-11-16 22:57:52]

Ordering issue fixed, turned out to be a one line mistake in buildbot. I changed
the variable naming to avoid falling into the trap again.

[tonyg, 2013-11-17 16:35:12]

lgtm

PLMK when you decide to land this (ideally on a weekday morning) and I'll help
watch the bots.

> If its cool, I'd love to hold that off. I worry already about this patch
having
> complications when I land it. 

Of course. I wasn't sure if being able to change that API would simplify your
life at all. We should definitely do the minimal thing here.

[nduca, 2013-11-17 23:27:27]

Yeah, I'll touch base with you Monday morning and we can come up with a game
plan. Dont want to dump this just on you.:)

[dtu, 2013-11-22 22:57:42]

LGTMDCOMMIT!!!

( ͡° ͜ʖ ͡°)