Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
367523002:
Handle multiple duplicate timeline interaction records. (Closed)
DescriptionHandle multiple duplicate timeline interaction records.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282739
Patch Set 1 #Patch Set 2 : #Patch Set 3 : add tests #Patch Set 4 : #
Total comments: 10
Patch Set 5 : address comments #Patch Set 6 : ensure repeatable records are identical #
Total comments: 16
Patch Set 7 : address comments #Patch Set 8 : change is_repeatable to repeatable #Patch Set 9 : update doc #Patch Set 10 : update unittests #
Messages
Total messages: 23 (0 generated)
Here's a take at handling multiple duplicate timeline interaction records. This does the following: (1) Enforces that only interactions with the same logical name can only show up more than once if they have the 'is_repeatable' flag. By default interactions are not repeatable. (2) If they are repeatable, then we pass the entire array of interactions to the appropriate metric's AddResults function, rather than adding results one interaction at a time. This delegates the responsibility of how to merge multiple interactions to the individual metric. (3) Smoothness and Responsiveness already handled this case for us, so no additional changes were needed there. Additional thoughts: Since interactions already supported arbitrary flags, adding the 'is_repeatable' flag to the javascript marker seemed to be the most straightforward way to add this feature, since it has to be encoded somehow into that string. I wasn't sure if it made sense going forward to distinguish between arbitrary flags (like is_repeatable) and available metrics (like is_smooth). For example, going from Interaction.logicalname/is_smooth,is_responsive,is_repeatable to something like Interaction.logicalname/metrics=is_smooth,is_responsive/flags=is_repeatable
On 2014/07/01 23:55:18, ariblue wrote:
> Here's a take at handling multiple duplicate timeline interaction records.
This
> does the following:
>
> (1) Enforces that only interactions with the same logical name can only show
up
> more than once if they have the 'is_repeatable' flag. By default interactions
> are not repeatable.
>
> (2) If they are repeatable, then we pass the entire array of interactions to
the
> appropriate metric's AddResults function, rather than adding results one
> interaction at a time. This delegates the responsibility of how to merge
> multiple interactions to the individual metric.
>
> (3) Smoothness and Responsiveness already handled this case for us, so no
> additional changes were needed there.
>
> Additional thoughts:
>
> Since interactions already supported arbitrary flags, adding the
'is_repeatable'
> flag to the javascript marker seemed to be the most straightforward way to add
> this feature, since it has to be encoded somehow into that string. I wasn't
sure
> if it made sense going forward to distinguish between arbitrary flags (like
> is_repeatable) and available metrics (like is_smooth). For example, going from
>
> Interaction.logicalname/is_smooth,is_responsive,is_repeatable
>
> to something like
>
> Interaction.logicalname/metrics=is_smooth,is_responsive/flags=is_repeatable
Since we will soon need to have InteractionLoad..., I am not so sure this kind
of format is a good solution for encoding info of interactions with complicated
properties. I think using json format may make it easier to maintain backward
compatibilities if we want to add more stuffs to the interaction records.
Maybe something like:
'{InteractionName: ..., metrics_flags: [....], is_repeatable: True,
is_load_interaction: False....}'
What do you guys think about this?
> What do you guys think about this? I think we're overthinking. is_repeatable as just another flag is fine.
i'd suggest throwing out 2 or 3 alternate names than is_repeatable that don't begin with is_, e.g. can_occur_more_than_once so we can bikeshed briefly. but aside from bikesheddding, this lgtm. https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:45: """Represents an interaction that took place during a timeline recording. you should be updating the docstrings here to explain the semantics of the flag
please coordinate with https://codereview.chromium.org/367283002 which has repeats in it. one of you should agree [maybe via gchat? wiltzius is in tokyo time, note] who lands first and who will adjust accordingly.
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/page/actions/action_runner.py:49: this interaction. Update doc for new flag here also. https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:35: nit: Two blank lines between top-level definitions. https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:101: i.HasMetric(metric_type)] 'interactions' here refer to the list of repeatable records with the same name, isn't it? I think it should be an error if repeatable records with the same name don't have the same set of flags. https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:141: def HasMetric(self, metric_type): This name sounds like this function should return False if metric_type not in METRICS and True otherwise.
On 2014/07/07 17:20:51, nduca wrote: > please coordinate with https://codereview.chromium.org/367283002 which has > repeats in it. one of you should agree [maybe via gchat? wiltzius is in tokyo > time, note] who lands first and who will adjust accordingly. key_silk_cases use the legacy smoothness measurement, so I think this change won't affect it.
On 2014/07/07 16:59:16, nduca wrote:
> i'd suggest throwing out 2 or 3 alternate names than is_repeatable that don't
> begin with is_, e.g. can_occur_more_than_once so we can bikeshed briefly. but
> aside from bikesheddding, this lgtm.
Here are some more alternative names for the new flag. Any of them seem
reasonable?
* can_repeat
* can_be_{repeated,duplicated}
* mergeable or can_merge
* not_distinct
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/action_runner.py (right):
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/action_runner.py:49: this interaction.
On 2014/07/07 17:39:03, nednguyen wrote:
> Update doc for new flag here also.
Updated.
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right):
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:35:
On 2014/07/07 17:39:03, nednguyen wrote:
> nit: Two blank lines between top-level definitions.
Done.
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:101:
i.HasMetric(metric_type)]
On 2014/07/07 17:39:03, nednguyen wrote:
> 'interactions' here refer to the list of repeatable records with the same
name,
> isn't it?
That's correct.
> I think it should be an error if repeatable records with the same name don't
> have the same set of flags.
Really? I figured we would want to gracefully handle the case where there are
two repeatable records of the same name but with different metrics. The logic as
it is right now ensures that each metric only uses the interactions that
specified that metric in their flags.
I'm fine either way. We can chat about this more tomorrow.
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right):
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:45:
"""Represents an interaction that took place during a timeline recording.
On 2014/07/07 16:59:16, nduca wrote:
> you should be updating the docstrings here to explain the semantics of the
flag
I've added a section to this docstring which lists the available flags and short
description of what they do. Is that what you meant?
https://codereview.chromium.org/367523002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:141: def
HasMetric(self, metric_type):
On 2014/07/07 17:39:03, nednguyen wrote:
> This name sounds like this function should return False if metric_type not in
> METRICS and True otherwise.
This is a method for a TimelineInteractionRecord object. It is not a
staticmethod. For example, i.HasMetric('is_smooth'). It throws an error if the
metric_type is not a valid metric_type. Otherwise, it returns true/false if the
TIR object has the appropriate flag.
On 2014/07/07 18:29:12, nednguyen wrote: > On 2014/07/07 17:20:51, nduca wrote: > > please coordinate with https://codereview.chromium.org/367283002 which has > > repeats in it. one of you should agree [maybe via gchat? wiltzius is in tokyo > > time, note] who lands first and who will adjust accordingly. > > key_silk_cases use the legacy smoothness measurement, so I think this change > won't affect it. https://codereview.chromium.org/367283002 is in the CQ now. I'm happy to land changes to it if you need; sorry I just saw this now.
lgtm LGTM after making sure that repeatable records must be identical.
https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:54: return '%s-%s-%s' % (metric_type, self._result_prefix, trace_name) The results wrapper is a hack that we eventually want to get rid off. Can you remove the metric_type from here and add 'responsive-' to responsiveness_metric's numbers?
https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:44: as this interaction. Is it just the logical name or the flag as well? https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:46: class _ResultsWrapper(object): Is this meant to have the same API as PageMeasurementResults eventually? Maybe document it for now. I feel that we should probably do this a bit later. In particular, Ned is trying to refactor calls to Add() to instead do AddValue(). https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:95: raise InvalidInteractions('Duplicate Unrepeatable Interactions on the ' unrepeatable interaction records https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:111: wrapped_results.SetMetricType(metric_type) Hm, you're using a single results wrapper and changing the metric type? Have you consider instead just creating a new one for each iteration? That should simplify the ResultsWrapper API. https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:112: metric = self._get_metric_from_metric_type_callback(metric_type) What about just calling _GetMetricFromMetricType directly? https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:37: def AssertFlagsAreValid(flags): Is this used anywhere else? If not, make private.
https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:46: class _ResultsWrapper(object): On 2014/07/09 20:52:14, chrishenry wrote: > Is this meant to have the same API as PageMeasurementResults eventually? Maybe > document it for now. I feel that we should probably do this a bit later. In > particular, Ned is trying to refactor calls to Add() to instead do AddValue(). The plan I have in mind is Value should be able to take in the record as an argument. So eventually: results.AddValue(ScalarValue(page=results.current_page,record=...,)) https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:112: metric = self._get_metric_from_metric_type_callback(metric_type) On 2014/07/09 20:52:14, chrishenry wrote: > What about just calling _GetMetricFromMetricType directly? The original idea is that people should be able to subclass timeline_based_measurement and override the mapping of flags -> metrics. I am not sure whether we still need to support this, so I am fine with either keeping this and making _GetMetricFromMetricType a method timeline_based_measurement or getting rid of this callback entirely.
https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:44: as this interaction. On 2014/07/09 20:52:14, chrishenry wrote: > Is it just the logical name or the flag as well? Both, sort of. Per Ned's comments, we enforce that if two interactions have the same logical name, they must also have the same metrics. https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/timeline_based_measurement.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:54: return '%s-%s-%s' % (metric_type, self._result_prefix, trace_name) On 2014/07/09 20:13:06, nednguyen wrote: > The results wrapper is a hack that we eventually want to get rid off. Can you > remove the metric_type from here and add 'responsive-' to > responsiveness_metric's numbers? It seems silly to need to add fast- or responsive- to every single result, but I'll go ahead and do it. https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:95: raise InvalidInteractions('Duplicate Unrepeatable Interactions on the ' On 2014/07/09 20:52:14, chrishenry wrote: > unrepeatable interaction records Done. https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:111: wrapped_results.SetMetricType(metric_type) On 2014/07/09 20:52:14, chrishenry wrote: > Hm, you're using a single results wrapper and changing the metric type? Have you > consider instead just creating a new one for each iteration? That should > simplify the ResultsWrapper API. After removing the metric_type above, I can just get rid of this. https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_based_measurement.py:112: metric = self._get_metric_from_metric_type_callback(metric_type) On 2014/07/09 21:27:50, nednguyen wrote: > On 2014/07/09 20:52:14, chrishenry wrote: > > What about just calling _GetMetricFromMetricType directly? > > The original idea is that people should be able to subclass > timeline_based_measurement and override the mapping of flags -> metrics. > I am not sure whether we still need to support this, so I am fine with either > keeping this and making _GetMetricFromMetricType a method > timeline_based_measurement or getting rid of this callback entirely. I'll leave it as is for now, and we can revisit this later in a separate CL. https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/web_perf/timeline_interaction_record.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/web_perf/timeline_interaction_record.py:37: def AssertFlagsAreValid(flags): On 2014/07/09 20:52:14, chrishenry wrote: > Is this used anywhere else? If not, make private. Done.
lgtm https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/page/actions/action_runner.py (right): https://codereview.chromium.org/367523002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/page/actions/action_runner.py:44: as this interaction. On 2014/07/10 00:35:09, ariblue wrote: > On 2014/07/09 20:52:14, chrishenry wrote: > > Is it just the logical name or the flag as well? > > Both, sort of. Per Ned's comments, we enforce that if two interactions have the > same logical name, they must also have the same metrics. Then let's update the doc appropriately.
The CQ bit was checked by ariblue@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ariblue@google.com/367523002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was checked by ariblue@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ariblue@google.com/367523002/200001
Message was sent while issue was closed.
Change committed as 282739 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
