Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2371873002:
Fail gracefully if no Chrome trace or insufficient power samples. (Closed)
|
Created:
4 years, 2 months ago by alexandermont Modified:
4 years, 2 months ago CC:
catapult-reviews_chromium.org, tracing-review_chromium.org Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionFail gracefully if no Chrome trace or insufficient power samples.
If there is no Chrome trace available or power samples do not cover
the entire region, return no metrics rather than crashing or
returning bogus metric values.
BUG=catapult:#2867
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/5c9f7723faa3ba50ff48094ae5dd4d00cfaaad1a
Patch Set 1 #Patch Set 2 : fix semicolons #
Total comments: 6
Patch Set 3 : fix whitespace #
Total comments: 2
Patch Set 4 : fix brace style and add browser process as well as renderer process #
Total comments: 1
Patch Set 5 : rebase #Patch Set 6 : add logging #
Total comments: 1
Patch Set 7 : change console.log to console.error #Patch Set 8 : rebase #
Messages
Total messages: 41 (15 generated)
alexandermont@chromium.org changed reviewers: + benjhayden@chromium.org, eakuefner@chromium.org, nednguyen@google.com
https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:274: // (add 1 ms grace period at each end) How do these happen?
Couple questions. :-) https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:273: // Fail gracefully if the power series doesn't cover the entire trace. Does this work with traces that include telemetry py_trace events? Would it be better to just only consider the part of the trace that does have power series? https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:275: model.updateBounds(); Is this necessary? Model importing should take care of this, right? https://github.com/catapult-project/catapult/blob/2fae0f4cec8b8ae76847d17686a...
https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:273: // Fail gracefully if the power series doesn't cover the entire trace. On 2016/09/26 at 19:48:37, benjhayden wrote: > Does this work with traces that include telemetry py_trace events? > Would it be better to just only consider the part of the trace that does have power series? Changed it so that it looks for the Chrome trace and only considers the Chrome section of the trace. Power series must continue over the entire Chrome section of the trace, and error will be thrown if this does not occur. https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:274: // (add 1 ms grace period at each end) On 2016/09/26 at 19:46:58, nednguyen wrote: > How do these happen? I just added this in case, e.g. you stop the Battor and the Chrome at the same time, but the last Battor sample was taken a fraction of a millisecond before you stopped the BattOr trace, then it wouldn't see that fraction of a millisecond with no data and think that invalidates the whole data set. https://codereview.chromium.org/2371873002/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:275: model.updateBounds(); On 2016/09/26 at 19:48:37, benjhayden wrote: > Is this necessary? Model importing should take care of this, right? > https://github.com/catapult-project/catapult/blob/2fae0f4cec8b8ae76847d17686a... This is fixed by the new update. I now use TestUtils.newModel to create the model.
https://codereview.chromium.org/2371873002/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2371873002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:269: for (var pid in chromeHelper.rendererHelpers) Why not use the browser processes's main thread instead of the renderer main threads? The style guide for curly braces was recently changed: https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#...
https://codereview.chromium.org/2371873002/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2371873002/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:269: for (var pid in chromeHelper.rendererHelpers) On 2016/09/28 at 19:47:18, benjhayden wrote: > Why not use the browser processes's main thread instead of the renderer main threads? > > The style guide for curly braces was recently changed: https://github.com/catapult-project/catapult/blob/master/docs/style-guide.md#... Added use of browser process in addition to renderer process. Fixed style.
lgtm
The CQ bit was checked by alexandermont@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...) Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...) Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
https://codereview.chromium.org/2371873002/diff/60001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2371873002/diff/60001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:287: // (add 1 ms grace period at each end) I still don't understand why we need to fail gracefully in this case. Which math blows up if this happen? Can we just add some padding value for power series, s.t like the median of nearest 100 power data points?
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2371873002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by nednguyen@google.com
On 2016/09/28 22:19:55, nednguyen wrote: > https://codereview.chromium.org/2371873002/diff/60001/tracing/tracing/metrics... > File tracing/tracing/metrics/system_health/power_metric.html (right): > > https://codereview.chromium.org/2371873002/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/power_metric.html:287: // (add 1 ms grace > period at each end) > I still don't understand why we need to fail gracefully in this case. Which math > blows up if this happen? Can we just add some padding value for power series, > s.t like the median of nearest 100 power data points? Sorry, can you answer my question here before landing?
On 2016/09/28 at 22:19:55, nednguyen wrote: > https://codereview.chromium.org/2371873002/diff/60001/tracing/tracing/metrics... > File tracing/tracing/metrics/system_health/power_metric.html (right): > > https://codereview.chromium.org/2371873002/diff/60001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/power_metric.html:287: // (add 1 ms grace period at each end) > I still don't understand why we need to fail gracefully in this case. Which math blows up if this happen? Can we just add some padding value for power series, s.t like the median of nearest 100 power data points? It's not so much that the "math blows up," it's that the metric just isn't meaningful in that case. For instance, let's say that a trace has a loading period and then an idle period, but through most of the loading period no power data was being collected. Then whatever "padding value" we use, most of the "energy for loading" that we report will come from this "padding" value rather than from actual power samples, so the measurement will effectively be a guess rather than a real measurement. Given that we only expect this situation to occur if there's a problem where the trace doesn't start when expected, it's better th not give any data here, so the user will see that there's a problem with the data collection, rather than report data which could be bogus.
On 2016/09/29 18:02:43, alexandermont wrote: > On 2016/09/28 at 22:19:55, nednguyen wrote: > > > https://codereview.chromium.org/2371873002/diff/60001/tracing/tracing/metrics... > > File tracing/tracing/metrics/system_health/power_metric.html (right): > > > > > https://codereview.chromium.org/2371873002/diff/60001/tracing/tracing/metrics... > > tracing/tracing/metrics/system_health/power_metric.html:287: // (add 1 ms > grace period at each end) > > I still don't understand why we need to fail gracefully in this case. Which > math blows up if this happen? Can we just add some padding value for power > series, s.t like the median of nearest 100 power data points? > > It's not so much that the "math blows up," it's that the metric just isn't > meaningful in that case. For instance, let's say that a trace has a loading > period and then an idle period, but through most of the loading period no power > data was being collected. Then whatever "padding value" we use, most of the > "energy for loading" that we report will come from this "padding" value rather > than from actual power samples, so the measurement will effectively be a guess > rather than a real measurement. Given that we only expect this situation to > occur if there's a problem where the trace doesn't start when expected, it's > better th not give any data here, so the user will see that there's a problem > with the data collection, rather than report data which could be bogus. I like your idea of "data integrity". However, can you add logging.error(...) for both min & max bounds check fail so that the consumer of metrics now that what has happened? lgtm
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2371873002/#ps100001 (title: "add logging")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by nednguyen@google.com
https://codereview.chromium.org/2371873002/diff/100001/tracing/tracing/metric... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2371873002/diff/100001/tracing/tracing/metric... tracing/tracing/metrics/system_health/power_metric.html:287: console.log('Power metric error: no Chrome trace.'); to indicate error, you can do: console.error('no Chrome trace') Same for the other one
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2371873002/#ps120001 (title: "change console.log to console.error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/2371873002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fail gracefully if no Chrome trace or insufficient power samples. If there is no Chrome trace available or power samples do not cover the entire region, return no metrics rather than crashing or returning bogus metric values. BUG=catapult:#2867 ========== to ========== Fail gracefully if no Chrome trace or insufficient power samples. If there is no Chrome trace available or power samples do not cover the entire region, return no metrics rather than crashing or returning bogus metric values. BUG=catapult:#2867 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2376193005/ by rnephew@chromium.org. The reason for reverting is: crbug.com/650306 Causing BattOr tests to fail with: MapFunctionError: Cannot read property 'bounds' of undefined. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
