tracing/tracing/metrics/media_metric.html - Issue 2996233004: Port a media metric to TBMv2

Side by Side Diff: tracing/tracing/metrics/media_metric.html

Issue 2996233004: Port a media metric to TBMv2 (Closed)

Patch Set: Created 3 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <!--

	3 Copyright 2017 The Chromium Authors. All rights reserved.

	4 Use of this source code is governed by a BSD-style license that can be

	5 found in the LICENSE file.
	CalebRouleau 2017/08/18 22:10:03 There should be overall description of what mediaM There should be overall description of what mediaMetric is trying to do and what is already done and what is work in progress. something like: "media_metrics uses Chrome trace events to calculate metrics about video playback. It is meant to be used for pages with a <video> element. It is used by videostack-eng@google.com team for regression testing." Also, is there some way to make it clear that we are the owners for this file so people will have us review changes to it? If the framework doesn't provide a formal way then perhaps we should just add a comment saying that crouleau@ and johnchen@ are owners. charliea (OOO until 10-5) 2017/08/25 19:35:09 There's definitely a way to add an OWNERS file and Show quoted text On 2017/08/18 22:10:03, CalebRouleau wrote: > There should be overall description of what mediaMetric is trying to do and what > is already done and what is work in progress. something like: > > "media_metrics uses Chrome trace events to calculate metrics about video > playback. It is meant to be used for pages with a <video> element. It is used by > mailto:videostack-eng@google.com team for regression testing." > > Also, is there some way to make it clear that we are the owners for this file so > people will have us review changes to it? If the framework doesn't provide a > formal way then perhaps we should just add a comment saying that crouleau@ and > johnchen@ are owners. There's definitely a way to add an OWNERS file and list this file as having explicitly different owners, but I see two problems with that: 1) tracing/ owners won't be able to make small refactoring changes, which happen relatively often in TBMv2, without your approval. 2) You'll be able to submit changes to the file without review from any tracing/ owners, which makes it likely that the stil in this codebase is likely to diverge from that in the rest of the codebase Neither of these is good. Realistically, I think that doing nothing is probably the right thing: if someone wants to make substantive changes to this file, the tracing/ owner should look at the blame lines for this file, see who's responsible for the bulk of it, and make sure to loop you into the review. Understanding the necessity of what reviews to delegate is as important as anything else in the process of becoming a reviewer. +benjhayden@chromium.org, eakuefner@chromium.org, would you agree with this? benjhayden 2017/08/28 17:40:15 +100, thank you! Show quoted text On 2017/08/25 at 19:35:09, charliea wrote: > On 2017/08/18 22:10:03, CalebRouleau wrote: > > There should be overall description of what mediaMetric is trying to do and what > > is already done and what is work in progress. something like: > > > > "media_metrics uses Chrome trace events to calculate metrics about video > > playback. It is meant to be used for pages with a <video> element. It is used by > > mailto:videostack-eng@google.com team for regression testing." > > > > Also, is there some way to make it clear that we are the owners for this file so > > people will have us review changes to it? If the framework doesn't provide a > > formal way then perhaps we should just add a comment saying that crouleau@ and > > johnchen@ are owners. > > There's definitely a way to add an OWNERS file and list this file as having explicitly different owners, but I see two problems with that: > > 1) tracing/ owners won't be able to make small refactoring changes, which happen relatively often in TBMv2, without your approval. > 2) You'll be able to submit changes to the file without review from any tracing/ owners, which makes it likely that the stil in this codebase is likely to diverge from that in the rest of the codebase > > Neither of these is good. Realistically, I think that doing nothing is probably the right thing: if someone wants to make substantive changes to this file, the tracing/ owner should look at the blame lines for this file, see who's responsible for the bulk of it, and make sure to loop you into the review. Understanding the necessity of what reviews to delegate is as important as anything else in the process of becoming a reviewer. > > +benjhayden@chromium.org, eakuefner@chromium.org, would you agree with this? +100, thank you! johnchen 2017/08/29 22:45:46 Added comments about the purpose of this metric, w Show quoted text On 2017/08/28 17:40:15, benjhayden wrote: > On 2017/08/25 at 19:35:09, charliea wrote: > > On 2017/08/18 22:10:03, CalebRouleau wrote: > > > There should be overall description of what mediaMetric is trying to do and > what > > > is already done and what is work in progress. something like: > > > > > > "media_metrics uses Chrome trace events to calculate metrics about video > > > playback. It is meant to be used for pages with a <video> element. It is > used by > > > mailto:videostack-eng@google.com team for regression testing." > > > > > > Also, is there some way to make it clear that we are the owners for this > file so > > > people will have us review changes to it? If the framework doesn't provide a > > > formal way then perhaps we should just add a comment saying that crouleau@ > and > > > johnchen@ are owners. > > > > There's definitely a way to add an OWNERS file and list this file as having > explicitly different owners, but I see two problems with that: > > > > 1) tracing/ owners won't be able to make small refactoring changes, which > happen relatively often in TBMv2, without your approval. > > 2) You'll be able to submit changes to the file without review from any > tracing/ owners, which makes it likely that the stil in this codebase is likely > to diverge from that in the rest of the codebase > > > > Neither of these is good. Realistically, I think that doing nothing is > probably the right thing: if someone wants to make substantive changes to this > file, the tracing/ owner should look at the blame lines for this file, see who's > responsible for the bulk of it, and make sure to loop you into the review. > Understanding the necessity of what reviews to delegate is as important as > anything else in the process of becoming a reviewer. > > > > mailto:+benjhayden@chromium.org, mailto:eakuefner@chromium.org, would you agree with this? > > +100, thank you! Added comments about the purpose of this metric, with emails of people responsible.
	6 -->

	7

	8 <link rel="import" href="/tracing/metrics/metric_registry.html">

	9 <link rel="import" href="/tracing/value/histogram.html">

	10

	11 <script>

	12 'use strict';

	13

	14 tr.exportTo('tr.metrics', function() {

	15 function mediaMetric(histograms, model) {

	16 let playStart;

	17 let timeToPlay;

	18

	19 for (const event of model.getDescendantEvents()) {
	benjhayden 2017/08/28 17:40:15 Metric performance doesn't matter terribly much wh Metric performance doesn't matter terribly much when running benchmarks on the waterfall, but users can and do run metrics interactively in trace viewer and telemetry directly. It's also nice to be good stewards of the bots when possible. Instead of iterating over many thousands of events, please consider the process[es], thread[s], and event container[s] that could contain your events. We provide the model helper system to make this a bit easier. The powerMetric demonstrates finding the browser and renderer processes and their main threads: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... Please let me know if you have any questions about finding your events efficiently. johnchen 2017/08/29 22:45:46 Modified the code to look for events in the render Show quoted text On 2017/08/28 17:40:15, benjhayden wrote: > Metric performance doesn't matter terribly much when running benchmarks on the > waterfall, but users can and do run metrics interactively in trace viewer and > telemetry directly. It's also nice to be good stewards of the bots when > possible. > Instead of iterating over many thousands of events, please consider the > process[es], thread[s], and event container[s] that could contain your events. > We provide the model helper system to make this a bit easier. > The powerMetric demonstrates finding the browser and renderer processes and > their main threads: > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... > > Please let me know if you have any questions about finding your events > efficiently. Modified the code to look for events in the renderer processes only. Can't restrict to a particular thread, since the two events we're looking for are raised from different threads. Please let me know if there are any more ways to improve the performance.
	20 if (event.title === 'EventDispatch') {

	21 const type = event.args.data.type;

	22 if (type === 'willPlay') {
	CalebRouleau 2017/08/18 22:10:03 Discussed offline: maybe we should either use "wil Discussed offline: maybe we should either use "willPlay" or use "play", but not both. Also, can Ned/someone else from speed infra team tell us if willPlay event (which is generated from JavaScript) is the wrong wway to do this? I suspect that we need to add trace events directly into Chrome. benjhayden 2017/08/28 17:40:15 This is a metric design question, which is mostly Show quoted text On 2017/08/18 at 22:10:03, CalebRouleau wrote: > Discussed offline: maybe we should either use "willPlay" or use "play", but not both. Also, can Ned/someone else from speed infra team tell us if willPlay event (which is generated from JavaScript) is the wrong wway to do this? I suspect that we need to add trace events directly into Chrome. This is a metric design question, which is mostly up to you to experiment with, but we can help somewhat. I recommend considering two factors: 1. which events more closely reflect the user's experience, and 2. which events are less noisy. Often, the noisier event is more representative of the user's experience, and the less noisy event is easier to bisect when the noisier metric regresses, so we recommend measuring both and defining the less noisy metric as a diagnostic metric of the more user-centric metric. https://codesearch.chromium.org/chromium/src/docs/speed/diagnostic_metrics.md
	23 playStart = event.start;

	24 } else if (type === 'play' \|\| type === 'loadedmetadata') {

	25 if (playStart === undefined) {

	26 playStart = event.start;

	27 }

	28 } else if (type === 'playing') {

	29 if (playStart !== undefined && timeToPlay === undefined) {

	30 timeToPlay = event.start - playStart;
	nednguyen 2017/08/18 23:50:55 How can we be sure that these two events belong to How can we be sure that these two events belong to a same video? Does this work correctly if: 1) THere are multiple videos 2) There are multiple videos and some videos are being corrupted & only have "willPlay" but not "playing"? johnchen 2017/08/29 22:45:46 For now we only support one video on the page, tho Show quoted text On 2017/08/18 23:50:55, nednguyen (ooo til 08-30) wrote: > How can we be sure that these two events belong to a same video? Does this work > correctly if: > 1) THere are multiple videos > 2) There are multiple videos and some videos are being corrupted & only have > "willPlay" but not "playing"? For now we only support one video on the page, though this would be an area for future improvements.
	31 }

	32 }

	33 }

	34 }

	35

	36 if (timeToPlay !== undefined) {

	37 addTimeValue(histograms, 'time_to_play', timeToPlay);
	CalebRouleau 2017/08/18 22:10:03 Is there at place where time_to_play is documented Is there at place where time_to_play is documented in a descriptive way? If there is not, it would be nice to describe it somewhere. Something like: "time_to_play calculates how long after a video is requested to start playing before the video actually starts. If time_to_play regresses, then users will click to play videos and then have to wait longer before the videos start actually playing." charliea (OOO until 10-5) 2017/08/25 19:35:09 Yea - I agree with you in this, but I'm not sure t Show quoted text On 2017/08/18 22:10:03, CalebRouleau wrote: > Is there at place where time_to_play is documented in a descriptive way? If > there is not, it would be nice to describe it somewhere. Something like: > > "time_to_play calculates how long after a video is requested to start playing > before the video actually starts. If time_to_play regresses, then users will > click to play videos and then have to wait longer before the videos start > actually playing." Yea - I agree with you in this, but I'm not sure there is any great place to describe this right now, beyond just a comment above the mediaMetric comment, which is where we typically do it. It'd be great to do it in a way that surfaced in the dashboard to help users there. If that happens, though, then we need to get into what database we store that string in, etc., which is very likely why it hasn't happened. johnchen 2017/08/29 22:45:46 Added comments in the file header. Added comments in the file header.
	38 }

	39 }

	40

	41 function addTimeValue(histograms, name, value) {
	benjhayden 2017/08/28 17:40:15 Instead of defining this helper function, please u Instead of defining this helper function, please use histograms.createHistogram(name, unit, samples, opt_options) https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu... johnchen 2017/08/29 22:45:46 Done. Show quoted text On 2017/08/28 17:40:15, benjhayden wrote: > Instead of defining this helper function, please use > histograms.createHistogram(name, unit, samples, opt_options) > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/valu... Done.
	42 const hist = new tr.v.Histogram(name,

	43 tr.b.Unit.byName.timeDurationInMs_smallerIsBetter);

	44 hist.addSample(value);

	45 histograms.addHistogram(hist);

	46 }

	47

	48 tr.metrics.MetricRegistry.register(mediaMetric);

	49

	50 return {

	51 mediaMetric,

	52 };

	53 });

	54 </script>

OLD	NEW

« no previous file with comments | « tracing/tracing/metrics/all_metrics.html ('k') | no next file » | no next file with comments »