Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2380183004:
Fail gracefully if no Chrome trace or insufficient power samples (reland) (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 (reland)
This is a reland of https://codereview.chromium.org/2371873002 to fix the
bug linked below. (The problem was that the tests use only the RAIL stage
tracing category, which includes only renderer events and not browser
events, so looking for the browser when computing bounds of Chrome trace was
throwing an error. I changed it to only consider renderer processes when
computing these bounds, since the metrics are based on RAIL stages and
only renderer events are relevant for RAIL stages.)
BUG=chromium:650306
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e87e17fbfb63cbe0e79be5c62ae7600bf320b62d
Patch Set 1 #Patch Set 2 : remove extraneous change #
Total comments: 22
Patch Set 3 : code cleanup #
Total comments: 4
Patch Set 4 : fix whitespace #
Messages
Total messages: 20 (8 generated)
alexandermont@chromium.org changed reviewers: + rnephew@chromium.org
rnephew@chromium.org changed reviewers: + benjhayden@chromium.org, eakuefner@chromium.org
Adding owners.
On 2016/09/30 19:50:17, rnephew (Reviews Here) wrote: > Adding owners. Also, just a tip for future CLs like this. If you upload the original version of the CL first, then upload a patchset with the changes its easier to review just the delta.
benjhayden@chromium.org changed reviewers: + nednguyen@google.com
lgtm but please wait for Ned to clarify whether the console.error()s should be exceptions instead. https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:316: console.error('Power metric error: power series ends before end of ' + @nednguyen: should these be thrown exceptions instead? I thought that we tried to limit console chatter. Both metrics-side-panel and telemetry have ways of handling and displaying thrown exceptions.
Description was changed from ========== Fail gracefully if no Chrome trace or insufficient power samples (reland) This is a reland of https://codereview.chromium.org/2371873002 to fix the bug linked below. (The problem was that the tests use only the RAIL stage tracing category, which includes only renderer events and not browser events, so looking for the browser when computing bounds of Chrome trace was throwing an error. I changed it to only consider renderer processes when computing these bounds, since the metrics are based on RAIL stages and only renderer events are relevant for RAIL stages.) BUG=chromium:650306 ========== to ========== Fail gracefully if no Chrome trace or insufficient power samples (reland) This is a reland of https://codereview.chromium.org/2371873002 to fix the bug linked below. (The problem was that the tests use only the RAIL stage tracing category, which includes only renderer events and not browser events, so looking for the browser when computing bounds of Chrome trace was throwing an error. I changed it to only consider renderer processes when computing these bounds, since the metrics are based on RAIL stages and only renderer events are relevant for RAIL stages.) BUG=chromium:650306 ==========
nednguyen@google.com changed reviewers: + charliea@chromium.org
On 2016/10/03 18:09:03, benjhayden wrote: > lgtm but please wait for Ned to clarify whether the console.error()s should be > exceptions instead. > > https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... > File tracing/tracing/metrics/system_health/power_metric.html (right): > > https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... > tracing/tracing/metrics/system_health/power_metric.html:316: > console.error('Power metric error: power series ends before end of ' + > @nednguyen: should these be thrown exceptions instead? > I thought that we tried to limit console chatter. Both metrics-side-panel and > telemetry have ways of handling and displaying thrown exceptions. +Charlie now he's back
On 2016/10/03 18:16:10, nednguyen wrote: > On 2016/10/03 18:09:03, benjhayden wrote: > > lgtm but please wait for Ned to clarify whether the console.error()s should be > > exceptions instead. > > > > > https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... > > File tracing/tracing/metrics/system_health/power_metric.html (right): > > > > > https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... > > tracing/tracing/metrics/system_health/power_metric.html:316: > > console.error('Power metric error: power series ends before end of ' + > > @nednguyen: should these be thrown exceptions instead? > > I thought that we tried to limit console chatter. Both metrics-side-panel and > > telemetry have ways of handling and displaying thrown exceptions. > > +Charlie now he's back I defer to Charlie on reviewing this
https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:271: function getChromeBounds(model) { In line with your other functions, my vote is to name this `computeChromeBounds()` to make it clear that some work needs to be done, we're not just returning something. https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:275: for (var pid in chromeHelper.rendererHelpers) { (Note: you know a lot more about this than I do, so you would know better than I would if this would be functionally equivalent for now) Could we add *both* the renderer ranges and the browser ranges? It seems like, if in the future there's some event that we rely on from the browser in order to calculate a power metric, we'd want the browser ranges in there too. The fact that we only rely on renderer events for now seems like more of a coincidence than anything else. https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:286: if (chromeHelper === undefined) { I think that, once you remove the console.errors as suggested by Ben below, this can be condensed. Also, in the spirit of using bounds checks, I think we should use "bounds.isEmpty" to check the powerSeries bounds rather than "samples.length === 0". Overall, that'd mean we get something like: // If we're missing any required traces, fail gracefully. if (!chromeHelper || !model.device.powerSeries || model.device.powerSeries.bounds.isEmpty) { return; } var chromeBounds = computeChromeBounds(model); if (chromeBounds.isEmpty) return; https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:292: if (!model.device.powerSeries) { nit: I'd recommend being consistent with how you check for `undefined`. Here, you just use if (!model.device.powerSeries), but above, you use if (chromeHelper === undefined). We don't really have a consistent style within trace viewer (we should, but we just haven't had that discussion yet), but it's a good idea to be consistent within a file. Otherwise, people will start wondering why you chose A in one place and B in another. https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:297: if (model.device.powerSeries.samples.length === 0) { Maybe it'd be useful to create a temp var for model.device.powerSeries, given how much you reference it? https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:297: if (model.device.powerSeries.samples.length === 0) { Maybe it'd be useful to create a temp var for model.device.powerSeries, given how much you reference it? https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:305: if (chromeBounds.min == undefined || chromeBounds.max == undefined) { I'd recommend using chromeBounds.isEmpty rather than checking against undefined. Also, you should always use triple equals (===, !==) comparisons instead of double equals in JS. I know this is hard to remember, especially with switching between Python and Javascript frequently. As with other problems that can be checked programmatically easily, the real problem here isn't that you made the mistake but the fact that the mistake wasn't caught by a presubmit. In that spirit, I filed a bug to make this caught by a presubmit: https://github.com/catapult-project/catapult/issues/2895 Maybe you can take that on after https://github.com/catapult-project/catapult/issues/2894 (which I ask about below - sorry, I'm jumping all around)? https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:310: if (model.device.powerSeries.bounds.min >= chromeBounds.min + 1.0) { Maybe this could be more naturally expressed with: // We must have power data for the entire Chrome trace, with a 1ms grace period // on both sides. var powerSeriesBoundsWithGracePeriod = Range.fromExplicitRange( powerSeries.bounds.min - 1, powerSeries.bounds.max + 1); if (!powerSeriesBoundsWithGracePeriod.containsRangeExclusive( chromeBounds)) { return; } ? https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:316: console.error('Power metric error: power series ends before end of ' + On 2016/10/03 at 18:09:02, benjhayden wrote: > @nednguyen: should these be thrown exceptions instead? > I thought that we tried to limit console chatter. Both metrics-side-panel and telemetry have ways of handling and displaying thrown exceptions. I agree with Ben here. From what I've seen, metrics should just not be added in the case where they're not applicable (like this). It's pretty safe to assume that console.warnings and console.errors are basically thrown out. https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:321: for(var exp of model.userModel.expectations) { nit: bad spacing (should be for (var ...) { ... } Would you be willing to take on https://github.com/catapult-project/catapult/issues/2894, which tracks turning on a presubmit to prevent stuff like this from making code review? It's an awesome way to multiply impact by turning something manual (code review) into something automatic (presubmit). https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric_test.html (right): https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric_test.html:46: args: {frame: '0xdeadbeef'} Just noticed this: why is there an "args" section here if we don't use it anywhere?
https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:271: function getChromeBounds(model) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > In line with your other functions, my vote is to name this `computeChromeBounds()` to make it clear that some work needs to be done, we're not just returning something. Done https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:275: for (var pid in chromeHelper.rendererHelpers) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > (Note: you know a lot more about this than I do, so you would know better than I would if this would be functionally equivalent for now) > > Could we add *both* the renderer ranges and the browser ranges? It seems like, if in the future there's some event that we rely on from the browser in order to calculate a power metric, we'd want the browser ranges in there too. The fact that we only rely on renderer events for now seems like more of a coincidence than anything else. Done. Added a check so it doesn't crash if there aren't any browser (or renderer) events. https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:286: if (chromeHelper === undefined) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > I think that, once you remove the console.errors as suggested by Ben below, this can be condensed. Also, in the spirit of using bounds checks, I think we should use "bounds.isEmpty" to check the powerSeries bounds rather than "samples.length === 0". Overall, that'd mean we get something like: > > // If we're missing any required traces, fail gracefully. > if (!chromeHelper || !model.device.powerSeries || > model.device.powerSeries.bounds.isEmpty) { > return; > } > > var chromeBounds = computeChromeBounds(model); > if (chromeBounds.isEmpty) return; console.errors removed. However, we're still throwing the exceptions in here, as benjhayden mentioned below. https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:286: if (chromeHelper === undefined) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > I think that, once you remove the console.errors as suggested by Ben below, this can be condensed. Also, in the spirit of using bounds checks, I think we should use "bounds.isEmpty" to check the powerSeries bounds rather than "samples.length === 0". Overall, that'd mean we get something like: > > // If we're missing any required traces, fail gracefully. > if (!chromeHelper || !model.device.powerSeries || > model.device.powerSeries.bounds.isEmpty) { > return; > } > > var chromeBounds = computeChromeBounds(model); > if (chromeBounds.isEmpty) return; Done https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:292: if (!model.device.powerSeries) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > nit: I'd recommend being consistent with how you check for `undefined`. Here, you just use if (!model.device.powerSeries), but above, you use if (chromeHelper === undefined). > > We don't really have a consistent style within trace viewer (we should, but we just haven't had that discussion yet), but it's a good idea to be consistent within a file. Otherwise, people will start wondering why you chose A in one place and B in another. Done https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:297: if (model.device.powerSeries.samples.length === 0) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > Maybe it'd be useful to create a temp var for model.device.powerSeries, given how much you reference it? Done https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:305: if (chromeBounds.min == undefined || chromeBounds.max == undefined) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > I'd recommend using chromeBounds.isEmpty rather than checking against undefined. > > Also, you should always use triple equals (===, !==) comparisons instead of double equals in JS. I know this is hard to remember, especially with switching between Python and Javascript frequently. As with other problems that can be checked programmatically easily, the real problem here isn't that you made the mistake but the fact that the mistake wasn't caught by a presubmit. In that spirit, I filed a bug to make this caught by a presubmit: https://github.com/catapult-project/catapult/issues/2895 > > Maybe you can take that on after https://github.com/catapult-project/catapult/issues/2894 (which I ask about below - sorry, I'm jumping all around)? Done https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:310: if (model.device.powerSeries.bounds.min >= chromeBounds.min + 1.0) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > Maybe this could be more naturally expressed with: > > // We must have power data for the entire Chrome trace, with a 1ms grace period > // on both sides. > var powerSeriesBoundsWithGracePeriod = Range.fromExplicitRange( > powerSeries.bounds.min - 1, powerSeries.bounds.max + 1); > if (!powerSeriesBoundsWithGracePeriod.containsRangeExclusive( > chromeBounds)) { > return; > } > > ? Done https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:316: console.error('Power metric error: power series ends before end of ' + On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > On 2016/10/03 at 18:09:02, benjhayden wrote: > > @nednguyen: should these be thrown exceptions instead? > > I thought that we tried to limit console chatter. Both metrics-side-panel and telemetry have ways of handling and displaying thrown exceptions. > > I agree with Ben here. From what I've seen, metrics should just not be added in the case where they're not applicable (like this). It's pretty safe to assume that console.warnings and console.errors are basically thrown out. Done https://codereview.chromium.org/2380183004/diff/20001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:321: for(var exp of model.userModel.expectations) { On 2016/10/04 at 14:48:58, charliea (OOO until 10-03-16) wrote: > nit: bad spacing (should be > > for (var ...) { > ... > } > > Would you be willing to take on https://github.com/catapult-project/catapult/issues/2894, which tracks turning on a presubmit to prevent stuff like this from making code review? It's an awesome way to multiply impact by turning something manual (code review) into something automatic (presubmit). Done
lgtm w/ comments https://codereview.chromium.org/2380183004/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2380183004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:300: throw new PowerMetricException('No Chrome trace'); (here and elsewhere): there are a couple of things you should probably do to make these exceptions more "trace-viewery" 1) I think that in a lot of these cases, we could make it clearer about *why* the exception is being thrown. For example, look at "No power series available". This makes it clear what the condition resulting in the exception was, but it's not clear *why* that condition is problematic, which makes it confusing to anyone who sees the error down the road. More helpful text might be "Can't calculate power metric due to missing power data", which makes it clear both what the condition is and why that condition is resulting in an exception. This future-proofs the exception. 2) Rather than creating your own exception type (common in Python), I'd just do "throw new Error" with the more specific text above (common in trace viewer). See this code search (https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/...) for how common this idiom is in trace viewer. Hope that helps. https://codereview.chromium.org/2380183004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:322: chromeBounds)) { nit: line continuation should be four spaces, not two (Also went ahead and filed https://github.com/catapult-project/catapult/issues/2909 so that one of us can enable this as a presubmit check)
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, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/2380183004/#ps60001 (title: "fix whitespace")
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 (reland) This is a reland of https://codereview.chromium.org/2371873002 to fix the bug linked below. (The problem was that the tests use only the RAIL stage tracing category, which includes only renderer events and not browser events, so looking for the browser when computing bounds of Chrome trace was throwing an error. I changed it to only consider renderer processes when computing these bounds, since the metrics are based on RAIL stages and only renderer events are relevant for RAIL stages.) BUG=chromium:650306 ========== to ========== Fail gracefully if no Chrome trace or insufficient power samples (reland) This is a reland of https://codereview.chromium.org/2371873002 to fix the bug linked below. (The problem was that the tests use only the RAIL stage tracing category, which includes only renderer events and not browser events, so looking for the browser when computing bounds of Chrome trace was throwing an error. I changed it to only consider renderer processes when computing these bounds, since the metrics are based on RAIL stages and only renderer events are relevant for RAIL stages.) BUG=chromium:650306 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
https://codereview.chromium.org/2380183004/diff/40001/tracing/tracing/metrics... File tracing/tracing/metrics/system_health/power_metric.html (right): https://codereview.chromium.org/2380183004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:300: throw new PowerMetricException('No Chrome trace'); On 2016/10/06 at 16:27:56, charliea wrote: > (here and elsewhere): there are a couple of things you should probably do to make these exceptions more "trace-viewery" > > 1) I think that in a lot of these cases, we could make it clearer about *why* the exception is being thrown. For example, look at "No power series available". This makes it clear what the condition resulting in the exception was, but it's not clear *why* that condition is problematic, which makes it confusing to anyone who sees the error down the road. More helpful text might be "Can't calculate power metric due to missing power data", which makes it clear both what the condition is and why that condition is resulting in an exception. This future-proofs the exception. > > 2) Rather than creating your own exception type (common in Python), I'd just do "throw new Error" with the more specific text above (common in trace viewer). See this code search (https://cs.corp.google.com/search/?q=p:github+f:%5Ecatapult-project/catapult/...) for how common this idiom is in trace viewer. > > Hope that helps. Done https://codereview.chromium.org/2380183004/diff/40001/tracing/tracing/metrics... tracing/tracing/metrics/system_health/power_metric.html:322: chromeBounds)) { On 2016/10/06 at 16:27:56, charliea wrote: > nit: line continuation should be four spaces, not two > > (Also went ahead and filed https://github.com/catapult-project/catapult/issues/2909 so that one of us can enable this as a presubmit check) done |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
