ios: Integrate Perfetto background tracing with UMA [chromium/src : main]
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen abandoned this change.
Justin Cohen abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
ptal
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 5 comments![]( "Open in Gerrit")
class IOSChromeBackgroundTracingMetricsProviderIs it worth inheriting from BackgroundTracingMetricsProvider (the same way you'd done in https://crrev.com/c/7819834)?
That would give us ProvideIndependentMetrics for free.
auto tracing_scenarios_config = tracing::GetFieldTracingScenariosConfig();
if (tracing_scenarios_config) {
IOSTracingController::GetInstance().InitializeFieldScenarios(
*tracing_scenarios_config, BackgroundTracingManager::ANONYMIZE_DATA,
tracing::kFieldTracingForceUploads.Get(),
tracing::kFieldTracingUploadLimitKb.Get());
}Now that BackgroundTracingManager is in services,
SetupFieldTracingFromFieldTrial() doesn't need to depend on content/ (this might be true for all of background_tracing_utils target), and could probably be reused here.
InitializeTraceReportDatabase();InitializeTraceReportDatabase is generally called lazily (e.g. in InitializeFieldScenarios), to avoid unnecessarily loading the database for most of stable users (since the field tracing config generally only runs on prestable).
active_scenario_ = nullptr;DisableScenarios() right below should handle this.
if (base::ThreadPoolInstance::Get()) {
base::ThreadPoolInstance::Get()->FlushForTesting(); // IN-TEST
}Ideally this is done through TaskEnvironment in the test suite?
Related details
- Justin Cohen
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. |
Justin Cohen (Gerrit)
Justin Cohen added 5 comments![]( "Open in Gerrit")
class IOSChromeBackgroundTracingMetricsProviderIs it worth inheriting from BackgroundTracingMetricsProvider (the same way you'd done in https://crrev.com/c/7819834)?
That would give us ProvideIndependentMetrics for free.
I'll look into it now. If the refactor is too complicated, perhaps that would be useful as a followup.
auto tracing_scenarios_config = tracing::GetFieldTracingScenariosConfig();
if (tracing_scenarios_config) {
IOSTracingController::GetInstance().InitializeFieldScenarios(
*tracing_scenarios_config, BackgroundTracingManager::ANONYMIZE_DATA,
tracing::kFieldTracingForceUploads.Get(),
tracing::kFieldTracingUploadLimitKb.Get());
}Now that BackgroundTracingManager is in services,
SetupFieldTracingFromFieldTrial() doesn't need to depend on content/ (this might be true for all of background_tracing_utils target), and could probably be reused here.
I'll look into it now. If the refactor is too complicated, perhaps that would be useful as a followup.
InitializeTraceReportDatabase is generally called lazily (e.g. in InitializeFieldScenarios), to avoid unnecessarily loading the database for most of stable users (since the field tracing config generally only runs on prestable).
Done
DisableScenarios() right below should handle this.
Done
if (base::ThreadPoolInstance::Get()) {
base::ThreadPoolInstance::Get()->FlushForTesting(); // IN-TEST
}Ideally this is done through TaskEnvironment in the test suite?
If we pull it out of here we need to add it to IOSTracingControllerTest::StartupTraceRecording and any other test that needs to reset/re-initialize within the test. I thought it was simpler to group it together. LMKWYT.
Related details
- Etienne Pierre-Doray
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 7 comments![]( "Open in Gerrit")
Mostly LG
In chrome/, we handle ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData (delete browsing history button)
by calling DeleteTracesInDateRange():
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc;l=563-564;drc=ebae80fd6865f2728dddad89f1af342779c789b5;bpv=1;bpt=1
I'm wondering if iOS has a equivalent.
"//content/public/browser",Nit: I don't think that's actually needed
if (base::ThreadPoolInstance::Get()) {
base::ThreadPoolInstance::Get()->FlushForTesting(); // IN-TEST
}Justin CohenIdeally this is done through TaskEnvironment in the test suite?
If we pull it out of here we need to add it to IOSTracingControllerTest::StartupTraceRecording and any other test that needs to reset/re-initialize within the test. I thought it was simpler to group it together. LMKWYT.
Yes, let's do it in the tests if that works.
base::FilePath user_data_dir;
if (base::PathService::Get(ios::DIR_USER_DATA, &user_data_dir)) {
base::DeleteFile(
user_data_dir.Append(FILE_PATH_LITERAL("LocalTraces.db")));
base::DeleteFile(
user_data_dir.Append(FILE_PATH_LITERAL("LocalTraces.db-journal")));
}In other tests we get BackgroundTracingManager to use in-memory database by calling `InitializeTraceReportDatabase(/*open_in_memory*/true)` first
IOSTracingController::GetInstance().ResetForTesting();Is this needed, or add a comment why IOSTracingControllerTest::TearDown() isn't enough. (same below)
TEST_F(IOSTracingControllerTest, GroomingAndPruningLogic) {
base::ThreadPoolInstance::Get()->FlushForTesting();
auto& instance = IOSTracingController::GetInstance();
size_t reports = GetDatabaseReportCount(instance);
EXPECT_LE(reports, 30u);
}I feel like that's a test that would belong in services/tracing/public/cpp/background_tracing/background_tracing_manager_unittest.cc, though I'm not sure if there's coverage for this behavior already.
Related details
- Justin Cohen
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. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 9 comments![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
Mostly LG
In chrome/, we handle ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData (delete browsing history button)
by calling DeleteTracesInDateRange():
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc;l=563-564;drc=ebae80fd6865f2728dddad89f1af342779c789b5;bpv=1;bpt=1I'm wondering if iOS has a equivalent.
It does -- thank you for catching this!
"//content/public/browser",Nit: I don't think that's actually needed
Done
class IOSChromeBackgroundTracingMetricsProviderJustin CohenIs it worth inheriting from BackgroundTracingMetricsProvider (the same way you'd done in https://crrev.com/c/7819834)?
That would give us ProvideIndependentMetrics for free.
I'll look into it now. If the refactor is too complicated, perhaps that would be useful as a followup.
Done
auto tracing_scenarios_config = tracing::GetFieldTracingScenariosConfig();
if (tracing_scenarios_config) {
IOSTracingController::GetInstance().InitializeFieldScenarios(
*tracing_scenarios_config, BackgroundTracingManager::ANONYMIZE_DATA,
tracing::kFieldTracingForceUploads.Get(),
tracing::kFieldTracingUploadLimitKb.Get());
}Justin CohenNow that BackgroundTracingManager is in services,
SetupFieldTracingFromFieldTrial() doesn't need to depend on content/ (this might be true for all of background_tracing_utils target), and could probably be reused here.
I'll look into it now. If the refactor is too complicated, perhaps that would be useful as a followup.
Done
if (base::ThreadPoolInstance::Get()) {
base::ThreadPoolInstance::Get()->FlushForTesting(); // IN-TEST
}Justin CohenIdeally this is done through TaskEnvironment in the test suite?
Etienne Pierre-DorayIf we pull it out of here we need to add it to IOSTracingControllerTest::StartupTraceRecording and any other test that needs to reset/re-initialize within the test. I thought it was simpler to group it together. LMKWYT.
Yes, let's do it in the tests if that works.
Done
namespace {Justin CohenNit: extra line
Done
base::FilePath user_data_dir;
if (base::PathService::Get(ios::DIR_USER_DATA, &user_data_dir)) {
base::DeleteFile(
user_data_dir.Append(FILE_PATH_LITERAL("LocalTraces.db")));
base::DeleteFile(
user_data_dir.Append(FILE_PATH_LITERAL("LocalTraces.db-journal")));
}In other tests we get BackgroundTracingManager to use in-memory database by calling `InitializeTraceReportDatabase(/*open_in_memory*/true)` first
Done
IOSTracingController::GetInstance().ResetForTesting();Is this needed, or add a comment why IOSTracingControllerTest::TearDown() isn't enough. (same below)
Done
TEST_F(IOSTracingControllerTest, GroomingAndPruningLogic) {
base::ThreadPoolInstance::Get()->FlushForTesting();
auto& instance = IOSTracingController::GetInstance();
size_t reports = GetDatabaseReportCount(instance);
EXPECT_LE(reports, 30u);
}I feel like that's a test that would belong in services/tracing/public/cpp/background_tracing/background_tracing_manager_unittest.cc, though I'm not sure if there's coverage for this behavior already.
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 2 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
2 comments
if (IOSTracingController::HasInstance()) {Nit: I'd expect IOSTracingController to be created unconditionally here?
https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/web/model/chrome_main_parts.mm;l=194;drc=ebae80fd6865f2728dddad89f1af342779c789b5;bpv=1;bpt=1
Or maybe this isn't true in some testing environments?
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. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
if (IOSTracingController::HasInstance()) {Nit: I'd expect IOSTracingController to be created unconditionally here?
https://source.chromium.org/chromium/chromium/src/+/main:ios/chrome/browser/web/model/chrome_main_parts.mm;l=194;drc=ebae80fd6865f2728dddad89f1af342779c789b5;bpv=1;bpt=1Or maybe this isn't true in some testing environments?
Correct, this is for tests.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rohit Rao (Gerrit)
Rohit Rao voted and added 3 comments![]( "Open in Gerrit")
Votes added by Rohit Rao
| Code-Review | +1 |
3 comments
return false;Will this always be false or is there a TODO here?
return true;Will this always be true or is there a TODO here?
return true;Will this always be true or is there a TODO here?
Related details
- Justin Cohen
- Justin Novosad
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. |
Justin Cohen (Gerrit)
Justin Cohen added 3 comments![]( "Open in Gerrit")
return false;Will this always be false or is there a TODO here?
This will always be false for iOS
return true;Will this always be true or is there a TODO here?
always true
return true;Will this always be true or is there a TODO here?
I'm not sure -- @etiennep should this ever be false on iOS?
Related details
- Justin Novosad
- Rohit Rao
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. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
return true;Justin CohenWill this always be true or is there a TODO here?
always true
Actually Android returns false, because (1) storage limitations and (2) it doesn't have UI support to see and load unuploaded traces anyways.
return true;Justin CohenWill this always be true or is there a TODO here?
I'm not sure -- @etiennep should this ever be false on iOS?
Yes, actually we should do something similar to ChromeTracingDelegate::IsRecordingAllowed to track incognito
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/tracing/chrome_tracing_delegate.cc;l=119;drc=ebae80fd6865f2728dddad89f1af342779c789b5;bpv=1;bpt=1
Related details
- Justin Cohen
- Justin Novosad
- Rohit Rao
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. |
Justin Cohen (Gerrit)
Justin Cohen added 3 comments![]( "Open in Gerrit")
return false;Justin CohenWill this always be false or is there a TODO here?
Justin CohenThis will always be false for iOS
Done
Justin CohenWill this always be true or is there a TODO here?
Etienne Pierre-Dorayalways true
Actually Android returns false, because (1) storage limitations and (2) it doesn't have UI support to see and load unuploaded traces anyways.
Looks like Android is also true -- leaving as true for iOS for now.
Justin CohenWill this always be true or is there a TODO here?
Etienne Pierre-DorayI'm not sure -- @etiennep should this ever be false on iOS?
Yes, actually we should do something similar to ChromeTracingDelegate::IsRecordingAllowed to track incognito
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/tracing/chrome_tracing_delegate.cc;l=119;drc=ebae80fd6865f2728dddad89f1af342779c789b5;bpv=1;bpt=1
- Justin Novosad
- Rohit Rao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rohit Rao (Gerrit)
Rohit Rao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rohit Rao
| Code-Review | +1 |
1 comment
->HasIncognitoSessionTabs();FYI this will disable tracing if any incognito windows are active. The desktop implementation may on disable if the current window is incognito.
Related details
- Justin Cohen
- Justin Novosad
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. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
1 comment
FYI this will disable tracing if any incognito windows are active. The desktop implementation may on disable if the current window is incognito.
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Justin Novosad
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
22 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ios/chrome/browser/tracing/ios_tracing_controller.mm
Insertions: 1, Deletions: 1.
@@ -172,7 +172,7 @@
std::optional<base::FilePath> IOSTracingController::GetLocalTracesDirectory() {
base::FilePath user_data_dir;
if (base::PathService::Get(ios::DIR_USER_DATA, &user_data_dir)) {
- return user_data_dir;
+ return user_data_dir.Append(FILE_PATH_LITERAL("Local Traces"));
}
return std::nullopt;
}
```
Change information
ios: Integrate Perfetto background tracing with UMA
Enables Chrome on iOS to collect and upload Perfetto background
tracing reports using the unified UMA pipeline.
- Introduces IOSChromeBackgroundTracingMetricsProvider to process and
merge background trace payloads into UMA logs on iOS.
- Adapts the shared BackgroundTracingMetricsProvider and background
tracing utility targets inside //components/tracing to compile
on iOS by abstracting content-layer dependencies.
- Integrates the new tracing metrics provider into
IOSChromeMetricsServiceClient.
- Inherits IOSTracingController from the shared BackgroundTracingManager
base class to manage scenarios, the local trace database, and
uploading, eliminating redundant iOS-specific boilerplate.
- Configures TracingScenario to utilize Perfetto's in-process backend
on iOS builds.
- M components/tracing/BUILD.gn
- M components/tracing/common/background_tracing_metrics_provider.cc
- M components/tracing/common/background_tracing_utils.cc
- M ios/chrome/browser/browsing_data/model/BUILD.gn
- M ios/chrome/browser/browsing_data/model/DEPS
- M ios/chrome/browser/browsing_data/model/browsing_data_remover_impl.mm
- M ios/chrome/browser/metrics/model/BUILD.gn
- M ios/chrome/browser/metrics/model/DEPS
- M ios/chrome/browser/metrics/model/ios_chrome_metrics_service_client.mm
- M ios/chrome/browser/metrics/model/ios_chrome_metrics_service_client_unittest.mm
- M ios/chrome/browser/tracing/BUILD.gn
- M ios/chrome/browser/tracing/DEPS
- A ios/chrome/browser/tracing/ios_chrome_background_tracing_metrics_provider.h
- A ios/chrome/browser/tracing/ios_chrome_background_tracing_metrics_provider.mm
- A ios/chrome/browser/tracing/ios_chrome_background_tracing_metrics_provider_unittest.mm
- M ios/chrome/browser/tracing/ios_tracing_controller.h
- M ios/chrome/browser/tracing/ios_tracing_controller.mm
- M ios/chrome/browser/tracing/ios_tracing_controller_unittest.mm
- M services/tracing/public/cpp/background_tracing/BUILD.gn
- M services/tracing/public/cpp/background_tracing/tracing_scenario.cc
Code-Review: +1 by Etienne Pierre-Doray, +1 by Rohit Rao
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |