Android: Implement automatic trace pulling for gtests [chromium/src : main]
Kartar Singh (Gerrit)
Kartar Singh added 1 comment![]( "Open in Gerrit")
Hi @khok...@google.com,
Can you please take a look at the change mainly at the part changing default save path from `chrometrace.log` to `chrome.pftrace`. Does it look safe to you, or do we need to do more things before we can change this default save path.
Related details
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mikhail Khokhlov
| Code-Review | +1 |
2 comments
Hi @khok...@google.com,
Can you please take a look at the change mainly at the part changing default save path from `chrometrace.log` to `chrome.pftrace`. Does it look safe to you, or do we need to do more things before we can change this default save path.
You never know who depends on things like that before you change them 😊
In general, it should be safe though, since tracing is a developer tool. Even if some tool assumes a specific path, it's always possible to provide a `--trace-startup-file` explicitly.
return BasenameToPath("chrome.pftrace");Please also update the comment in [tracing_switches.cc](https://source.chromium.org/chromium/chromium/src/+/main:components/tracing/common/tracing_switches.cc;l=73?q=chrometrace.log)
Related details
- Kartar Singh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh added 2 comments![]( "Open in Gerrit")
Mikhail KhokhlovHi @khok...@google.com,
Can you please take a look at the change mainly at the part changing default save path from `chrometrace.log` to `chrome.pftrace`. Does it look safe to you, or do we need to do more things before we can change this default save path.
You never know who depends on things like that before you change them 😊
In general, it should be safe though, since tracing is a developer tool. Even if some tool assumes a specific path, it's always possible to provide a `--trace-startup-file` explicitly.
Thanks Mikhail, makes sense.
Please also update the comment in [tracing_switches.cc](https://source.chromium.org/chromium/chromium/src/+/main:components/tracing/common/tracing_switches.cc;l=73?q=chrometrace.log)
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh added 1 comment![]( "Open in Gerrit")
Hi @hy...@google.com,
Can you please take a look at the change. We are now pulling the traces from device for local test run into `TEST_*` directory in out.
Though there is an issue when `--skip-clear-data` is passed, which will retain the traces across multiple runs. What do you suggest should we try to solve this here, or is it fine to submit it as is? If we want to solve this we can probably keep a list of traces before the test run and just pull the new traces after the test run.
Related details
- Haiyang Pan
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh added 1 comment![]( "Open in Gerrit")
f, 'traces', datatype=output_manager.Datatype.TEXT) as host_file:Traces are binary files, should we create a new `Datatype` here? Or is it fine to use TEXT here? @hy...@google.com
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mikhail Khokhlov
| Code-Review | +1 |
1 comment
- Haiyang Pan
- Kartar Singh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haiyang Pan (Gerrit)
Haiyang Pan added 1 comment![]( "Open in Gerrit")
f, 'traces', datatype=output_manager.Datatype.TEXT) as host_file:Traces are binary files, should we create a new `Datatype` here? Or is it fine to use TEXT here? @hy...@google.com
Given traces are binary files, we should not use TEXT here.
The `output_manager` can upload the file to a remote storage service (logdog or gcs), or store locally, depending on if the test runner is called locally or on swarming.
Do you only want to pull the trace file only to your local environment or also upload it?
Related details
- Kartar Singh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh added 1 comment![]( "Open in Gerrit")
f, 'traces', datatype=output_manager.Datatype.TEXT) as host_file:Haiyang PanTraces are binary files, should we create a new `Datatype` here? Or is it fine to use TEXT here? @hy...@google.com
Given traces are binary files, we should not use TEXT here.
The `output_manager` can upload the file to a remote storage service (logdog or gcs), or store locally, depending on if the test runner is called locally or on swarming.
Do you only want to pull the trace file only to your local environment or also upload it?
Do you only want to pull the trace file only to your local environment or also upload it?
The current use case I am looking to solve is only for local development i.e. pull trace into local environment no uploads.
Related details
- Haiyang Pan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh added 1 comment![]( "Open in Gerrit")
f, 'traces', datatype=output_manager.Datatype.TEXT) as host_file:Haiyang PanTraces are binary files, should we create a new `Datatype` here? Or is it fine to use TEXT here? @hy...@google.com
Kartar SinghGiven traces are binary files, we should not use TEXT here.
The `output_manager` can upload the file to a remote storage service (logdog or gcs), or store locally, depending on if the test runner is called locally or on swarming.
Do you only want to pull the trace file only to your local environment or also upload it?
Do you only want to pull the trace file only to your local environment or also upload it?
The current use case I am looking to solve is only for local development i.e. pull trace into local environment no uploads.
Updated to used newly added Datatype.BINARY. Could you please take a look.
The pulled trace has this mime type:
```
$ file --mime-type test.pftrace
test.pftrace: application/octet-stream
```
Related details
- Haiyang Pan
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haiyang Pan (Gerrit)
Haiyang Pan added 3 comments![]( "Open in Gerrit")
Android: Implement automatic trace pulling for gtestsCan you create a bug if you haven't done so? And put the bug number in the commit message, as well as in the `local_device_gtest_run.py` (See my comment here.)
f, 'traces', datatype=output_manager.Datatype.TEXT) as host_file:Haiyang PanTraces are binary files, should we create a new `Datatype` here? Or is it fine to use TEXT here? @hy...@google.com
Kartar SinghGiven traces are binary files, we should not use TEXT here.
The `output_manager` can upload the file to a remote storage service (logdog or gcs), or store locally, depending on if the test runner is called locally or on swarming.
Do you only want to pull the trace file only to your local environment or also upload it?
Kartar SinghDo you only want to pull the trace file only to your local environment or also upload it?
The current use case I am looking to solve is only for local development i.e. pull trace into local environment no uploads.
Updated to used newly added Datatype.BINARY. Could you please take a look.
The pulled trace has this mime type:
```
$ file --mime-type test.pftrace
test.pftrace: application/octet-stream
```
Done
self._PullSavedTraces(device)Can you add something like this, to ensure it is pulling only for local run?
Also please add the bug link here so that users can read more if interested
```
# Pull trace files for local run only. See crbug.com/XXX for details.
if isinstance(self._env.output_manager, local_output_manager.LocalOutputManager):
self._PullSavedTraces(device)
```
Related details
- Kartar Singh
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh added 2 comments![]( "Open in Gerrit")
Can you create a bug if you haven't done so? And put the bug number in the commit message, as well as in the `local_device_gtest_run.py` (See my comment here.)
Done
Can you add something like this, to ensure it is pulling only for local run?
Also please add the bug link here so that users can read more if interested
```
# Pull trace files for local run only. See crbug.com/XXX for details.
if isinstance(self._env.output_manager, local_output_manager.LocalOutputManager):
self._PullSavedTraces(device)
```
- Haiyang Pan
- Mikhail Khokhlov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mikhail Khokhlov (Gerrit)
Mikhail Khokhlov voted Code-Review+1![]( "Open in Gerrit")
- Haiyang Pan
- Kartar Singh
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Haiyang Pan (Gerrit)
Haiyang Pan voted and added 2 comments![]( "Open in Gerrit")
Votes added by Haiyang Pan
| Code-Review | +1 |
Hi @hy...@google.com,
Can you please take a look at the change. We are now pulling the traces from device for local test run into `TEST_*` directory in out.
Though there is an issue when `--skip-clear-data` is passed, which will retain the traces across multiple runs. What do you suggest should we try to solve this here, or is it fine to submit it as is? If we want to solve this we can probably keep a list of traces before the test run and just pull the new traces after the test run.
If the pftrace files are not big, I think it is probably fine to pull them everytime?
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh added 1 comment![]( "Open in Gerrit")
Haiyang PanHi @hy...@google.com,
Can you please take a look at the change. We are now pulling the traces from device for local test run into `TEST_*` directory in out.
Though there is an issue when `--skip-clear-data` is passed, which will retain the traces across multiple runs. What do you suggest should we try to solve this here, or is it fine to submit it as is? If we want to solve this we can probably keep a list of traces before the test run and just pull the new traces after the test run.
If the pftrace files are not big, I think it is probably fine to pull them everytime?
They are <1mb each so not terribly big. We can wait for developers to actually start using the feature before we expand implementation.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kartar Singh (Gerrit)
Kartar Singh voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Android: Implement automatic trace pulling for gtests
This CL implements host-side automatic pulling of generated Perfetto
traces (*.pftrace) from Android devices to the host's
TEST_RESULTS_*/traces/ folder.
The change also rename default startup trace basename from
"chrometrace.log" to "chrome.pftrace" to match the host-side pull filter
for "*.pftrace" files.
- M build/android/pylib/base/output_manager.py
- M build/android/pylib/local/device/local_device_gtest_run.py
- M components/tracing/common/tracing_switches.cc
- M services/tracing/public/cpp/startup_tracing_controller.cc
- M services/tracing/public/cpp/trace_startup_config.cc
- M services/tracing/public/cpp/trace_startup_config.h
Code-Review: +1 by Mikhail Khokhlov, +1 by Haiyang Pan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |