Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1819183002:
[Telemetry] Add battor tracing agent (Closed)
|
Created:
4 years, 9 months ago by rnephew (Reviews Here) Modified:
4 years, 7 months ago CC:
catapult-reviews_chromium.org, Zhen Wang Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Telemetry] Add battor tracing agent.
BUG=chromium:574888
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/e166442cd31d5e31bbcb5b7d105b6dac79c4f70a
Patch Set 1 #
Total comments: 25
Patch Set 2 : small fixes #
Total comments: 30
Patch Set 3 : #Patch Set 4 : dependency config #
Total comments: 2
Patch Set 5 : nix local_paths #
Total comments: 4
Patch Set 6 : bucket/folder in config #
Total comments: 1
Patch Set 7 : dependency manager added #
Total comments: 11
Patch Set 8 : #
Total comments: 13
Patch Set 9 : #
Total comments: 12
Patch Set 10 : #
Total comments: 24
Patch Set 11 : #
Total comments: 6
Patch Set 12 : #Patch Set 13 : change dep manager config #Patch Set 14 : Monkey Patch Agent Iter #Patch Set 15 : #
Total comments: 2
Patch Set 16 : #Patch Set 17 : #
Total comments: 11
Patch Set 18 : #Patch Set 19 : other cl landed #
Total comments: 4
Patch Set 20 : #Patch Set 21 : #
Total comments: 18
Patch Set 22 : #
Total comments: 4
Patch Set 23 : #Messages
Total messages: 89 (24 generated)
rnephew@chromium.org changed reviewers: + aiolos@chromium.org, alexandermont@chromium.org, nednguyen@google.com, zhenw@chromium.org
This is mostly done, I just need a few questions answered to finish it up and need to write unit tests. https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:8: #from devil.utils import find_usb_devices Alex, what is the best way to go from device_id to battor_address usign this tool? https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:50: #TODO(rnephew): Process config and return based off that. Nedn/Zhenw: What will the trace_option for enabling battor look like? https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:26: '../../../../../../../../../chromium_vanilla/src/out/Default/' aiolos: This is going to deal with dependency manager; can you send me an example of how to pull this from dependency manager?
https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:50: #TODO(rnephew): Process config and return based off that. On 2016/03/21 23:18:17, rnephew1 wrote: > Nedn/Zhenw: What will the trace_option for enabling battor look like? For Chrome, enabling system tracing will enable battor automatically (if battor is connected). Do we want to follow the same convention?
https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:8: #from devil.utils import find_usb_devices On 2016/03/21 at 23:18:17, rnephew1 wrote: > Alex, what is the best way to go from device_id to battor_address usign this tool? See example at: https://codereview.chromium.org/1822893002 We might want to pull out this functionality into a separate function to go into find_usb_devices.py, and then have both this and the systrace battor_tracing_agent call that. https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:48: battor_cmd.append('--bator-path=%s' % self._battor_path) ' --battor-path=%s' (and note the space at beginning) https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:51: assert self._battor_shell.poll() is None, 'Shell did not start properly.' extra space between assert and self https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:60: assert self._tracing, 'Must be run StartTracing before StopTracing' 'Must be run' --> 'Must run' https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:61: # TODO(rnephew): Change this when dumping to file is complete. It's not actually necessary to use the 'dumping to file' option; you could just keep it the way it is and avoid the extra step of battor_agent_binary --> file --> results. The purpose of the 'dumping to file' option is mainly when the user is using it interactively - so the user can see the results of each command (like the "Done.") printed to the screen, but then have the trace output printed to the file instead. https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:74: return bool(self._SendBattorCommand(self._SUPPORTS_CLOCKSYNC_CMD, Should use bool(int(...)) instead of bool(...). In current version, if the command returns a '0', that will show up as true since bool('0') = true.
https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:8: #from devil.utils import find_usb_devices On 2016/03/21 23:47:54, alexandermont wrote: > On 2016/03/21 at 23:18:17, rnephew1 wrote: > > Alex, what is the best way to go from device_id to battor_address usign this > tool? > > See example at: > > https://codereview.chromium.org/1822893002 > > We might want to pull out this functionality into a separate function to go into > find_usb_devices.py, and then have both this and the systrace > battor_tracing_agent call that. +1 https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:26: '../../../../../../../../../chromium_vanilla/src/out/Default/' On 2016/03/21 23:18:18, rnephew1 wrote: > aiolos: This is going to deal with dependency manager; can you send me an > example of how to pull this from dependency manager? You should probably look at the binary_manager in telemetry. This doc is still unfinished, but has links to the examples you'll want to look at: https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... Let me know if you have any questions.
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:48: battor_cmd.append('--bator-path=%s' % self._battor_path) On 2016/03/21 23:47:54, alexandermont wrote: > ' --battor-path=%s' (and note the space at beginning) Doesn't it automatically assume spaces between command arguments passed in as lists? https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:61: # TODO(rnephew): Change this when dumping to file is complete. On 2016/03/21 23:47:54, alexandermont wrote: > It's not actually necessary to use the 'dumping to file' option; you could just > keep it the way it is and avoid the extra step of battor_agent_binary --> file > --> results. The purpose of the 'dumping to file' option is mainly when the user > is using it interactively - so the user can see the results of each command > (like the "Done.") printed to the screen, but then have the trace output printed > to the file instead. I remember there being talk of wanting to separate StopTracing and collection of results in order to minimize the length difference of different traces. I was envisioning this going something like: Call StopTracing Battor stops tracing battor agent returns done battor agent dumps results and while it is dumping results the other agents are being stopped. Then a collection pass. Zhen, Ned; let me know if I am remembering wrong about the separating of stopping and collecting.
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:26: '../../../../../../../../../chromium_vanilla/src/out/Default/' On 2016/03/22 00:02:43, aiolos wrote: > On 2016/03/21 23:18:18, rnephew1 wrote: > > aiolos: This is going to deal with dependency manager; can you send me an > > example of how to pull this from dependency manager? > > You should probably look at the binary_manager in telemetry. This doc is still > unfinished, but has links to the examples you'll want to look at: > https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... > > Let me know if you have any questions. I think this work will be blocked on having the battor_agent_binary being added to the dependency manager. Adding Charlie to this to see if this is the case; looking at the configs I do not see anything about it. https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:51: assert self._battor_shell.poll() is None, 'Shell did not start properly.' On 2016/03/21 23:47:54, alexandermont wrote: > extra space between assert and self Done. https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:60: assert self._tracing, 'Must be run StartTracing before StopTracing' On 2016/03/21 23:47:54, alexandermont wrote: > 'Must be run' --> 'Must run' Done. https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:74: return bool(self._SendBattorCommand(self._SUPPORTS_CLOCKSYNC_CMD, On 2016/03/21 23:47:54, alexandermont wrote: > Should use bool(int(...)) instead of bool(...). In current version, if the > command returns a '0', that will show up as true since bool('0') = true. Done.
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:48: battor_cmd.append('--bator-path=%s' % self._battor_path) On 2016/03/22 at 00:06:48, rnephew1 wrote: > On 2016/03/21 23:47:54, alexandermont wrote: > > ' --battor-path=%s' (and note the space at beginning) > > Doesn't it automatically assume spaces between command arguments passed in as lists? You are correct here. I didn't see that it was a list and was thinking it was string concatenation.
rnephew@chromium.org changed reviewers: + charliea@chromium.org
https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:8: #from devil.utils import find_usb_devices Commented out import? https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:15: """ A Battor tracing agent. """ None of the pydocs I can find have leading/trailing spaces like this one does. Is this normal? https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:15: """ A Battor tracing agent. """ In general, Chromium-style comments generally have to have the form of a complete sentence. Also, I think we try our hardest to avoid these types of tautological comments (Of *course* a BattOrTracingAgent is A BattOr tracing agent. But what the heck is a tracing agent? What's a BattOr? Given that this is probably the first BattOr-related file that someone browsing Telemetry is apt to come across, I think it makes sense to give some more context). Maybe something like: BattOrTracingAgent allows Telemetry to issue high-level tracing commands (StartTracing, StopTracing, RecordClockSyncMarker) to BattOrs, which are high-frequency power monitors used for battery testing. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:27: """ Returns if battor is supported. """ Leading/trailing spaces https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:29: return True # if the mapping to device_id to battor works. Someone else might know better, but same comment as above about Chromium comments generally being complete sentences https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:31: return True # if there is only one battor attached. Comment complete sentence https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:35: """ Start tracing on the battor. Leading/trailing spaces https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:38: config: tracing_config instance that contains trace_option and Is it trace_option or trace_options? You call it different things here and below https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:38: config: tracing_config instance that contains trace_option and The description of just config: doesn't seem to provide much value. If the way that you've documented trace_options and category_filter is standard pydoc, then it seems like it'd be fair to just get rid of the config: description and have: config: trace_options: An instance of trace_options.TracingOptions that controls which core tracing systems should be enabled. category_filter: An instance of tracing_category_filter.TracingCategoryFilter https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:41: control which core tracing systems should be enabled. s/control/controls https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:43: tracing_category_filter.TracingCategoryFilter Is it normal pydoc to just describe the type of the parameter without more context about its purpose in the method? https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:50: #TODO(rnephew): Process config and return based off that. I think that this should be: # TODO(rnephew)... (Note space after #) https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:55: """ Stop tracing on the battor. """ Leading/trailing spaces https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:57: # Merge results into trace_data_builder here This should probably be a TODO(rnephew): https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:64: """ Record clock sync marker on battor. Leading/trailing spaces https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:69: and a timestamp as argument. Those are the parameters, but what does the function actually do?
Also, one more nit: in general, most patch names I see don't have the trailing period
Description was changed from ========== [Telemetry] Add battor tracing agent. Also adds battor communication wrapper. BUG=chromium:574888 ========== to ========== [Telemetry] Add battor tracing agent. Also adds battor communication wrapper. BUG=chromium:574888 ==========
https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:8: #from devil.utils import find_usb_devices On 2016/03/22 18:01:42, charliea wrote: > Commented out import? This CL is still a WIP, I uploaded it to give context to asking people questions. This will not be commented out in its final form. I should have mentioned the WIP status; my bad. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:15: """ A Battor tracing agent. """ On 2016/03/22 18:01:42, charliea wrote: > In general, Chromium-style comments generally have to have the form of a > complete sentence. Also, I think we try our hardest to avoid these types of > tautological comments (Of *course* a BattOrTracingAgent is A BattOr tracing > agent. But what the heck is a tracing agent? What's a BattOr? Given that this is > probably the first BattOr-related file that someone browsing Telemetry is apt to > come across, I think it makes sense to give some more context). > > Maybe something like: > > BattOrTracingAgent allows Telemetry to issue high-level tracing commands > (StartTracing, StopTracing, RecordClockSyncMarker) to BattOrs, which are > high-frequency power monitors used for battery testing. Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:27: """ Returns if battor is supported. """ On 2016/03/22 18:01:42, charliea wrote: > Leading/trailing spaces Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:29: return True # if the mapping to device_id to battor works. On 2016/03/22 18:01:42, charliea wrote: > Someone else might know better, but same comment as above about Chromium > comments generally being complete sentences This comment is just a reminder for me to do this. This comment will never exist in the real code. Just waiting on a CL of Alex's to land before I can do it. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:31: return True # if there is only one battor attached. On 2016/03/22 18:01:42, charliea wrote: > Comment complete sentence Same as above. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:35: """ Start tracing on the battor. On 2016/03/22 18:01:42, charliea wrote: > Leading/trailing spaces Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:38: config: tracing_config instance that contains trace_option and On 2016/03/22 18:01:42, charliea wrote: > The description of just config: doesn't seem to provide much value. If the way > that you've documented trace_options and category_filter is standard pydoc, then > it seems like it'd be fair to just get rid of the config: description and have: > > config: > trace_options: An instance of trace_options.TracingOptions that controls which > core tracing systems should be enabled. > category_filter: An instance of tracing_category_filter.TracingCategoryFilter Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:41: control which core tracing systems should be enabled. On 2016/03/22 18:01:42, charliea wrote: > s/control/controls Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:43: tracing_category_filter.TracingCategoryFilter On 2016/03/22 18:01:42, charliea wrote: > Is it normal pydoc to just describe the type of the parameter without more > context about its purpose in the method? I mostly didn't change this from the documentation in the base class. I haven't done a final pass on this CL for polishing it, mostly uploaded it for context to ask questions. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:50: #TODO(rnephew): Process config and return based off that. On 2016/03/22 18:01:42, charliea wrote: > I think that this should be: > > # TODO(rnephew)... > > (Note space after #) Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:55: """ Stop tracing on the battor. """ On 2016/03/22 18:01:42, charliea wrote: > Leading/trailing spaces Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:57: # Merge results into trace_data_builder here On 2016/03/22 18:01:42, charliea wrote: > This should probably be a TODO(rnephew): Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:64: """ Record clock sync marker on battor. On 2016/03/22 18:01:42, charliea wrote: > Leading/trailing spaces Done. https://codereview.chromium.org/1819183002/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:69: and a timestamp as argument. On 2016/03/22 18:01:42, charliea wrote: > Those are the parameters, but what does the function actually do? Done.
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:26: '../../../../../../../../../chromium_vanilla/src/out/Default/' On 2016/03/22 16:57:44, rnephew1 wrote: > On 2016/03/22 00:02:43, aiolos wrote: > > On 2016/03/21 23:18:18, rnephew1 wrote: > > > aiolos: This is going to deal with dependency manager; can you send me an > > > example of how to pull this from dependency manager? > > > > You should probably look at the binary_manager in telemetry. This doc is still > > unfinished, but has links to the examples you'll want to look at: > > > https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... > > > > Let me know if you have any questions. > > I think this work will be blocked on having the battor_agent_binary being added > to the dependency manager. Adding Charlie to this to see if this is the case; > looking at the configs I do not see anything about it. You can make your own config for battor dependencies, and just initialize a dependency manager instance with it. Where is the binary stored (or going to be stored)? Is it in a checkout somewhere, or will it be stored in cloud storage? (I suggest cloud storage for binaries in general, unless the binary has to be built locally.) Note that you can both have a default binary stored in cloud storage, and an expected path for locally built binary. The local path would be used if it exists, and otherwise would fall back to the one in cloud storage.
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:26: '../../../../../../../../../chromium_vanilla/src/out/Default/' On 2016/03/22 19:08:19, aiolos wrote: > On 2016/03/22 16:57:44, rnephew1 wrote: > > On 2016/03/22 00:02:43, aiolos wrote: > > > On 2016/03/21 23:18:18, rnephew1 wrote: > > > > aiolos: This is going to deal with dependency manager; can you send me an > > > > example of how to pull this from dependency manager? > > > > > > You should probably look at the binary_manager in telemetry. This doc is > still > > > unfinished, but has links to the examples you'll want to look at: > > > > > > https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... > > > > > > Let me know if you have any questions. > > > > I think this work will be blocked on having the battor_agent_binary being > added > > to the dependency manager. Adding Charlie to this to see if this is the case; > > looking at the configs I do not see anything about it. > > You can make your own config for battor dependencies, and just initialize a > dependency manager instance with it. > > Where is the binary stored (or going to be stored)? Is it in a checkout > somewhere, or will it be stored in cloud storage? (I suggest cloud storage for > binaries in general, unless the binary has to be built locally.) Note that you > can both have a default binary stored in cloud storage, and an expected path for > locally built binary. The local path would be used if it exists, and otherwise > would fall back to the one in cloud storage. I would assume it would be stored in cloud storage. It being put there by someone (who that someone is I do not know, I added Charlie because he created it so I thought he might have some idea of the plan) is the part I am blocked on. We are not building it locally, the way the build files are made it wont build on android checkouts so has to live somewhere else. Would this be the appropriate config file to put it in? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu...
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:26: '../../../../../../../../../chromium_vanilla/src/out/Default/' On 2016/03/22 19:08:19, aiolos wrote: > On 2016/03/22 16:57:44, rnephew1 wrote: > > On 2016/03/22 00:02:43, aiolos wrote: > > > On 2016/03/21 23:18:18, rnephew1 wrote: > > > > aiolos: This is going to deal with dependency manager; can you send me an > > > > example of how to pull this from dependency manager? > > > > > > You should probably look at the binary_manager in telemetry. This doc is > still > > > unfinished, but has links to the examples you'll want to look at: > > > > > > https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... > > > > > > Let me know if you have any questions. > > > > I think this work will be blocked on having the battor_agent_binary being > added > > to the dependency manager. Adding Charlie to this to see if this is the case; > > looking at the configs I do not see anything about it. > > You can make your own config for battor dependencies, and just initialize a > dependency manager instance with it. > > Where is the binary stored (or going to be stored)? Is it in a checkout > somewhere, or will it be stored in cloud storage? (I suggest cloud storage for > binaries in general, unless the binary has to be built locally.) Note that you > can both have a default binary stored in cloud storage, and an expected path for > locally built binary. The local path would be used if it exists, and otherwise > would fall back to the one in cloud storage. Something that may be unclear: the dependency manager is more similar to a data structure than a set of dependencies. Devil has a dependency manager instance configured with the devil dependencies, and Telemetry has a separate dependency manager instance configured with the telemetry dependencies. Additionally, more than one config file can be used in each instance. This makes it easier to share dependencies across projects in the same repo, and also allows us to "overwrite" dependencies with local versions in client repos. The telemetry one uses 3 config files: one for telemetry specific dependencies (in catapult's telemetry folder), one for the chrome reference builds which are also used in other catapult projects (in catapult's catapult_base), and one for chromium specific dependencies (in chromium's tools/perf.) You should make a battor config file if it doesn't already exist, and decide where it should live based on whether you're using a locally built version or have a binary that is stored in cloud_storage.
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:26: '../../../../../../../../../chromium_vanilla/src/out/Default/' On 2016/03/22 19:20:19, aiolos wrote: > On 2016/03/22 19:08:19, aiolos wrote: > > On 2016/03/22 16:57:44, rnephew1 wrote: > > > On 2016/03/22 00:02:43, aiolos wrote: > > > > On 2016/03/21 23:18:18, rnephew1 wrote: > > > > > aiolos: This is going to deal with dependency manager; can you send me > an > > > > > example of how to pull this from dependency manager? > > > > > > > > You should probably look at the binary_manager in telemetry. This doc is > > still > > > > unfinished, but has links to the examples you'll want to look at: > > > > > > > > > > https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... > > > > > > > > Let me know if you have any questions. > > > > > > I think this work will be blocked on having the battor_agent_binary being > > added > > > to the dependency manager. Adding Charlie to this to see if this is the > case; > > > looking at the configs I do not see anything about it. > > > > You can make your own config for battor dependencies, and just initialize a > > dependency manager instance with it. > > > > Where is the binary stored (or going to be stored)? Is it in a checkout > > somewhere, or will it be stored in cloud storage? (I suggest cloud storage for > > binaries in general, unless the binary has to be built locally.) Note that you > > can both have a default binary stored in cloud storage, and an expected path > for > > locally built binary. The local path would be used if it exists, and otherwise > > would fall back to the one in cloud storage. > > Something that may be unclear: the dependency manager is more similar to a data > structure than a set of dependencies. Devil has a dependency manager instance > configured with the devil dependencies, and Telemetry has a separate dependency > manager instance configured with the telemetry dependencies. > > Additionally, more than one config file can be used in each instance. This makes > it easier to share dependencies across projects in the same repo, and also > allows us to "overwrite" dependencies with local versions in client repos. The > telemetry one uses 3 config files: one for telemetry specific dependencies (in > catapult's telemetry folder), one for the chrome reference builds which are also > used in other catapult projects (in catapult's catapult_base), and one for > chromium specific dependencies (in chromium's tools/perf.) You should make a > battor config file if it doesn't already exist, and decide where it should live > based on whether you're using a locally built version or have a binary that is > stored in cloud_storage. Thanks for the explanation, Kari. Randy, for our use case, since we control the binary under test (battor_agent_bin) and it does not support to be updated to regularly, I think it's fine to just grab a compiled binary & push it to cloud storage
On 2016/03/22 20:04:48, nednguyen wrote: > https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... > File tools/battor/battor/battor_wrapper.py (right): > > https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... > tools/battor/battor/battor_wrapper.py:26: > '../../../../../../../../../chromium_vanilla/src/out/Default/' > On 2016/03/22 19:20:19, aiolos wrote: > > On 2016/03/22 19:08:19, aiolos wrote: > > > On 2016/03/22 16:57:44, rnephew1 wrote: > > > > On 2016/03/22 00:02:43, aiolos wrote: > > > > > On 2016/03/21 23:18:18, rnephew1 wrote: > > > > > > aiolos: This is going to deal with dependency manager; can you send me > > an > > > > > > example of how to pull this from dependency manager? > > > > > > > > > > You should probably look at the binary_manager in telemetry. This doc is > > > still > > > > > unfinished, but has links to the examples you'll want to look at: > > > > > > > > > > > > > > > https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... > > > > > > > > > > Let me know if you have any questions. > > > > > > > > I think this work will be blocked on having the battor_agent_binary being > > > added > > > > to the dependency manager. Adding Charlie to this to see if this is the > > case; > > > > looking at the configs I do not see anything about it. > > > > > > You can make your own config for battor dependencies, and just initialize a > > > dependency manager instance with it. > > > > > > Where is the binary stored (or going to be stored)? Is it in a checkout > > > somewhere, or will it be stored in cloud storage? (I suggest cloud storage > for > > > binaries in general, unless the binary has to be built locally.) Note that > you > > > can both have a default binary stored in cloud storage, and an expected path > > for > > > locally built binary. The local path would be used if it exists, and > otherwise > > > would fall back to the one in cloud storage. > > > > Something that may be unclear: the dependency manager is more similar to a > data > > structure than a set of dependencies. Devil has a dependency manager instance > > configured with the devil dependencies, and Telemetry has a separate > dependency > > manager instance configured with the telemetry dependencies. > > > > Additionally, more than one config file can be used in each instance. This > makes > > it easier to share dependencies across projects in the same repo, and also > > allows us to "overwrite" dependencies with local versions in client repos. The > > telemetry one uses 3 config files: one for telemetry specific dependencies (in > > catapult's telemetry folder), one for the chrome reference builds which are > also > > used in other catapult projects (in catapult's catapult_base), and one for > > chromium specific dependencies (in chromium's tools/perf.) You should make a > > battor config file if it doesn't already exist, and decide where it should > live > > based on whether you're using a locally built version or have a binary that is > > stored in cloud_storage. > > Thanks for the explanation, Kari. > > Randy, for our use case, since we control the binary under test > (battor_agent_bin) and it does not support to be updated to regularly, I think > it's fine to just grab a compiled binary & push it to cloud storage Dont we need different builds for mac/windows though, since the battor will work directly on them?
On 2016/03/22 20:08:30, rnephew1 wrote: > On 2016/03/22 20:04:48, nednguyen wrote: > > > https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... > > File tools/battor/battor/battor_wrapper.py (right): > > > > > https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... > > tools/battor/battor/battor_wrapper.py:26: > > '../../../../../../../../../chromium_vanilla/src/out/Default/' > > On 2016/03/22 19:20:19, aiolos wrote: > > > On 2016/03/22 19:08:19, aiolos wrote: > > > > On 2016/03/22 16:57:44, rnephew1 wrote: > > > > > On 2016/03/22 00:02:43, aiolos wrote: > > > > > > On 2016/03/21 23:18:18, rnephew1 wrote: > > > > > > > aiolos: This is going to deal with dependency manager; can you send > me > > > an > > > > > > > example of how to pull this from dependency manager? > > > > > > > > > > > > You should probably look at the binary_manager in telemetry. This doc > is > > > > still > > > > > > unfinished, but has links to the examples you'll want to look at: > > > > > > > > > > > > > > > > > > > > > https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... > > > > > > > > > > > > Let me know if you have any questions. > > > > > > > > > > I think this work will be blocked on having the battor_agent_binary > being > > > > added > > > > > to the dependency manager. Adding Charlie to this to see if this is the > > > case; > > > > > looking at the configs I do not see anything about it. > > > > > > > > You can make your own config for battor dependencies, and just initialize > a > > > > dependency manager instance with it. > > > > > > > > Where is the binary stored (or going to be stored)? Is it in a checkout > > > > somewhere, or will it be stored in cloud storage? (I suggest cloud storage > > for > > > > binaries in general, unless the binary has to be built locally.) Note that > > you > > > > can both have a default binary stored in cloud storage, and an expected > path > > > for > > > > locally built binary. The local path would be used if it exists, and > > otherwise > > > > would fall back to the one in cloud storage. > > > > > > Something that may be unclear: the dependency manager is more similar to a > > data > > > structure than a set of dependencies. Devil has a dependency manager > instance > > > configured with the devil dependencies, and Telemetry has a separate > > dependency > > > manager instance configured with the telemetry dependencies. > > > > > > Additionally, more than one config file can be used in each instance. This > > makes > > > it easier to share dependencies across projects in the same repo, and also > > > allows us to "overwrite" dependencies with local versions in client repos. > The > > > telemetry one uses 3 config files: one for telemetry specific dependencies > (in > > > catapult's telemetry folder), one for the chrome reference builds which are > > also > > > used in other catapult projects (in catapult's catapult_base), and one for > > > chromium specific dependencies (in chromium's tools/perf.) You should make a > > > battor config file if it doesn't already exist, and decide where it should > > live > > > based on whether you're using a locally built version or have a binary that > is > > > stored in cloud_storage. > > > > Thanks for the explanation, Kari. > > > > Randy, for our use case, since we control the binary under test > > (battor_agent_bin) and it does not support to be updated to regularly, I think > > it's fine to just grab a compiled binary & push it to cloud storage > > Dont we need different builds for mac/windows though, since the battor will work > directly on them? Yes we do, sorry I forget about this.
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:26: '../../../../../../../../../chromium_vanilla/src/out/Default/' On 2016/03/22 20:04:48, nednguyen wrote: > On 2016/03/22 19:20:19, aiolos wrote: > > On 2016/03/22 19:08:19, aiolos wrote: > > > On 2016/03/22 16:57:44, rnephew1 wrote: > > > > On 2016/03/22 00:02:43, aiolos wrote: > > > > > On 2016/03/21 23:18:18, rnephew1 wrote: > > > > > > aiolos: This is going to deal with dependency manager; can you send me > > an > > > > > > example of how to pull this from dependency manager? > > > > > > > > > > You should probably look at the binary_manager in telemetry. This doc is > > > still > > > > > unfinished, but has links to the examples you'll want to look at: > > > > > > > > > > > > > > > https://docs.google.com/document/d/1Xj841C1bPCLJrxgCNBsuohLA_XV-vqAbfF0Gx0bf8... > > > > > > > > > > Let me know if you have any questions. > > > > > > > > I think this work will be blocked on having the battor_agent_binary being > > > added > > > > to the dependency manager. Adding Charlie to this to see if this is the > > case; > > > > looking at the configs I do not see anything about it. > > > > > > You can make your own config for battor dependencies, and just initialize a > > > dependency manager instance with it. > > > > > > Where is the binary stored (or going to be stored)? Is it in a checkout > > > somewhere, or will it be stored in cloud storage? (I suggest cloud storage > for > > > binaries in general, unless the binary has to be built locally.) Note that > you > > > can both have a default binary stored in cloud storage, and an expected path > > for > > > locally built binary. The local path would be used if it exists, and > otherwise > > > would fall back to the one in cloud storage. > > > > Something that may be unclear: the dependency manager is more similar to a > data > > structure than a set of dependencies. Devil has a dependency manager instance > > configured with the devil dependencies, and Telemetry has a separate > dependency > > manager instance configured with the telemetry dependencies. > > > > Additionally, more than one config file can be used in each instance. This > makes > > it easier to share dependencies across projects in the same repo, and also > > allows us to "overwrite" dependencies with local versions in client repos. The > > telemetry one uses 3 config files: one for telemetry specific dependencies (in > > catapult's telemetry folder), one for the chrome reference builds which are > also > > used in other catapult projects (in catapult's catapult_base), and one for > > chromium specific dependencies (in chromium's tools/perf.) You should make a > > battor config file if it doesn't already exist, and decide where it should > live > > based on whether you're using a locally built version or have a binary that is > > stored in cloud_storage. > > Thanks for the explanation, Kari. > > Randy, for our use case, since we control the binary under test > (battor_agent_bin) and it does not support to be updated to regularly, I think > it's fine to just grab a compiled binary & push it to cloud storage I have added (most of) the config file. They are not uploaded yet, so I have not been able to add the hashes. Could you take a look and see if its approximately correct?
https://codereview.chromium.org/1819183002/diff/60001/tools/battor/battor/bin... File tools/battor/battor/binary_dependencies.json (right): https://codereview.chromium.org/1819183002/diff/60001/tools/battor/battor/bin... tools/battor/battor/binary_dependencies.json:10: "local_paths": ["../../../../../out/Debug/battor_agent", You can leave the local_path out.
https://codereview.chromium.org/1819183002/diff/60001/tools/battor/battor/bin... File tools/battor/battor/binary_dependencies.json (right): https://codereview.chromium.org/1819183002/diff/60001/tools/battor/battor/bin... tools/battor/battor/binary_dependencies.json:10: "local_paths": ["../../../../../out/Debug/battor_agent", On 2016/03/22 20:34:08, nednguyen wrote: > You can leave the local_path out. Done.
> I have added (most of) the config file. They are not uploaded yet, so I have not > been able to add the hashes. Could you take a look and see if its approximately > correct? If you make the suggested changes to the config file, the easiest way to upload to cloud storage is to use BaseConfig's (dependency_manager/base_config.py) AddCloudStorageDependencyUpdateJob. You never need to know the hash, and it will handle the upload to the correct place in cloud_storage for you. You just need the name of the dependency, the platform you're updating, and a local path to the file you want to put in cloud_storage. I can write a pretty quick script for that if you need me to, but it should be easy enough to use from python. I can show you an example usage of that for the update reference build script offline if you'd like. https://codereview.chromium.org/1819183002/diff/80001/tools/battor/battor/bin... File tools/battor/battor/binary_dependencies.json (right): https://codereview.chromium.org/1819183002/diff/80001/tools/battor/battor/bin... tools/battor/battor/binary_dependencies.json:4: "cloud_storage_base_folder": "...", "chrome-partner-telemetry/battor" https://codereview.chromium.org/1819183002/diff/80001/tools/battor/battor/bin... tools/battor/battor/binary_dependencies.json:5: "cloud_storage_bucket": "...", If the binary is available publicly, you can use the "chromium-telemetry" bucket. If it should only be available to google + partners use "chrome-partner-telemetry" Only google: "chrome-telemetry" We'll probably eventually get catapult-wide buckets, but you shouldn't block on it.
It seems pretty straight forward how to do it, I'll talk to people offline and get the correct binaries together and upload them. If I have any problems I'll let you know. Thanks! https://codereview.chromium.org/1819183002/diff/80001/tools/battor/battor/bin... File tools/battor/battor/binary_dependencies.json (right): https://codereview.chromium.org/1819183002/diff/80001/tools/battor/battor/bin... tools/battor/battor/binary_dependencies.json:4: "cloud_storage_base_folder": "...", On 2016/03/22 21:19:19, aiolos wrote: > "chrome-partner-telemetry/battor" Done. https://codereview.chromium.org/1819183002/diff/80001/tools/battor/battor/bin... tools/battor/battor/binary_dependencies.json:5: "cloud_storage_bucket": "...", On 2016/03/22 21:19:19, aiolos wrote: > If the binary is available publicly, you can use the "chromium-telemetry" > bucket. > If it should only be available to google + partners use > "chrome-partner-telemetry" > Only google: "chrome-telemetry" > > We'll probably eventually get catapult-wide buckets, but you shouldn't block on > it. Since the source is part of chromium, I will assume it goes in chromium-telemetry unless someone says otherwise.
On 2016/03/22 21:29:50, rnephew1 wrote: > It seems pretty straight forward how to do it, I'll talk to people offline and > get the correct binaries together and upload them. If I have any problems I'll > let you know. > Thanks! np. Using that API will update the hash in your json file for you. Just make sure you update this cl afterward with the change. Let me know if you run into any snags, or wish something was handled differently.
https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/1/tools/battor/battor/battor_... tools/battor/battor/battor_wrapper.py:61: # TODO(rnephew): Change this when dumping to file is complete. On 2016/03/22 00:06:48, rnephew1 wrote: > On 2016/03/21 23:47:54, alexandermont wrote: > > It's not actually necessary to use the 'dumping to file' option; you could > just > > keep it the way it is and avoid the extra step of battor_agent_binary --> file > > --> results. The purpose of the 'dumping to file' option is mainly when the > user > > is using it interactively - so the user can see the results of each command > > (like the "Done.") printed to the screen, but then have the trace output > printed > > to the file instead. > > I remember there being talk of wanting to separate StopTracing and collection of > results in order to minimize the length difference of different traces. I was > envisioning this going something like: > > Call StopTracing > Battor stops tracing > battor agent returns done > battor agent dumps results > > and while it is dumping results the other agents are being stopped. Then a > collection pass. > > Zhen, Ned; let me know if I am remembering wrong about the separating of > stopping and collecting. Yes. I think we need to separate StopTracing and collection of results. For battor, this is important because collecting the results takes a long time. This means, we will need to update the StopTracing API to 2 steps. Something close to Alex's systrace refactor CL is preferred. So later when we unify all tracing tools, it will be easier. https://codereview.chromium.org/1776013005/diff/200001/systrace/systrace/trac...
https://codereview.chromium.org/1819183002/diff/100001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/100001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:8: #from devil.utils import find_usb_devices Do we need to put hub_types and serial_map in tracing_config? My suggestion would be separating Chrome side trace config and telemetry side trace config. Right now, they are all in tracing_config. The Chrome side trace config, say it is ChromeTraceConfig, should be one-to-one mapping to DevTools tracing protocol https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... The Telemetry side trace config contains other things, e.g., - enable_chrome_trace - enable_platform_display_trace - hub_types, serial_map, etc ChromeTraceConfig is only used when enable_chrome_trace is True. The above will go to a different CL. And it should be done as part of https://github.com/catapult-project/catapult/issues/2162.
On 2016/03/22 23:50:56, Zhen Wang wrote: > https://codereview.chromium.org/1819183002/diff/100001/telemetry/telemetry/in... > File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py > (right): > > https://codereview.chromium.org/1819183002/diff/100001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:8: > #from devil.utils import find_usb_devices > Do we need to put hub_types and serial_map in tracing_config? > > My suggestion would be separating Chrome side trace config and telemetry side > trace config. Right now, they are all in tracing_config. > > The Chrome side trace config, say it is ChromeTraceConfig, should be one-to-one > mapping to DevTools tracing protocol > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... > > The Telemetry side trace config contains other things, e.g., > - enable_chrome_trace > - enable_platform_display_trace > - hub_types, serial_map, etc > > ChromeTraceConfig is only used when enable_chrome_trace is True. > > > The above will go to a different CL. And it should be done as part of > https://github.com/catapult-project/catapult/issues/2162. Splitting this into 2 CLs. one for the tracing agent, one for the python wrapper around battor_agent_binary.
On 2016/03/24 18:12:32, rnephew1 wrote: > On 2016/03/22 23:50:56, Zhen Wang wrote: > > > https://codereview.chromium.org/1819183002/diff/100001/telemetry/telemetry/in... > > File > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py > > (right): > > > > > https://codereview.chromium.org/1819183002/diff/100001/telemetry/telemetry/in... > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:8: > > #from devil.utils import find_usb_devices > > Do we need to put hub_types and serial_map in tracing_config? > > > > My suggestion would be separating Chrome side trace config and telemetry side > > trace config. Right now, they are all in tracing_config. > > > > The Chrome side trace config, say it is ChromeTraceConfig, should be > one-to-one > > mapping to DevTools tracing protocol > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... > > > > The Telemetry side trace config contains other things, e.g., > > - enable_chrome_trace > > - enable_platform_display_trace > > - hub_types, serial_map, etc > > > > ChromeTraceConfig is only used when enable_chrome_trace is True. > > > > > > The above will go to a different CL. And it should be done as part of > > https://github.com/catapult-project/catapult/issues/2162. > > Splitting this into 2 CLs. one for the tracing agent, one for the python wrapper > around battor_agent_binary. https://codereview.chromium.org/1828143004
https://codereview.chromium.org/1819183002/diff/120001/tools/battor/battor/bi... File tools/battor/battor/binary_dependencies.json (right): https://codereview.chromium.org/1819183002/diff/120001/tools/battor/battor/bi... tools/battor/battor/binary_dependencies.json:5: "cloud_storage_base_folder": "chrome-partner-telemetry/battor", Ugg. I apparently had a copy-paste fail earlier. This should be binary_dependencies, not chrome-partner-telemetry. I'm really sorry about that. https://codereview.chromium.org/1819183002/diff/120001/tools/battor/battor/bi... tools/battor/battor/binary_dependencies.json:12: "mac_x86_64": { You still need the mac and win deps.
https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:11: class BattorTracingAgent(tracing_agent.TracingAgent): I think you still need a GetResults function in this class. https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:30: if platform_backend.GetOsName() == 'android': You will probably be finding out if the battor mapping worked (in both the android and linux cases) during the battor_wrapper.BattorWrapper() function in __init__ - the code to do the mapping throws a BattorError if it doens't work. So I think that the __init__function could catch the BattorError and record that it failed, and then IsSupported could check this record. https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:44: An intance of tracing_category_filter.TracingCategoryFilter instance https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:68: sync_id: Unqiue id for sync event. unique https://codereview.chromium.org/1819183002/diff/120001/tools/battor/battor/ba... File tools/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/120001/tools/battor/battor/ba... tools/battor/battor/battor_wrapper.py:65: battor_cmd.append('--bator-path=%s' % self._battor_path) battor-path
rnephew@chromium.org changed reviewers: - aiolos@chromium.org
Still needs https://codereview.chromium.org/1828143004/ to land. https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:11: class BattorTracingAgent(tracing_agent.TracingAgent): On 2016/03/28 18:22:41, alexandermont wrote: > I think you still need a GetResults function in this class. Done. https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:30: if platform_backend.GetOsName() == 'android': On 2016/03/28 18:22:41, alexandermont wrote: > You will probably be finding out if the battor mapping worked (in both the > android and linux cases) during the battor_wrapper.BattorWrapper() function in > __init__ - the code to do the mapping throws a BattorError if it doens't work. > So I think that the __init__function could catch the BattorError and record that > it failed, and then IsSupported could check this record. This is a class method that is called before an instance of battor_tracing_agent is created, so it will not be initialized yet. Because of this it will have to use find_usb_devices. This is likely the site where it will make sure the mapping file exists for android, and if not create it. We probably want a way to set the mapping configs path from the command line and pass it through to here: Thoughts on this Ned? IsSupported is used the tracing_controller_backend on line 63 if you want more context on where it is used. https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:44: An intance of tracing_category_filter.TracingCategoryFilter On 2016/03/28 18:22:41, alexandermont wrote: > instance Done. https://codereview.chromium.org/1819183002/diff/120001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:68: sync_id: Unqiue id for sync event. On 2016/03/28 18:22:41, alexandermont wrote: > unique Done.
Description was changed from ========== [Telemetry] Add battor tracing agent. Also adds battor communication wrapper. BUG=chromium:574888 ========== to ========== [Telemetry] Add battor tracing agent. BUG=chromium:574888 ==========
https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:11: class BattorTracingAgent(tracing_agent.TracingAgent): s/Battor/BattOr It's more correct and also matches https://code.google.com/p/chromium/codesearch#chromium/src/tools/battor_agent... https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:12: """A tracing agent for getting power data from battor device. s/battor/BattOr here (and everywhere else) https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:45: timeout: number of seconds that this tracing agent should try to start nit: wrong indentation for |timeout| https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:49: True if tracing agent started successfully. nit: "if tracing agent" => "if the tracing agent" https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:56: """Stop tracing on the battor.""" nit: "Stop tracing on the battor." => "Stops tracing on the BattOr." https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:64: """Record clock sync marker on battor. nit: "Record clock sync marker on battor." => "Records a clock sync marker in the BattOr trace." Technically, the clock sync marker is recorded on the client-side, so saying it happens "in the BattOr trace" is more accurate than saying that it happens on the BattOr.
Patchset #9 (id:160001) has been deleted
Patchset #9 (id:180001) has been deleted
Patchset #9 (id:200001) has been deleted
Patchset #9 (id:220001) has been deleted
https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:11: class BattorTracingAgent(tracing_agent.TracingAgent): On 2016/04/04 20:39:31, charliea wrote: > s/Battor/BattOr > > It's more correct and also matches > https://code.google.com/p/chromium/codesearch#chromium/src/tools/battor_agent... Done. https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:12: """A tracing agent for getting power data from battor device. On 2016/04/04 20:39:32, charliea wrote: > s/battor/BattOr here (and everywhere else) Done. https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:45: timeout: number of seconds that this tracing agent should try to start On 2016/04/04 20:39:32, charliea wrote: > nit: wrong indentation for |timeout| Whats wrong with the indentation? It should be at the same level as config. Trace_options and category_filter are indented more because they are what is inside of config. Since the merger of tracing options and tracing config is complete, I will get rid of the extra explanation of what config is. https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:49: True if tracing agent started successfully. On 2016/04/04 20:39:32, charliea wrote: > nit: "if tracing agent" => "if the tracing agent" Done. https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:56: """Stop tracing on the battor.""" On 2016/04/04 20:39:31, charliea wrote: > nit: "Stop tracing on the battor." => "Stops tracing on the BattOr." Done. https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:64: """Record clock sync marker on battor. On 2016/04/04 20:39:31, charliea wrote: > nit: "Record clock sync marker on battor." => "Records a clock sync marker in > the BattOr trace." > > Technically, the clock sync marker is recorded on the client-side, so saying it > happens "in the BattOr trace" is more accurate than saying that it happens on > the BattOr. Done.
charliea@chromium.org changed reviewers: - zhenw@chromium.org
https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/140001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:45: timeout: number of seconds that this tracing agent should try to start On 2016/04/04 22:35:27, rnephew1 wrote: > On 2016/04/04 20:39:32, charliea wrote: > > nit: wrong indentation for |timeout| > > Whats wrong with the indentation? It should be at the same level as config. > Trace_options and category_filter are indented more because they are what is > inside of config. Since the merger of tracing options and tracing config is > complete, I will get rid of the extra explanation of what config is. Ah, you're right - apologies. For some reason I was thinking that it should line up w/ trace_options and category_filter https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:33: """Returns if BattOr is supported.""" nit: I think this comment could be better. Maybe "Returns true if BattOr tracing is available."? https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:37: # telemetry this will likely change. It might be worth adding a comment here about what the situation for Android phones and BattOrs is here. It's obvious to us now, but it might not be obvious to people browsing the code in the future. Something like: A single host running this code can have multiple Android/BattOr pairs attached. Check to see if the Android device we're tracing on has a corresponding BattOr. https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:38: mapping = battor_device_mapping.GenerateSerialMap() nit: given that the function `GenerateSerialMap()` has a pretty vague name, I think it might be good to have a more specific variable name for `mapping` here. It's not obvious as a reader why `platform_backend.device in mapping` means that BattOr tracing is supported, although it became more obvious after a couple of minutes looking into it. Maybe `device_to_battor_map`? Then, `platform_backend.device in device_to_battor_map` makes a whole lot more sense. https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:41: return True Same note about the comment here. Something like: For non-Android devices, a single BattOr is a sufficient signal that power tracing is available. https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:78: record_controller_clocksync_marker_callback: Function that takes sync_id nit: clocksync is two words everywhere else (ClockSync), so should probably be clock_sync here https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:87: # TODO(rnephew): add trace data to trace_data_builder nit: capitalize first letter of comment
Patchset #10 (id:260001) has been deleted
https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:33: """Returns if BattOr is supported.""" On 2016/04/05 18:50:21, charliea wrote: > nit: I think this comment could be better. Maybe "Returns true if BattOr tracing > is available."? Done. https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:37: # telemetry this will likely change. On 2016/04/05 18:50:21, charliea wrote: > It might be worth adding a comment here about what the situation for Android > phones and BattOrs is here. It's obvious to us now, but it might not be obvious > to people browsing the code in the future. > > Something like: > > A single host running this code can have multiple Android/BattOr pairs attached. > Check to see if the Android device we're tracing on has a corresponding BattOr. Done. https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:38: mapping = battor_device_mapping.GenerateSerialMap() On 2016/04/05 18:50:21, charliea wrote: > nit: given that the function `GenerateSerialMap()` has a pretty vague name, I > think it might be good to have a more specific variable name for `mapping` here. > It's not obvious as a reader why `platform_backend.device in mapping` means that > BattOr tracing is supported, although it became more obvious after a couple of > minutes looking into it. > > Maybe `device_to_battor_map`? Then, `platform_backend.device in > device_to_battor_map` makes a whole lot more sense. Done. https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:41: return True On 2016/04/05 18:50:21, charliea wrote: > Same note about the comment here. > > Something like: > > For non-Android devices, a single BattOr is a sufficient signal that power > tracing is available. Done. https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:78: record_controller_clocksync_marker_callback: Function that takes sync_id On 2016/04/05 18:50:21, charliea wrote: > nit: clocksync is two words everywhere else (ClockSync), so should probably be > clock_sync here Done. https://codereview.chromium.org/1819183002/diff/240001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:87: # TODO(rnephew): add trace data to trace_data_builder On 2016/04/05 18:50:21, charliea wrote: > nit: capitalize first letter of comment Done.
https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:39: # telemetry this will likely change. We can pass the mapping file into telemetry through the TracingConfig.
https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:15: """A tracing agent for getting power data from BattOr device. nit: from BattOr device -> from a BattOr device https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:54: tracing until time out. nit: s/time out/timing out https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:81: record_controller_clock_sync_marker_callback: Function that takes sync_id supernit: sync_id is a variable name, but "a timestamp" isn't. Should probably either be "takes a sync ID and a timestamp" (my preference) or "takes sync_id and timestamp" https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:82: and a timestamp as argument. This function typically will record the nit: s/argument/arguments https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:90: # TODO(rnephew): Add trace data to trace_data_builder after ubertracing Maybe replace "ubertracing format" with "trace container format" now that we're pretty sure our container is going to be a totally-lame .tar or .zip file. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:67: del timeout #unused nit: think there should probably be a space before "unused" source: https://code.google.com/p/chromium/codesearch#search/&q=f:.py%20%5C%20%5C%20d... https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:79: # Monkey patch BattOr, battor_device_mapping and find_usb_devices calls. nit: do you mean BattOrWrapper (instead of BattOr)? https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:150: self.android_agent._battor.StartTracing = lambda: (_ for _ in ()).throw( What do this line do? Why can't you just have it be: lambda: throw battor_wrapper.BattOrError https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:159: # TODO(rnephew): Finish test when ubertracing is figured out. Same preference as in other CL about preferring "tracing container" (self-explanatory) over ubertracing https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:167: del a # Unused. I think "# unused" (no period, lower case) is a lot more idiomatic source: https://code.google.com/p/chromium/codesearch#search/&q=f:.py%20%5C%20%5C%20d... https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:225: nit: are this many newlines needed before __main__?
https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:15: """A tracing agent for getting power data from BattOr device. On 2016/04/21 19:49:09, charliea wrote: > nit: from BattOr device -> from a BattOr device Done. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:39: # telemetry this will likely change. On 2016/04/09 00:32:52, nednguyen wrote: > We can pass the mapping file into telemetry through the TracingConfig. I will add that in another CL if you are ok with that. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:54: tracing until time out. On 2016/04/21 19:49:09, charliea wrote: > nit: s/time out/timing out Done. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:81: record_controller_clock_sync_marker_callback: Function that takes sync_id On 2016/04/21 19:49:09, charliea wrote: > supernit: sync_id is a variable name, but "a timestamp" isn't. Should probably > either be "takes a sync ID and a timestamp" (my preference) or "takes sync_id > and timestamp" Done. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:82: and a timestamp as argument. This function typically will record the On 2016/04/21 19:49:09, charliea wrote: > nit: s/argument/arguments Done. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:90: # TODO(rnephew): Add trace data to trace_data_builder after ubertracing On 2016/04/21 19:49:09, charliea wrote: > Maybe replace "ubertracing format" with "trace container format" now that we're > pretty sure our container is going to be a totally-lame .tar or .zip file. Done. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:67: del timeout #unused On 2016/04/21 19:49:09, charliea wrote: > nit: think there should probably be a space before "unused" > > source: > https://code.google.com/p/chromium/codesearch#search/&q=f:.py%20%5C%20%5C%20d... Done. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:79: # Monkey patch BattOr, battor_device_mapping and find_usb_devices calls. On 2016/04/21 19:49:09, charliea wrote: > nit: do you mean BattOrWrapper (instead of BattOr)? Done. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:150: self.android_agent._battor.StartTracing = lambda: (_ for _ in ()).throw( On 2016/04/21 19:49:09, charliea wrote: > What do this line do? Why can't you just have it be: > > lambda: throw battor_wrapper.BattOrError That doesn't work in python lambdas. I changed it to not use a lambda, I think that makes it clearer whats going on. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:159: # TODO(rnephew): Finish test when ubertracing is figured out. On 2016/04/21 19:49:09, charliea wrote: > Same preference as in other CL about preferring "tracing container" > (self-explanatory) over ubertracing Acknowledged. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:167: del a # Unused. On 2016/04/21 19:49:09, charliea wrote: > I think "# unused" (no period, lower case) is a lot more idiomatic > > source: > https://code.google.com/p/chromium/codesearch#search/&q=f:.py%20%5C%20%5C%20d... Done. https://codereview.chromium.org/1819183002/diff/270001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:225: On 2016/04/21 19:49:09, charliea wrote: > nit: are this many newlines needed before __main__? Done.
Patchset #12 (id:310001) has been deleted
Patchset #11 (id:290001) has been deleted
https://codereview.chromium.org/1819183002/diff/330001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/330001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:36: else platform_backend.device) Use positive boolean tests rather than negative: android_device = (platform_backend.device if platform_backend.GetOSName() == 'android' else None) https://codereview.chromium.org/1819183002/diff/330001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:62: def StartAgentTracing(self, config, timeout): This function doesn't seem to use the "timeout" argument. https://codereview.chromium.org/1819183002/diff/330001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:101: record_controller_clock_sync_marker_callback(sync_id, timestamp) Is it supposed to be (sync_id, timestamp) or (timestamp, sync_id)? I remember we discussed this at one point; I don't remember what our conclusion was.
https://codereview.chromium.org/1819183002/diff/330001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/330001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:36: else platform_backend.device) On 2016/04/26 21:47:31, alexandermont wrote: > Use positive boolean tests rather than negative: > > android_device = (platform_backend.device if platform_backend.GetOSName() == > 'android' else None) While I agree in general, in this case it is not possible; platform_backend.device will not exist if the platform_backend is not for android. https://codereview.chromium.org/1819183002/diff/330001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:62: def StartAgentTracing(self, config, timeout): On 2016/04/26 21:47:31, alexandermont wrote: > This function doesn't seem to use the "timeout" argument. Its more just there because all other tracing agents take timeout. https://codereview.chromium.org/1819183002/diff/330001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:101: record_controller_clock_sync_marker_callback(sync_id, timestamp) On 2016/04/26 21:47:31, alexandermont wrote: > Is it supposed to be (sync_id, timestamp) or (timestamp, sync_id)? I remember we > discussed this at one point; I don't remember what our conclusion was. I dont think there ever was a conclusion. I am still a proponent of sync_id first though.
lgtm https://codereview.chromium.org/1819183002/diff/410001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/410001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:20: from serial.tools import list_ports # pylint: disable=unused-import I thought this is already handled by the battor_wrapper?
rnephew@chromium.org changed reviewers: + aiolos@chromium.org
A lot of tests are failing because of this: CloudStorageIODisabled: Environment variable DISABLE_CLOUD_STORAGE_IO is set to 1. Command ['C:\\b\\depot_tools\\python276_bin\\python.exe', 'C:\\b\\build\\slave\\catapult\\build\\catapult\\third_party\\gsutil\\gsutil', 'cp', u'gs://chromium-telemetry/binary_dependencies/battor/battor_agent_binary_8de8edc555f2d66fd53dde2c42351d08e7f4f6f2', u'C:\\b\\build\\slave\\catapult\\build\\catapult\\common\\battor\\bin\\win\\AMD64\\tmptxood9'] is not allowed to run The same test one pass will fail with this, then the next attempt pass. Anyone see this before? Adding Kari since this seems to be a problem related to Dep Manager. https://codereview.chromium.org/1819183002/diff/410001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/410001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:20: from serial.tools import list_ports # pylint: disable=unused-import On 2016/04/27 17:50:21, nednguyen wrote: > I thought this is already handled by the battor_wrapper? It is needed for IsSupported to detect if there is a battor. I can move this to a module scoped function in BattorWrapper so that we do not need this here.
On 2016/04/27 18:07:11, rnephew1 wrote: > A lot of tests are failing because of this: > CloudStorageIODisabled: Environment variable DISABLE_CLOUD_STORAGE_IO is set > to 1. Command ['C:\\b\\depot_tools\\python276_bin\\python.exe', > 'C:\\b\\build\\slave\\catapult\\build\\catapult\\third_party\\gsutil\\gsutil', > 'cp', > u'gs://chromium-telemetry/binary_dependencies/battor/battor_agent_binary_8de8edc555f2d66fd53dde2c42351d08e7f4f6f2', > u'C:\\b\\build\\slave\\catapult\\build\\catapult\\common\\battor\\bin\\win\\AMD64\\tmptxood9'] > is not allowed to run > > The same test one pass will fail with this, then the next attempt pass. > Anyone see this before? Adding Kari since this seems to be a problem related to > Dep Manager. > > https://codereview.chromium.org/1819183002/diff/410001/telemetry/telemetry/in... > File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py > (right): > > https://codereview.chromium.org/1819183002/diff/410001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:20: > from serial.tools import list_ports # pylint: disable=unused-import > On 2016/04/27 17:50:21, nednguyen wrote: > > I thought this is already handled by the battor_wrapper? > > It is needed for IsSupported to detect if there is a battor. I can move this to > a module scoped function in BattorWrapper so that we do not need this here. Ah, you need to add the battor_binary_dependencies.json dep to this list: https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
Patchset #16 (id:430001) has been deleted
Patchset #16 (id:450001) has been deleted
On 2016/04/27 18:20:45, nednguyen wrote: > On 2016/04/27 18:07:11, rnephew1 wrote: > > A lot of tests are failing because of this: > > CloudStorageIODisabled: Environment variable DISABLE_CLOUD_STORAGE_IO is set > > to 1. Command ['C:\\b\\depot_tools\\python276_bin\\python.exe', > > 'C:\\b\\build\\slave\\catapult\\build\\catapult\\third_party\\gsutil\\gsutil', > > 'cp', > > > u'gs://chromium-telemetry/binary_dependencies/battor/battor_agent_binary_8de8edc555f2d66fd53dde2c42351d08e7f4f6f2', > > > u'C:\\b\\build\\slave\\catapult\\build\\catapult\\common\\battor\\bin\\win\\AMD64\\tmptxood9'] > > is not allowed to run > > > > The same test one pass will fail with this, then the next attempt pass. > > Anyone see this before? Adding Kari since this seems to be a problem related > to > > Dep Manager. > > > > > https://codereview.chromium.org/1819183002/diff/410001/telemetry/telemetry/in... > > File > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py > > (right): > > > > > https://codereview.chromium.org/1819183002/diff/410001/telemetry/telemetry/in... > > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:20: > > from serial.tools import list_ports # pylint: disable=unused-import > > On 2016/04/27 17:50:21, nednguyen wrote: > > > I thought this is already handled by the battor_wrapper? > > > > It is needed for IsSupported to detect if there is a battor. I can move this > to > > a module scoped function in BattorWrapper so that we do not need this here. > > Ah, you need to add the battor_binary_dependencies.json dep to this list: > https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/... That seems to have fixed it. I will wait for https://codereview.chromium.org/1920023002 to land so that I can move the windows battor detection to battor_wrapper. I will post here again when that happens so you can take another look.
Made some changes based off of https://codereview.chromium.org/1946773002/ that simplifies some parts of this CL. It must land before this can land. https://codereview.chromium.org/1819183002/diff/490001/common/battor/battor/b... File common/battor/battor/battor_wrapper.py (right): https://codereview.chromium.org/1819183002/diff/490001/common/battor/battor/b... common/battor/battor/battor_wrapper.py:20: def AutoDetectBattOr(hey, **kwargs): This will be gotten rid of when this cl lands, only here for local testing: https://codereview.chromium.org/1946773002/
Made some changes based off of https://codereview.chromium.org/1946773002/ that simplifies some parts of this CL. It must land before this can land.
https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:26: None if not platform_backend.GetOSName() == 'android' This logic seems sort of convoluted. What about: android_device = ( platform_backend.device if platform_backend.GetOSName() == 'android' else None) (This prevents the double negative we have right now where we assign it to platform_backend.device `if not not platform_backend.GetOSName() == 'android'`) https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:75: id and a timestamp as arguments. This function typically will record the supernit: s/id/ID https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:146: # TODO(rnephew): Finish test when ubertracing is figured out. What sort of input do you still need re: ubertracing to make this test the way you want it? I think basically everything is ironed out. https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:197: nit: no need for second blank line https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py (right): https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py:102: tracing_controller_backend._IterAlLTracingAgentClasses = lambda: None Is this necessary? Seems like there's a typo here and things are still working anyway?
https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:26: None if not platform_backend.GetOSName() == 'android' On 2016/05/04 15:12:49, charliea wrote: > This logic seems sort of convoluted. What about: > > android_device = ( > platform_backend.device if platform_backend.GetOSName() == 'android' > else None) > > (This prevents the double negative we have right now where we assign it to > platform_backend.device `if not not platform_backend.GetOSName() == 'android'`) Hmm, I did it that way because not all platforms have (platform_backend).device but it doesn't seem to matter since its behind the boolean guard of GetOSName() == 'android' and never gets evaluated. Done. https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:75: id and a timestamp as arguments. This function typically will record the On 2016/05/04 15:12:49, charliea wrote: > supernit: s/id/ID Done. https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:146: # TODO(rnephew): Finish test when ubertracing is figured out. On 2016/05/04 15:12:49, charliea wrote: > What sort of input do you still need re: ubertracing to make this test the way > you want it? I think basically everything is ironed out. Just a leftover from more uncertain days. Gone. https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:197: On 2016/05/04 15:12:49, charliea wrote: > nit: no need for second blank line Done. https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py (right): https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py:102: tracing_controller_backend._IterAlLTracingAgentClasses = lambda: None On 2016/05/04 15:12:49, charliea wrote: > Is this necessary? Seems like there's a typo here and things are still working > anyway? I was just not wanting it to find the other tracing agents and run their code paths for IsSupported. Since this is a unit test, they should be compartmentalized from the tracing agents themselves. Its not strictly necessary, I just wanted to limit dependencies in the unit tests. This was also a stab in the dark to fix the dependency manager problem discussed in other patchsets, but that was figured out in a different way. Got rid of it for now.
On 2016/05/04 17:10:33, rnephew1 wrote: > https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... > File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py > (right): > > https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:26: > None if not platform_backend.GetOSName() == 'android' > On 2016/05/04 15:12:49, charliea wrote: > > This logic seems sort of convoluted. What about: > > > > android_device = ( > > platform_backend.device if platform_backend.GetOSName() == 'android' > > else None) > > > > (This prevents the double negative we have right now where we assign it to > > platform_backend.device `if not not platform_backend.GetOSName() == > 'android'`) > Hmm, I did it that way because not all platforms have (platform_backend).device > but it doesn't seem to matter since its behind the boolean guard of GetOSName() > == 'android' and never gets evaluated. > > Done. > > https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:75: > id and a timestamp as arguments. This function typically will record the > On 2016/05/04 15:12:49, charliea wrote: > > supernit: s/id/ID > > Done. > > https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... > File > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py > (right): > > https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:146: > # TODO(rnephew): Finish test when ubertracing is figured out. > On 2016/05/04 15:12:49, charliea wrote: > > What sort of input do you still need re: ubertracing to make this test the way > > you want it? I think basically everything is ironed out. > > Just a leftover from more uncertain days. Gone. > > https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:197: > > On 2016/05/04 15:12:49, charliea wrote: > > nit: no need for second blank line > > Done. > > https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... > File > telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py > (right): > > https://codereview.chromium.org/1819183002/diff/490001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py:102: > tracing_controller_backend._IterAlLTracingAgentClasses = lambda: None > On 2016/05/04 15:12:49, charliea wrote: > > Is this necessary? Seems like there's a typo here and things are still working > > anyway? > > I was just not wanting it to find the other tracing agents and run their code > paths for IsSupported. Since this is a unit test, they should be > compartmentalized from the tracing agents themselves. Its not strictly > necessary, I just wanted to limit dependencies in the unit tests. > > This was also a stab in the dark to fix the dependency manager problem discussed > in other patchsets, but that was figured out in a different way. > > Got rid of it for now. I think that the last tryjobs failed because of https://codereview.chromium.org/1946773002/ not landing yet. I will rerun them when I rebase with that in.
Let create a test in tracing_controller_unittest.py that smoke test battor agent
data. s.t like:
tracing_controller = self._browser.platform.tracing_controller
config = tracing_config.TracingConfig()
config.enable_battor_trace = True
tracing_controller.StartTracing(config)
...
trace_data = tracing_controller.StopTracing()
assert s.t about trace_data
https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in...
File
telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py
(right):
https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in...
telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:216:
if __name__ == '__main__':
You don't need these for telemetry tests
https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in...
File
telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py
(right):
https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in...
telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py:354:
if __name__ == '__main__':
Same here
On 2016/05/06 02:40:36, nednguyen wrote: > Let create a test in tracing_controller_unittest.py that smoke test battor agent > data. s.t like: > tracing_controller = self._browser.platform.tracing_controller > config = tracing_config.TracingConfig() > config.enable_battor_trace = True > tracing_controller.StartTracing(config) > ... > trace_data = tracing_controller.StopTracing() > assert s.t about trace_data > > https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in... > File > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py > (right): > > https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:216: > if __name__ == '__main__': > You don't need these for telemetry tests > > https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in... > File > telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py > (right): > > https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py:354: > if __name__ == '__main__': > Same here FYI: if you just want to run a single test in telemetry, you can do: ./telemetry/bin/run_tests <test_case_name or test_method_name>
That test already found a bug. Also, pylint keeps having these errors when I upload, but the fact that the test runs proves that they can import it... any way to get rid of these? ************* Module telemetry.core.tracing_controller_unittest F: 7, 0: Unable to import 'battor' (import-error) ************* Module telemetry.internal.platform.tracing_agent.battor_tracing_agent F: 7, 0: Unable to import 'battor' (import-error) F: 8, 0: Unable to import 'battor' (import-error) ************* Module telemetry.internal.platform.tracing_agent.battor_tracing_agent_unittest F: 8, 0: Unable to import 'battor' (import-error) F: 9, 0: Unable to import 'battor' (import-error) https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:216: if __name__ == '__main__': On 2016/05/06 02:40:36, nednguyen wrote: > You don't need these for telemetry tests Done. https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py (right): https://codereview.chromium.org/1819183002/diff/530001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_controller_backend_unittest.py:354: if __name__ == '__main__': On 2016/05/06 02:40:36, nednguyen wrote: > Same here Done.
Patchset #21 (id:570001) has been deleted
The CQ bit was checked by rnephew@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819183002/590001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819183002/590001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... File telemetry/telemetry/core/tracing_controller_unittest.py (right): https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... telemetry/telemetry/core/tracing_controller_unittest.py:196: return # Test require battor present. nit: 'Test require battor present.' doesn't seem like a great comment here. How about: 'Don't run the test if no BattOr is connected.' https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... telemetry/telemetry/core/tracing_controller_unittest.py:202: time.sleep(1) # Cannot imediately start then stop tracing. crbug.com/602266 nit: s/imediately/immediately Also, not crazy about tacking on the crbug at the end without including context as to how it's relevant. I think I'd prefer something like: # We wait 1s before starting and stopping tracing to avoid crbug.com/602266, which would cause a crash otherwise. https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:28: self._battor = battor_wrapper.BattorWrapper(platform_backend.GetOSName(), We should change this from BattorWrapper to BattOrWrapper so that we're consistent w/ capitalization across all of our code (not really relevant here, but just pointing it out) https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:35: # TODO(rnephew): When we pass BattOr device map into telemetry, change nit: s/telemetry/Telemetry https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:82: battor_wrapper.BattorWrapper = FakeBattor Do we have to undo this in the tearDown portion? (I've seen this done elsewhere, where we save what was battor_wrapper.BattorWrapper to a tmp variable for the duration of the test, and then switch it back in the tearDown.) It seems like this might impact any tests that want to use the BattorWrapper that run after this https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:102: battor_wrapper.IsBattOrConnected = lambda x, android_device=None: False Could this be lambda *unused, *unused? https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:158: def callback_without_exception(a, b): How are these different behaviors? It seems like what happens in the callback doesn't ahve anything to do with what's happening in the Android agent https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:166: def testNormalRun(self): I'm kind of confused by this test: it doesn't seem to provide any additional value over the tests that you have above, but it's by far the longest test you have. Is it possible to nix this one? https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:167: callback = lambda x, y: None why not *unused, *unused? https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:169: self.assertFalse(self.android_agent._battor._is_shell_running) Don't think that you have to do this round of asserts: it seems true by definition. If this were borked, your tests above would be broken too
https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... File telemetry/telemetry/core/tracing_controller_unittest.py (right): https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... telemetry/telemetry/core/tracing_controller_unittest.py:196: return # Test require battor present. On 2016/05/09 21:30:32, charliea wrote: > nit: 'Test require battor present.' doesn't seem like a great comment here. How > about: 'Don't run the test if no BattOr is connected.' Done. https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... telemetry/telemetry/core/tracing_controller_unittest.py:202: time.sleep(1) # Cannot imediately start then stop tracing. crbug.com/602266 On 2016/05/09 21:30:32, charliea wrote: > nit: s/imediately/immediately > > Also, not crazy about tacking on the crbug at the end without including context > as to how it's relevant. I think I'd prefer something like: > > # We wait 1s before starting and stopping tracing to avoid crbug.com/602266, > which would cause a crash otherwise. Done. https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py (right): https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:28: self._battor = battor_wrapper.BattorWrapper(platform_backend.GetOSName(), On 2016/05/09 21:30:32, charliea wrote: > We should change this from BattorWrapper to BattOrWrapper so that we're > consistent w/ capitalization across all of our code (not really relevant here, > but just pointing it out) Also need to change battor_error, I will do this in a seperate CL after this one lands. https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:35: # TODO(rnephew): When we pass BattOr device map into telemetry, change On 2016/05/09 21:30:32, charliea wrote: > nit: s/telemetry/Telemetry Done. https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py (right): https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:82: battor_wrapper.BattorWrapper = FakeBattor On 2016/05/09 21:30:32, charliea wrote: > Do we have to undo this in the tearDown portion? (I've seen this done elsewhere, > where we save what was battor_wrapper.BattorWrapper to a tmp variable for the > duration of the test, and then switch it back in the tearDown.) > > It seems like this might impact any tests that want to use the BattorWrapper > that run after this Done. https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:102: battor_wrapper.IsBattOrConnected = lambda x, android_device=None: False On 2016/05/09 21:30:33, charliea wrote: > Could this be lambda *unused, *unused? keyword arguments make *unused impossible to use, at least thats been my experience. https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:158: def callback_without_exception(a, b): On 2016/05/09 21:30:32, charliea wrote: > How are these different behaviors? It seems like what happens in the callback > doesn't ahve anything to do with what's happening in the Android agent This just tests that the exception in the callback isnt being swallowed somewhere. https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:166: def testNormalRun(self): On 2016/05/09 21:30:33, charliea wrote: > I'm kind of confused by this test: it doesn't seem to provide any additional > value over the tests that you have above, but it's by far the longest test you > have. Is it possible to nix this one? Its just an end to end normal run of it, it probably doenst cover anything the above tests do not cover.
On 2016/05/10 15:51:44, rnephew1 wrote: > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... > File telemetry/telemetry/core/tracing_controller_unittest.py (right): > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... > telemetry/telemetry/core/tracing_controller_unittest.py:196: return # Test > require battor present. > On 2016/05/09 21:30:32, charliea wrote: > > nit: 'Test require battor present.' doesn't seem like a great comment here. > How > > about: 'Don't run the test if no BattOr is connected.' > > Done. > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... > telemetry/telemetry/core/tracing_controller_unittest.py:202: time.sleep(1) # > Cannot imediately start then stop tracing. crbug.com/602266 > On 2016/05/09 21:30:32, charliea wrote: > > nit: s/imediately/immediately > > > > Also, not crazy about tacking on the crbug at the end without including > context > > as to how it's relevant. I think I'd prefer something like: > > > > # We wait 1s before starting and stopping tracing to avoid crbug.com/602266, > > which would cause a crash otherwise. > > Done. > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > File telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py > (right): > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:28: > self._battor = battor_wrapper.BattorWrapper(platform_backend.GetOSName(), > On 2016/05/09 21:30:32, charliea wrote: > > We should change this from BattorWrapper to BattOrWrapper so that we're > > consistent w/ capitalization across all of our code (not really relevant here, > > but just pointing it out) > > Also need to change battor_error, I will do this in a seperate CL after this one > lands. > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:35: > # TODO(rnephew): When we pass BattOr device map into telemetry, change > On 2016/05/09 21:30:32, charliea wrote: > > nit: s/telemetry/Telemetry > > Done. > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > File > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py > (right): > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:82: > battor_wrapper.BattorWrapper = FakeBattor > On 2016/05/09 21:30:32, charliea wrote: > > Do we have to undo this in the tearDown portion? (I've seen this done > elsewhere, > > where we save what was battor_wrapper.BattorWrapper to a tmp variable for the > > duration of the test, and then switch it back in the tearDown.) > > > > It seems like this might impact any tests that want to use the BattorWrapper > > that run after this > > Done. > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:102: > battor_wrapper.IsBattOrConnected = lambda x, android_device=None: False > On 2016/05/09 21:30:33, charliea wrote: > > Could this be lambda *unused, *unused? > > keyword arguments make *unused impossible to use, at least thats been my > experience. > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:158: > def callback_without_exception(a, b): > On 2016/05/09 21:30:32, charliea wrote: > > How are these different behaviors? It seems like what happens in the callback > > doesn't ahve anything to do with what's happening in the Android agent > > This just tests that the exception in the callback isnt being swallowed > somewhere. > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:166: > def testNormalRun(self): > On 2016/05/09 21:30:33, charliea wrote: > > I'm kind of confused by this test: it doesn't seem to provide any additional > > value over the tests that you have above, but it's by far the longest test you > > have. Is it possible to nix this one? > > Its just an end to end normal run of it, it probably doenst cover anything the > above tests do not cover. Ping on adding tracing_controller_unittest that cover battor
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819183002/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819183002/610001
On 2016/05/10 20:43:28, nednguyen wrote: > On 2016/05/10 15:51:44, rnephew1 wrote: > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... > > File telemetry/telemetry/core/tracing_controller_unittest.py (right): > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... > > telemetry/telemetry/core/tracing_controller_unittest.py:196: return # Test > > require battor present. > > On 2016/05/09 21:30:32, charliea wrote: > > > nit: 'Test require battor present.' doesn't seem like a great comment here. > > How > > > about: 'Don't run the test if no BattOr is connected.' > > > > Done. > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/co... > > telemetry/telemetry/core/tracing_controller_unittest.py:202: time.sleep(1) # > > Cannot imediately start then stop tracing. crbug.com/602266 > > On 2016/05/09 21:30:32, charliea wrote: > > > nit: s/imediately/immediately > > > > > > Also, not crazy about tacking on the crbug at the end without including > > context > > > as to how it's relevant. I think I'd prefer something like: > > > > > > # We wait 1s before starting and stopping tracing to avoid crbug.com/602266, > > > which would cause a crash otherwise. > > > > Done. > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > > File > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py > > (right): > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:28: > > self._battor = battor_wrapper.BattorWrapper(platform_backend.GetOSName(), > > On 2016/05/09 21:30:32, charliea wrote: > > > We should change this from BattorWrapper to BattOrWrapper so that we're > > > consistent w/ capitalization across all of our code (not really relevant > here, > > > but just pointing it out) > > > > Also need to change battor_error, I will do this in a seperate CL after this > one > > lands. > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent.py:35: > > # TODO(rnephew): When we pass BattOr device map into telemetry, change > > On 2016/05/09 21:30:32, charliea wrote: > > > nit: s/telemetry/Telemetry > > > > Done. > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > > File > > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py > > (right): > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:82: > > battor_wrapper.BattorWrapper = FakeBattor > > On 2016/05/09 21:30:32, charliea wrote: > > > Do we have to undo this in the tearDown portion? (I've seen this done > > elsewhere, > > > where we save what was battor_wrapper.BattorWrapper to a tmp variable for > the > > > duration of the test, and then switch it back in the tearDown.) > > > > > > It seems like this might impact any tests that want to use the BattorWrapper > > > that run after this > > > > Done. > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:102: > > battor_wrapper.IsBattOrConnected = lambda x, android_device=None: False > > On 2016/05/09 21:30:33, charliea wrote: > > > Could this be lambda *unused, *unused? > > > > keyword arguments make *unused impossible to use, at least thats been my > > experience. > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:158: > > def callback_without_exception(a, b): > > On 2016/05/09 21:30:32, charliea wrote: > > > How are these different behaviors? It seems like what happens in the > callback > > > doesn't ahve anything to do with what's happening in the Android agent > > > > This just tests that the exception in the callback isnt being swallowed > > somewhere. > > > > > https://codereview.chromium.org/1819183002/diff/590001/telemetry/telemetry/in... > > > telemetry/telemetry/internal/platform/tracing_agent/battor_tracing_agent_unittest.py:166: > > def testNormalRun(self): > > On 2016/05/09 21:30:33, charliea wrote: > > > I'm kind of confused by this test: it doesn't seem to provide any additional > > > value over the tests that you have above, but it's by far the longest test > you > > > have. Is it possible to nix this one? > > > > Its just an end to end normal run of it, it probably doenst cover anything the > > above tests do not cover. > > Ping on adding tracing_controller_unittest that cover battor nvm, I just saw it :P
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1819183002/diff/610001/telemetry/telemetry/co... File telemetry/telemetry/core/tracing_controller_unittest.py (right): https://codereview.chromium.org/1819183002/diff/610001/telemetry/telemetry/co... telemetry/telemetry/core/tracing_controller_unittest.py:202: # We wait 1s before starting and stopping tracing to vaoid crbug.com/602266, nits: "to avoid".
lgtm w/ a nit https://codereview.chromium.org/1819183002/diff/610001/telemetry/telemetry/co... File telemetry/telemetry/core/tracing_controller_unittest.py (right): https://codereview.chromium.org/1819183002/diff/610001/telemetry/telemetry/co... telemetry/telemetry/core/tracing_controller_unittest.py:203: # which would cause a crash otherwise nit: end in a period.
https://codereview.chromium.org/1819183002/diff/610001/telemetry/telemetry/co... File telemetry/telemetry/core/tracing_controller_unittest.py (right): https://codereview.chromium.org/1819183002/diff/610001/telemetry/telemetry/co... telemetry/telemetry/core/tracing_controller_unittest.py:202: # We wait 1s before starting and stopping tracing to vaoid crbug.com/602266, On 2016/05/10 21:20:35, nednguyen wrote: > nits: "to avoid". Done. https://codereview.chromium.org/1819183002/diff/610001/telemetry/telemetry/co... telemetry/telemetry/core/tracing_controller_unittest.py:203: # which would cause a crash otherwise On 2016/05/11 13:54:06, charliea wrote: > nit: end in a period. Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, charliea@chromium.org Link to the patchset: https://codereview.chromium.org/1819183002/#ps630001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819183002/630001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819183002/630001
Message was sent while issue was closed.
Description was changed from ========== [Telemetry] Add battor tracing agent. BUG=chromium:574888 ========== to ========== [Telemetry] Add battor tracing agent. BUG=chromium:574888 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #23 (id:630001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
A revert of this CL (patchset #23 id:630001) has been created in https://codereview.chromium.org/1967243002/ by rnephew@chromium.org. The reason for reverting is: crbug.com/611159. |
