Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1980773002:
[BattOr][Telemetry] Changes needed to run BattOr tests on telemetry. (Closed)
|
Created:
4 years, 7 months ago by rnephew (Reviews Here) Modified:
4 years, 7 months ago CC:
catapult-reviews_chromium.org, telemetry-reviews_chromium.org Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[BattOr][Telemetry] Changes needed to run BattOr tests on telemetry.
Small changes required to push battor data through to telemetry results and disable charging.
BUG=catapult:#2309
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/20d5ca9cc4f3c1612f8a0e29e4d5cfa6f146cbfc
Patch Set 1 : #Patch Set 2 : Add disabling of charging #
Total comments: 2
Patch Set 3 : add test #
Total comments: 10
Patch Set 4 : #
Total comments: 8
Patch Set 5 : #
Messages
Total messages: 34 (5 generated)
Patchset #1 (id:1) has been deleted
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com
On 2016/05/13 22:19:11, rnephew1 wrote: Looks like the tests are failing...
On 2016/05/13 22:35:44, nednguyen wrote: > On 2016/05/13 22:19:11, rnephew1 wrote: > > Looks like the tests are failing... That failure should be fixed now.
Description was changed from ========== [BattOr][Telemetry] Changes needed to pass BattOr data through telemetry. Small changes required to push battor data through to telemetry results. BUG=catapult:#2309 ========== to ========== [BattOr][Telemetry] Changes needed to run BattOr tests on telemetry. Small changes required to push battor data through to telemetry results and disable charging. BUG=catapult:#2309 ==========
lgtm
https://codereview.chromium.org/1980773002/diff/40001/telemetry/telemetry/val... File telemetry/telemetry/value/trace.py (right): https://codereview.chromium.org/1980773002/diff/40001/telemetry/telemetry/val... telemetry/telemetry/value/trace.py:48: if trace_data.HasEventsFor(p)] Can you add test coverage for this? S.t like: trace_data = ... trace_value = TraceValue(trace_data) assert trace_value.file ...
https://codereview.chromium.org/1980773002/diff/40001/telemetry/telemetry/val... File telemetry/telemetry/value/trace.py (right): https://codereview.chromium.org/1980773002/diff/40001/telemetry/telemetry/val... telemetry/telemetry/value/trace.py:48: if trace_data.HasEventsFor(p)] On 2016/05/14 16:26:24, nednguyen wrote: > Can you add test coverage for this? S.t like: > trace_data = ... > trace_value = TraceValue(trace_data) > > assert trace_value.file ... Done.
nolgtm https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:75: self._battery.SetCharging(True) Is this needed? Aren't we already going to reenable charging at exit? https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:22: def SetCharging(self, state): Doesn't this need to include self._charging_state = state? https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:172: Shouldn't we have a test to make sure that we're actually setting the charging state correctly?
notlgtm? I have no idea how to use this feature...
https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:75: self._battery.SetCharging(True) On 2016/05/16 17:59:29, charliea wrote: > Is this needed? Aren't we already going to reenable charging at exit? Enabling charging at exit is done as a last resource action. In general, the callsite should be responsible for cleaning up the resource as so on.
https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/tim... File telemetry/telemetry/timeline/trace_data.py (right): https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/tim... telemetry/telemetry/timeline/trace_data.py:188: def AddEventsTo(self, part, events, as_string=False): Why not just auto detect whether it's a string or a list and throw an exception if it's neither? It seems like if we're able to check if it's correct with assertions then the as_string parameter is unnecessary anyhow.
On 2016/05/16 18:01:54, nednguyen wrote: > https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... > File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py > (right): > > https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:75: > self._battery.SetCharging(True) > On 2016/05/16 17:59:29, charliea wrote: > > Is this needed? Aren't we already going to reenable charging at exit? > > Enabling charging at exit is done as a last resource action. In general, the > callsite should be responsible for cleaning up the resource as so on. Ah, okay. What's the reasoning behind that? It seems like it makes sense that, if we're already sure that we can guarantee reenabling at exit using the atexit code, we should just do that and get rid of the code that doesn't do anything?
https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:75: self._battery.SetCharging(True) On 2016/05/16 18:01:54, nednguyen wrote: > On 2016/05/16 17:59:29, charliea wrote: > > Is this needed? Aren't we already going to reenable charging at exit? > > Enabling charging at exit is done as a last resource action. In general, the > callsite should be responsible for cleaning up the resource as so on. > Ah, okay. What's the reasoning behind that? It seems like it makes > sense that, if we're already sure that we can guarantee reenabling at > exit using the atexit code, we should just do that and get rid of the > code that doesn't do anything? atexit is only run at the end of python process (except when SystemExit error is thrown, https://docs.python.org/2/library/atexit.html) hence we can count on it completely either. On another hand, a telemetry benchmark that run 5 stories can look s.t like this: Disable charge run story 1 Enable Charge until full Disable charge run story 2 Enable Charge until full Disable charge ... We may want the Disable charge interleaving here to ensure the stability of the benchmark run, hence can't count of atexit either.
https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:75: self._battery.SetCharging(True) On 2016/05/16 18:16:34, nednguyen wrote: > On 2016/05/16 18:01:54, nednguyen wrote: > > On 2016/05/16 17:59:29, charliea wrote: > > > Is this needed? Aren't we already going to reenable charging at exit? > > > > Enabling charging at exit is done as a last resource action. In general, the > > callsite should be responsible for cleaning up the resource as so on. > > > Ah, okay. What's the reasoning behind that? It seems like it makes > > sense that, if we're already sure that we can guarantee reenabling at > exit > using the atexit code, we should just do that and get rid of the > code that > doesn't do anything? > > atexit is only run at the end of python process (except when SystemExit error is > thrown, https://docs.python.org/2/library/atexit.html) hence we can count on it > completely either. > > On another hand, a telemetry benchmark that run 5 stories can look s.t like > this: > > Disable charge > run story 1 > Enable Charge until full > Disable charge > run story 2 > Enable Charge until full > Disable charge > ... > > We may want the Disable charge interleaving here to ensure the stability of the > benchmark run, hence can't count of atexit either. Also, more time charging is strictly better, since if they dip too low it will wait until the phone is charged to continue a test run. https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:22: def SetCharging(self, state): On 2016/05/16 17:59:29, charliea wrote: > Doesn't this need to include self._charging_state = state? Done. https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:172: On 2016/05/16 17:59:29, charliea wrote: > Shouldn't we have a test to make sure that we're actually setting the charging > state correctly? Done. https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/tim... File telemetry/telemetry/timeline/trace_data.py (right): https://codereview.chromium.org/1980773002/diff/60001/telemetry/telemetry/tim... telemetry/telemetry/timeline/trace_data.py:188: def AddEventsTo(self, part, events, as_string=False): On 2016/05/16 18:03:48, charliea wrote: > Why not just auto detect whether it's a string or a list and throw an exception > if it's neither? It seems like if we're able to check if it's correct with > assertions then the as_string parameter is unnecessary anyhow. I dont foresee as_string being used very much so I didn't want to accept strings by default, so I wanted the user to specifically call out that they wanted it as a string.
ping.
https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/val... File telemetry/telemetry/value/trace_unittest.py (right): https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/val... telemetry/telemetry/value/trace_unittest.py:116: self.assertListEqual(['Battor Data', 'Chrome Data', 'Tab Data'], parts) Instead of assert on v._GetTraceParts(data) (test the private implementation), I would assert v.filename contains appropriate data (test the public API).
https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/tim... File telemetry/telemetry/timeline/trace_data.py (right): https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/tim... telemetry/telemetry/timeline/trace_data.py:188: def AddEventsTo(self, part, events, as_string=False): Why is the as_string parameter necessary here? Can't we just autodetect whether |events| is a basestring or a list? https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/val... File telemetry/telemetry/value/trace_unittest.py (right): https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/val... telemetry/telemetry/value/trace_unittest.py:116: self.assertListEqual(['Battor Data', 'Chrome Data', 'Tab Data'], parts) On 2016/05/19 16:42:34, nednguyen wrote: > Instead of assert on v._GetTraceParts(data) (test the private implementation), I > would assert v.filename contains appropriate data (test the public API). +1 (Might be worth noting: running against the filesystem is generally not recommended in unit tests because unit tests should be really, really fast (O(ms) or faster). In google3 code, you can get around this by using an in-memory file system in unit tests, but that's probably not worth it here unless Chromium also has something similar available.)
https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/tim... File telemetry/telemetry/timeline/trace_data.py (right): https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/tim... telemetry/telemetry/timeline/trace_data.py:188: def AddEventsTo(self, part, events, as_string=False): On 2016/05/19 17:58:44, charliea wrote: > Why is the as_string parameter necessary here? Can't we just autodetect whether > |events| is a basestring or a list? The only time we want it to accept a string is for BattOrData (as of right now). Everything else is expected to be a list. This flag is kind of a 'I know what I am doing let it happen' flag. https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/val... File telemetry/telemetry/value/trace_unittest.py (right): https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/val... telemetry/telemetry/value/trace_unittest.py:116: self.assertListEqual(['Battor Data', 'Chrome Data', 'Tab Data'], parts) On 2016/05/19 16:42:34, nednguyen wrote: > Instead of assert on v._GetTraceParts(data) (test the private implementation), I > would assert v.filename contains appropriate data (test the public API). Done.
lgtm
https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/val... File telemetry/telemetry/value/trace_unittest.py (right): https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/val... telemetry/telemetry/value/trace_unittest.py:116: self.assertListEqual(['Battor Data', 'Chrome Data', 'Tab Data'], parts) On 2016/05/19 17:58:44, charliea wrote: > On 2016/05/19 16:42:34, nednguyen wrote: > > Instead of assert on v._GetTraceParts(data) (test the private implementation), > I > > would assert v.filename contains appropriate data (test the public API). > > +1 > > (Might be worth noting: running against the filesystem is generally not > recommended in unit tests because unit tests should be really, really fast > (O(ms) or faster). In google3 code, you can get around this by using an > in-memory file system in unit tests, but that's probably not worth it here > unless Chromium also has something similar available.) We have pyfakefs in telemetry/third_party. However, after using it for a while, we realize that it doesn't suppport windows well & just ignore it. The main overhead in our telemetry unittests are around starting the browsers. These file I/O cost are pretty small compared to those hence are ok to ignore :)
https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/tim... File telemetry/telemetry/timeline/trace_data.py (right): https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/tim... telemetry/telemetry/timeline/trace_data.py:188: def AddEventsTo(self, part, events, as_string=False): On 2016/05/19 18:27:41, rnephew (Reviews Here) wrote: > On 2016/05/19 17:58:44, charliea wrote: > > Why is the as_string parameter necessary here? Can't we just autodetect > whether > > |events| is a basestring or a list? > > The only time we want it to accept a string is for BattOrData (as of right now). > Everything else is expected to be a list. This flag is kind of a 'I know what I > am doing let it happen' flag. In the future, though, we know that we'll be adding atrace, so it's likely there are going to be other users to this. It just seems like we either want to allow it or we don't. I kind of get the argument that it's a safety check to make sure that people don't accidentally add strings they don't want, but I wonder if that safety check wouldn't be better done by splitting the method entirely (AddEventsListTo, AddEventsStringTo). That's definitely a personal preference, but I'm generally against these bool do_something_else parameters just because they can get kind of annoying and can make the calling code hard to read. If Ned's good with it though, I am too.
lgtm
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980773002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980773002/100001
Message was sent while issue was closed.
Description was changed from ========== [BattOr][Telemetry] Changes needed to run BattOr tests on telemetry. Small changes required to push battor data through to telemetry results and disable charging. BUG=catapult:#2309 ========== to ========== [BattOr][Telemetry] Changes needed to run BattOr tests on telemetry. Small changes required to push battor data through to telemetry results and disable charging. BUG=catapult:#2309 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/tim... File telemetry/telemetry/timeline/trace_data.py (right): https://codereview.chromium.org/1980773002/diff/80001/telemetry/telemetry/tim... telemetry/telemetry/timeline/trace_data.py:188: def AddEventsTo(self, part, events, as_string=False): On 2016/05/19 19:09:01, charliea wrote: > On 2016/05/19 18:27:41, rnephew (Reviews Here) wrote: > > On 2016/05/19 17:58:44, charliea wrote: > > > Why is the as_string parameter necessary here? Can't we just autodetect > > whether > > > |events| is a basestring or a list? > > > > The only time we want it to accept a string is for BattOrData (as of right > now). > > Everything else is expected to be a list. This flag is kind of a 'I know what > I > > am doing let it happen' flag. > > In the future, though, we know that we'll be adding atrace, so it's likely there > are going to be other users to this. It just seems like we either want to allow > it or we don't. I kind of get the argument that it's a safety check to make sure > that people don't accidentally add strings they don't want, but I wonder if that > safety check wouldn't be better done by splitting the method entirely > (AddEventsListTo, AddEventsStringTo). That's definitely a personal preference, > but I'm generally against these bool do_something_else parameters just because > they can get kind of annoying and can make the calling code hard to read. > > If Ned's good with it though, I am too. Oops, I missed this part. I am with Charlie on don't use parameter to signal type. I would just do thing differently based on the type of events here.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/1999073002/ by skyostil@chromium.org. The reason for reverting is: Reverting since this is causing redness on the waterfall (tested with a local revert): https://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%283%29/bu... Note that https://codereview.chromium.org/1998673003/ also needed to be reverted..
Message was sent while issue was closed.
On 2016/05/20 13:03:26, Sami wrote: > A revert of this CL (patchset #5 id:100001) has been created in > https://codereview.chromium.org/1999073002/ by mailto:skyostil@chromium.org. > > The reason for reverting is: Reverting since this is causing redness on the > waterfall (tested with a local revert): > > https://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%283%29/bu... > > Note that https://codereview.chromium.org/1998673003/ also needed to be > reverted.. Randy, when you try to reland this, can you look into why timeline_based_page_test_unittest smoke test did not catch this? (https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...)
Message was sent while issue was closed.
> Randy, when you try to reland this, can you look into why > timeline_based_page_test_unittest smoke test did not catch this? > (https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...) Looking at: https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... It looks like it doens't run timeline_based_page_test_unittest.TimelineBasedPageTestTest. If It did I am relatively sure that testTBM2ForSmoke would have caught it.
Message was sent while issue was closed.
On 2016/05/24 17:16:10, rnephew (Reviews Here) wrote: > > Randy, when you try to reland this, can you look into why > > timeline_based_page_test_unittest smoke test did not catch this? > > > (https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...) > > Looking at: > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > It looks like it doens't run > timeline_based_page_test_unittest.TimelineBasedPageTestTest. If It did I am > relatively sure that testTBM2ForSmoke would have caught it. We do run timeline_based_page_test_unittest on telemetry_unittest in chromium CQ & catapult CQ though
Message was sent while issue was closed.
On 2016/05/24 17:21:28, nednguyen wrote: > On 2016/05/24 17:16:10, rnephew (Reviews Here) wrote: > > > Randy, when you try to reland this, can you look into why > > > timeline_based_page_test_unittest smoke test did not catch this? > > > > > > (https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...) > > > > Looking at: > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi... > > > > It looks like it doens't run > > timeline_based_page_test_unittest.TimelineBasedPageTestTest. If It did I am > > relatively sure that testTBM2ForSmoke would have caught it. > > We do run timeline_based_page_test_unittest on telemetry_unittest in chromium CQ > & catapult CQ though Ah. It looks like none of them enable chrome tracing, so there is no clock domain conflict. I will add this to the reland CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
