|
|
DescriptionFinish migrating media metrics to TBMv2
Update media metrics to add buffering time, dropped frame count,
and seek time.
An example trace that can use these metrics is available at
http://www/~zhanliang/3020433002.trace.html, with corresponding
histograms at http://www/~zhanliang/3020433002.html.
BUG=chromium:700160
Review-Url: https://chromiumcodereview.appspot.com/3020433002
Committed: https://chromium.googlesource.com/catapult/+/e79820f09d19a90d428f9534ce80e47203fe185c
Patch Set 1 #
Total comments: 2
Patch Set 2 : Refactoring #
Total comments: 6
Patch Set 3 : Optimization #Patch Set 4 : Fix typo #
Total comments: 19
Patch Set 5 : More refactoring #
Total comments: 1
Patch Set 6 : Update based on feedback from crouleau@ #
Total comments: 2
Patch Set 7 : Remove an unnecessary if #
Messages
Total messages: 30 (10 generated)
johnchen@chromium.org changed reviewers: + benjhayden@chromium.org
This CL, together with a CL in media directory (https://crrev.com/c/674166), finishes migrating media metrics to TBMv2. The following metrics are added: buffering time, dropped frame count, and seek time. A note about the seek time metric: Tough video cases page set does two seeks per page. Previously, these seeks were labeled as "seek warm" and "seek cold". However, it is inconvenient to use such labels in TBMv2, so I used the seek target time as label instead. "Seek warm" has become "seek 0.5" (seek to 0.5 second mark), and "seek cold" has become "seek 9". This way, if we add or remove seeks in the future, the code can always handle it correctly as long as each seek is to a different target.
Description was changed from ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. BUG=chromium:700160 ========== to ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. BUG=chromium:700160 ==========
crouleau@chromium.org changed reviewers: + crouleau@chromium.org
Thanks for working on this! I hope my comment isn't too rough: I just know that once this code is in it will become a canonical solution and no one will likely refactor it. So I want to make sure it's done in a way that is easy to extend. Feel free to disagree with my comment, and we can discuss in more detail if that is the case. https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:83: function processMainThread(thread) { Even though the concept of what you're doing here is pretty simple, the code ends up being difficult to read because you're calculating all the metrics at the same time. It would be much more readable to have a bunch of independent functions to calculate each of the metrics: playStart, playEnd, timeToAudioPlay, framesDropped, etc. I know this could be a bit slower because you will need to loop through getDescendentEvents() multiple times, but the improvement to readability will be worth it. I also don't think it will be very much slower. If you do things like this, then it will be easy for to understand how a metric is calculated duration = getPlaybackDuration() playStart = getPlayStart() playEnd = getPlayEnd() timeToVideoPlay = getTimeToVideoPlay() timeToAudioPlay = getTimeToAudioPlay() etc. So if I'm reading the above code, then I can quickly find the function whose metric I want to understand and read it. I don't have to try to understand all of this at once. I could also easily change how a specific metric is calculated without being worried that I might break the code to calculate the other metrics. https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... tracing/tracing/metrics/media_metric.html:107: } We throw an error for multiple playStart events. Why not throw an error for multiple playEnd events? Just a sanity check.
On 2017/09/20 22:53:20, CalebRouleau wrote: > Thanks for working on this! I hope my comment isn't too rough: I just know that > once this code is in it will become a canonical solution and no one will likely > refactor it. So I want to make sure it's done in a way that is easy to extend. > Feel free to disagree with my comment, and we can discuss in more detail if that > is the case. > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > File tracing/tracing/metrics/media_metric.html (right): > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:83: function processMainThread(thread) > { > Even though the concept of what you're doing here is pretty simple, the code > ends up being difficult to read because you're calculating all the metrics at > the same time. It would be much more readable to have a bunch of independent > functions to calculate each of the metrics: playStart, playEnd, timeToAudioPlay, > framesDropped, etc. I know this could be a bit slower because you will need to > loop through getDescendentEvents() multiple times, but the improvement to > readability will be worth it. I also don't think it will be very much slower. If > you do things like this, then it will be easy for to understand how a metric is > calculated > > duration = getPlaybackDuration() > playStart = getPlayStart() > playEnd = getPlayEnd() > timeToVideoPlay = getTimeToVideoPlay() > timeToAudioPlay = getTimeToAudioPlay() > etc. > > So if I'm reading the above code, then I can quickly find the function whose > metric I want to understand and read it. I don't have to try to understand all > of this at once. I could also easily change how a specific metric is calculated > without being worried that I might break the code to calculate the other > metrics. > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:107: } > We throw an error for multiple playStart events. Why not throw an error for > multiple playEnd events? Just a sanity check. I have re-factored the code based on your suggestions, and it does look more readable. Please take a look and let know what you think. The only drawback is now we iterate through the events multiple times, so the runtime perf might be slightly worse.
On 2017/09/20 22:53:20, CalebRouleau wrote: > Thanks for working on this! I hope my comment isn't too rough: I just know that > once this code is in it will become a canonical solution and no one will likely > refactor it. So I want to make sure it's done in a way that is easy to extend. > Feel free to disagree with my comment, and we can discuss in more detail if that > is the case. > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > File tracing/tracing/metrics/media_metric.html (right): > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:83: function processMainThread(thread) > { > Even though the concept of what you're doing here is pretty simple, the code > ends up being difficult to read because you're calculating all the metrics at > the same time. It would be much more readable to have a bunch of independent > functions to calculate each of the metrics: playStart, playEnd, timeToAudioPlay, > framesDropped, etc. I know this could be a bit slower because you will need to > loop through getDescendentEvents() multiple times, but the improvement to > readability will be worth it. I also don't think it will be very much slower. If > you do things like this, then it will be easy for to understand how a metric is > calculated > > duration = getPlaybackDuration() > playStart = getPlayStart() > playEnd = getPlayEnd() > timeToVideoPlay = getTimeToVideoPlay() > timeToAudioPlay = getTimeToAudioPlay() > etc. > > So if I'm reading the above code, then I can quickly find the function whose > metric I want to understand and read it. I don't have to try to understand all > of this at once. I could also easily change how a specific metric is calculated > without being worried that I might break the code to calculate the other > metrics. > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > tracing/tracing/metrics/media_metric.html:107: } > We throw an error for multiple playStart events. Why not throw an error for > multiple playEnd events? Just a sanity check. I have re-factored the code based on your suggestions, and it does look more readable. Please take a look and let know what you think. The only drawback is now we iterate through the events multiple times, so the runtime perf might be slightly worse.
Thanks! I have a few questions. https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:116: for (const event of audioThread.getDescendantEvents()) { Can you iterate over only one of the thread's event containers? sliceGroup, asyncSliceGroup, kernelSliceGroup, samples Same for the other getDescendantEvents() callers. https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:136: seekStartTimes.set(event.args.target, event.start); We're a bit leery of copying strings from the trace data into histogram names. We're trying to find a balance between metric flexibility and predictability. Can you add some comments about 'event.args.target'? Where does it come from? (C++ enum? HTML element id?) Can it contain non-alphanumeric characters? Can you categorize its possible values and use names of the categories in the histogram names instead? https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:188: if (timeToVideoPlay === undefined) return undefined; If this function only needs timeToVideoPlay in order to decide whether to return early, then maybe the caller should handle that branch so this function can focus on its job? Same goes for the other helper functions.
On 2017/09/21 04:34:33, johnchen wrote: > On 2017/09/20 22:53:20, CalebRouleau wrote: > > Thanks for working on this! I hope my comment isn't too rough: I just know > that > > once this code is in it will become a canonical solution and no one will > likely > > refactor it. So I want to make sure it's done in a way that is easy to extend. > > Feel free to disagree with my comment, and we can discuss in more detail if > that > > is the case. > > > > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > > File tracing/tracing/metrics/media_metric.html (right): > > > > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > > tracing/tracing/metrics/media_metric.html:83: function > processMainThread(thread) > > { > > Even though the concept of what you're doing here is pretty simple, the code > > ends up being difficult to read because you're calculating all the metrics at > > the same time. It would be much more readable to have a bunch of independent > > functions to calculate each of the metrics: playStart, playEnd, > timeToAudioPlay, > > framesDropped, etc. I know this could be a bit slower because you will need to > > loop through getDescendentEvents() multiple times, but the improvement to > > readability will be worth it. I also don't think it will be very much slower. > If > > you do things like this, then it will be easy for to understand how a metric > is > > calculated > > > > duration = getPlaybackDuration() > > playStart = getPlayStart() > > playEnd = getPlayEnd() > > timeToVideoPlay = getTimeToVideoPlay() > > timeToAudioPlay = getTimeToAudioPlay() > > etc. > > > > So if I'm reading the above code, then I can quickly find the function whose > > metric I want to understand and read it. I don't have to try to understand all > > of this at once. I could also easily change how a specific metric is > calculated > > without being worried that I might break the code to calculate the other > > metrics. > > > > > https://codereview.chromium.org/3020433002/diff/1/tracing/tracing/metrics/med... > > tracing/tracing/metrics/media_metric.html:107: } > > We throw an error for multiple playStart events. Why not throw an error for > > multiple playEnd events? Just a sanity check. > > I have re-factored the code based on your suggestions, and it does look more > readable. Please take a look and let know what you think. > > The only drawback is now we iterate through the events multiple times, so the > runtime perf might be slightly worse. Looks a lot better! I think using benjhayden's suggestions you can clean this up a bit more even. I'll review again after that.
https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:116: for (const event of audioThread.getDescendantEvents()) { On 2017/09/21 05:57:30, benjhayden wrote: > Can you iterate over only one of the thread's event containers? sliceGroup, > asyncSliceGroup, kernelSliceGroup, samples > Same for the other getDescendantEvents() callers. Done. Now getting events from sliceGroup. https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:136: seekStartTimes.set(event.args.target, event.start); On 2017/09/21 05:57:30, benjhayden wrote: > We're a bit leery of copying strings from the trace data into histogram names. > We're trying to find a balance between metric flexibility and predictability. > > Can you add some comments about 'event.args.target'? Where does it come from? > (C++ enum? HTML element id?) Can it contain non-alphanumeric characters? > Can you categorize its possible values and use names of the categories in the > histogram names instead? Target are numerical values, not strings. It equals to the target location of the seek, in unit of seconds. For any automated testing, this should always be a small set of fixed values. (It's always 0.5 and 9 in current media benchmark.) https://codereview.chromium.org/3020433002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:188: if (timeToVideoPlay === undefined) return undefined; On 2017/09/21 05:57:30, benjhayden wrote: > If this function only needs timeToVideoPlay in order to decide whether to return > early, then maybe the caller should handle that branch so this function can > focus on its job? > Same goes for the other helper functions. Done
https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:73: const droppedFrameCount = timeToVideoPlay === undefined ? undefined : I don't know javascript very well, but I'm wondering if this can be written as droppedFrameCount = timeToVideoPlay && getDroppedFrameCount( compositorThread, histograms); since false && console.log('yay') does not print 'yay'. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:78: getBufferingTime(mainThread, playStart, timeToVideoPlay, getX functions should be used for getting a value, not for their side effects. Some of your getX functions create histograms as their primary purpose. Instead you should either: 1. setup the histograms here instead or 2. rename the functions. If a function's primary purpose is to create a histogram, then it should be called "createBufferingTimeHistogram" or something like that. Perhaps I'm confused, but I don't know that the bufferingTime variable that you define here is even used? I would prefer 1 because it allows a reader to clearly understand the purpose of this code: the purpose of the code seems to be to make histograms. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric_test.html (right): https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:21: // No media data This is not necessary since it is redundant with the name "mediaMetric_noData" below. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:25: {name: 'a', args: {}, pid: 52, ts: 524, cat: 'foo', tid: 53, ph: 'B'}, Are all of these fields needed by the test case? If not, could you remove the unnecessary ones? It would be nice to define event types once as constants and then use them in each of the unit tests. Something like define these at the top: const videoLoadEvent = { name: 'WebMediaPlayerImpl::DoLoad', args: {}, pid: 52, ts: 524, cat: 'media', tid: 1, ph: 'X'} const videoRenderEvent = { name: 'VideoRendererImpl::Render', args: {}, pid: 52, ts: 560, cat: 'media', tid: 53, ph: 'X'} etc. then in each test case you can use them const events = [videoRenderEvent, videoLoadEvent, compositorEvent] You could also have the events be parameterized so that you could edit their timestamps or whatever else: function videoRenderEvent(timestamp) { return {name: 'VideoRendererImpl::Render', args: {}, pid: 52, ts: timestamp, cat: 'media', tid: 53, ph: 'X'} const events = [videoRenderEvent(30), videoRenderEvent(40), ...] etc. etc. This makes it easier to read what a test case is supposed to be doing, and it makes it easier to add new test cases. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:32: // Video time-to-play This comment isn't needed. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:57: // Audio time-to-play Comment not needed https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:80: // Buffering time with video Comment not needed. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:107: // Buffering time with audio Comment not needed. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:132: // With seek, no buffering time should be reported This comment contradicts the name of the test case below... I'm thinking the test case is misnamed. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:158: // Dropped frame count with video Comment not needed. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:187: // Seek time Comment not needed. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:225: // Scenario: Play mixed audio/video from start to finish This comment is better because it does add some clarity. Still probably not needed, but it's a judgment call.
https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:73: const droppedFrameCount = timeToVideoPlay === undefined ? undefined : On 2017/09/22 17:44:21, CalebRouleau wrote: > I don't know javascript very well, but I'm wondering if this can be written as > > droppedFrameCount = timeToVideoPlay && getDroppedFrameCount( > compositorThread, histograms); > > since > > false && console.log('yay') > > does not print 'yay'. "const droppedFrameCount = timeToVideoPlay && get.." would work, unless our devs do a good optimization and timeToVideoPlay goes down to 0. I think explicitly checking for undefined value is safer, the extra typing is small, and the extra runtime overhead is negligible. But if you feel strongly about this I'll be happy to change it. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:78: getBufferingTime(mainThread, playStart, timeToVideoPlay, On 2017/09/22 17:44:21, CalebRouleau wrote: > getX functions should be used for getting a value, not for their side effects. > Some of your getX functions create histograms as their primary purpose. Instead > you should either: > > 1. setup the histograms here instead > or > 2. rename the functions. If a function's primary purpose is to create a > histogram, then it should be called "createBufferingTimeHistogram" or something > like that. > > Perhaps I'm confused, but I don't know that the bufferingTime variable that you > define here is even used? > > I would prefer 1 because it allows a reader to clearly understand the purpose of > this code: the purpose of the code seems to be to make histograms. Done. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric_test.html (right): https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:21: // No media data On 2017/09/22 17:44:21, CalebRouleau wrote: > This is not necessary since it is redundant with the name "mediaMetric_noData" > below. Done. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:25: {name: 'a', args: {}, pid: 52, ts: 524, cat: 'foo', tid: 53, ph: 'B'}, On 2017/09/22 17:44:21, CalebRouleau wrote: > Are all of these fields needed by the test case? If not, could you remove the > unnecessary ones? > > It would be nice to define event types once as constants and then use them in > each of the unit tests. Something like > > define these at the top: > > const videoLoadEvent = { > name: 'WebMediaPlayerImpl::DoLoad', args: {}, pid: 52, ts: 524, cat: 'media', > tid: 1, ph: 'X'} > const videoRenderEvent = { > name: 'VideoRendererImpl::Render', args: {}, pid: 52, ts: 560, cat: 'media', > tid: 53, ph: 'X'} > > etc. > > then in each test case you can use them > const events = [videoRenderEvent, videoLoadEvent, compositorEvent] > > You could also have the events be parameterized so that you could edit their > timestamps or whatever else: > > function videoRenderEvent(timestamp) { > return {name: 'VideoRendererImpl::Render', args: {}, pid: 52, ts: timestamp, > cat: 'media', tid: 53, ph: 'X'} > > const events = [videoRenderEvent(30), videoRenderEvent(40), ...] > > etc. etc. > > This makes it easier to read what a test case is supposed to be doing, and it > makes it easier to add new test cases. I've refactored the code to have functions to create the necessary events. Also removed the two unnecessary events in the noData test case. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:132: // With seek, no buffering time should be reported On 2017/09/22 17:44:21, CalebRouleau wrote: > This comment contradicts the name of the test case below... I'm thinking the > test case is misnamed. The test case name I chose was too confusing. I used word "negative" to mean "no", which isn't obvious. Renamed the test case. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric_test.html:225: // Scenario: Play mixed audio/video from start to finish On 2017/09/22 17:44:21, CalebRouleau wrote: > This comment is better because it does add some clarity. Still probably not > needed, but it's a judgment call. I would prefer to keep this one, but OK either way.
LGTM % nits and offline nits. https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:73: const droppedFrameCount = timeToVideoPlay === undefined ? undefined : On 2017/09/23 04:43:02, johnchen wrote: > On 2017/09/22 17:44:21, CalebRouleau wrote: > > I don't know javascript very well, but I'm wondering if this can be written as > > > > > droppedFrameCount = timeToVideoPlay && getDroppedFrameCount( > > compositorThread, histograms); > > > > since > > > > false && console.log('yay') > > > > does not print 'yay'. > > "const droppedFrameCount = timeToVideoPlay && get.." would work, unless our devs > do a good optimization and timeToVideoPlay goes down to 0. I think explicitly > checking for undefined value is safer, the extra typing is small, and the extra > runtime overhead is negligible. But if you feel strongly about this I'll be > happy to change it. No, you're right. Good catch. :) https://codereview.chromium.org/3020433002/diff/80001/tracing/tracing/metrics... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/80001/tracing/tracing/metrics... tracing/tracing/metrics/media_metric.html:163: if (seekTimes.size === 0) return undefined; You can drop all this code checking for the case where you don't find any seek times and just return the empty Map() if you don't find any. That actually makes the code in the caller easier because you don't have to check whether seekTimes is undefined but instead you can just loop through it: if it is empty then no iterations will happen.
A couple of minor updates: * Use an empty Map (instead of undefined) to indicate no seek times * Use timestamps that are easier for human beings in test data. Ben: Could you PTAL? Thanks.
https://codereview.chromium.org/3020433002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/media_metric.html:95: if (seekTimes.size !== 0) { You can drop this "if"
https://codereview.chromium.org/3020433002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/media_metric.html (right): https://codereview.chromium.org/3020433002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/media_metric.html:95: if (seekTimes.size !== 0) { On 2017/09/26 01:26:35, CalebRouleau wrote: > You can drop this "if" Done.
Thanks for the ping! The code looks good, but there are just a few other things that I try to help metric authors consider. Can you add to the CL description links to a sample trace and results.html that demonstrates this metric? You can vulcanize your metric into a trace html file using tracing/bin/trace2html so that we can see your metric in the Metrics side panel. Is there any reason not to support rangeOfInterest? It allows users to run the metric over a subset of a trace in the Metrics side panel, which can sometimes be useful. Telemetry never uses rangeOfInterest. cpuTimeMetric is a good example: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... It's ok if you want to save it for another time. TBMv2 provides a powerful system of Diagnostics to help users navigate metric results. https://github.com/catapult-project/catapult/blob/master/docs/how-to-write-me... Can you add RelatedEventSet sample diagnostics? These are displayed as links to traces in order to help users understand measurements in context in the trace. The accessibility metric is a good example: https://github.com/catapult-project/catapult/blob/ae4cc909a3bb9b54c09107a87f2... You can pass them like histograms.createHistogram(name, unit, {value: ..., diagnostics: {event: new tr.v.d.RelatedEventSet(...)}}); Would it be possible to break down these measurements into another level of detail using sample Breakdown diagnostics? The loading metric is a good example: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... If not, that's ok! It's also ok if you want to leave Breakdowns for another time.
Description was changed from ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. BUG=chromium:700160 ========== to ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. An example set of histograms using these metrics is available at http://www/~zhanliang/3020433002.html BUG=chromium:700160 ==========
Description was changed from ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. An example set of histograms using these metrics is available at http://www/~zhanliang/3020433002.html BUG=chromium:700160 ========== to ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. An example trace that can use these metrics is available at http://www/~zhanliang/3020433002.trace.html, with corresponding histograms at http://www/~zhanliang/3020433002.html. BUG=chromium:700160 ==========
Thanks for the suggestions! Please see reply below: > Can you add to the CL description links to a sample trace and results.html that > demonstrates this metric? > You can vulcanize your metric into a trace html file using > tracing/bin/trace2html so that we can see your metric in the Metrics side panel. I've added links in the CL description. > Is there any reason not to support rangeOfInterest? It allows users to run the > metric over a subset of a trace in the Metrics side panel, which can sometimes > be useful. Telemetry never uses rangeOfInterest. > cpuTimeMetric is a good example: > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... > It's ok if you want to save it for another time. Supporting rangeOfInterest is probably of limited value, as most measurements in this metric are singletons, and are not spread out like CPU time. Also, calculation of these measurements require multiple events, and with rangeOfInterest it's easy to accidentally to remove some necessary events from the range. > TBMv2 provides a powerful system of Diagnostics to help users navigate metric > results. > https://github.com/catapult-project/catapult/blob/master/docs/how-to-write-me... > > Can you add RelatedEventSet sample diagnostics? These are displayed as links to > traces in order to help users understand measurements in context in the trace. > The accessibility metric is a good example: > https://github.com/catapult-project/catapult/blob/ae4cc909a3bb9b54c09107a87f2... > You can pass them like histograms.createHistogram(name, unit, {value: ..., > diagnostics: {event: new tr.v.d.RelatedEventSet(...)}}); This requires some careful design. There are no obvious diagnostics that can be added easily. > Would it be possible to break down these measurements into another level of > detail using sample Breakdown diagnostics? > The loading metric is a good example: > https://github.com/catapult-project/catapult/blob/master/tracing/tracing/metr... > If not, that's ok! It's also ok if you want to leave Breakdowns for another > time. This is probably best left for another time, as some design work is required.
Description was changed from ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. An example trace that can use these metrics is available at http://www/~zhanliang/3020433002.trace.html, with corresponding histograms at http://www/~zhanliang/3020433002.html. BUG=chromium:700160 ========== to ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. An example trace that can use these metrics is available at http://www/~zhanliang/3020433002.trace.html, with corresponding histograms at http://www/~zhanliang/3020433002.html. BUG=chromium:700160 ==========
On 2017/09/27 at 00:43:35, johnchen wrote: > Thanks for the suggestions! Please see reply below: > > > Can you add to the CL description links to a sample trace and results.html that > > demonstrates this metric? > > You can vulcanize your metric into a trace html file using > > tracing/bin/trace2html so that we can see your metric in the Metrics side panel. > > I've added links in the CL description. They return 403 Forbidden for me. Can you set the Share permissions to Public for Google FTEs?
On 2017/09/27 20:48:16, benjhayden wrote: > On 2017/09/27 at 00:43:35, johnchen wrote: > > Thanks for the suggestions! Please see reply below: > > > > > Can you add to the CL description links to a sample trace and results.html > that > > > demonstrates this metric? > > > You can vulcanize your metric into a trace html file using > > > tracing/bin/trace2html so that we can see your metric in the Metrics side > panel. > > > > I've added links in the CL description. > > They return 403 Forbidden for me. Can you set the Share permissions to Public > for Google FTEs? I believe I've fixed the permission. Could you please try again? Thanks.
On 2017/09/27 at 21:13:33, johnchen wrote: > On 2017/09/27 20:48:16, benjhayden wrote: > > On 2017/09/27 at 00:43:35, johnchen wrote: > > > Thanks for the suggestions! Please see reply below: > > > > > > > Can you add to the CL description links to a sample trace and results.html > > that > > > > demonstrates this metric? > > > > You can vulcanize your metric into a trace html file using > > > > tracing/bin/trace2html so that we can see your metric in the Metrics side > > panel. > > > > > > I've added links in the CL description. > > > > They return 403 Forbidden for me. Can you set the Share permissions to Public > > for Google FTEs? > > I believe I've fixed the permission. Could you please try again? Thanks. lgtm, thanks!
The CQ bit was checked by johnchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from crouleau@chromium.org Link to the patchset: https://codereview.chromium.org/3020433002/#ps120001 (title: "Remove an unnecessary if")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1506550439554800, "parent_rev": "5d8ddb9f52690dfc67111317e882f24f41634b69", "commit_rev": "e79820f09d19a90d428f9534ce80e47203fe185c"}
Message was sent while issue was closed.
Description was changed from ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. An example trace that can use these metrics is available at http://www/~zhanliang/3020433002.trace.html, with corresponding histograms at http://www/~zhanliang/3020433002.html. BUG=chromium:700160 ========== to ========== Finish migrating media metrics to TBMv2 Update media metrics to add buffering time, dropped frame count, and seek time. An example trace that can use these metrics is available at http://www/~zhanliang/3020433002.trace.html, with corresponding histograms at http://www/~zhanliang/3020433002.html. BUG=chromium:700160 Review-Url: https://chromiumcodereview.appspot.com/3020433002 Committed: https://chromium.googlesource.com/catapult/+/e79820f09d19a90d428f9534ce80e472... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/catapult/+/e79820f09d19a90d428f9534ce80e472... |