tracing: Relocate database persistence layer and decouple metrics provider [chromium/src : main]
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
1 comment
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 8 comments![]( "Open in Gerrit")
"//content/public/browser",I think this isn't needed at all anymore with BackgroundTracingMetricsProvider changes
base::OnceCallback<
void(std::optional<std::string> compressed_trace_content,
std::optional<std::string> serialized_system_profile,
base::OnceClosure upload_complete)> callback) = 0;Nit: let's define an alias (OnTraceToUploadCallback) to use here and in derived classes.
db_path = database_dir->Append(FILE_PATH_LITERAL("trace_report.db"));This is already included in GetLocalTracesDirectory?
if (self && !success) {
self->trace_report_manager_.reset();
}Let's keep the existing behavior: if this fails, trace_report_manager_ is left non-null, but every subsequent operation will fail. This prevent trying to reopen the database each time this is called.
if (!database_) {
database_ = {new TraceReportDatabase,
base::OnTaskRunnerDeleter(database_task_runner_)};
}Let's move this in constructor and assume that `database_` is always valid.
std::optional<ClientTraceReport> client_report =
db->GetNextReportPendingUpload();
if (client_report) {
result.pending_report =
std::optional<BaseTraceReport>(*client_report);
}Nit: Does it works to rely on optional conversion?
`result.pending_report = std::move(client_report)`
if (!database_) {
ui_runner->PostTask(
FROM_HERE, base::BindOnce(std::move(on_done), std::nullopt, false));
return;
}Here and throughout file below: we previously assumed that database_ is initialized once and never reset. If initialization fails, it's kept non-null but operation will fail.
So it should be enough to make this `CHECK(database_)`
if (!database || !database->is_initialized()) {Related to comment above: `!database` isn't needed (redundant).
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 1 comment![]( "Open in Gerrit")
db_path = database_dir->Append(FILE_PATH_LITERAL("trace_report.db"));This is already included in GetLocalTracesDirectory?
Is it? It looks like it GetLocalTracesDirectory() only returns the base directory path (mapping to chrome::DIR_LOCAL_TRACES / android_webview::DIR_LOCAL_TRACES)
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 1 comment![]( "Open in Gerrit")
db_path = database_dir->Append(FILE_PATH_LITERAL("trace_report.db"));Justin CohenThis is already included in GetLocalTracesDirectory?
Is it? It looks like it GetLocalTracesDirectory() only returns the base directory path (mapping to chrome::DIR_LOCAL_TRACES / android_webview::DIR_LOCAL_TRACES)
Oh sorry, it's included in TraceReportDatabase::OpenDatabase rather:
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 9 comments![]( "Open in Gerrit")
I think this isn't needed at all anymore with BackgroundTracingMetricsProvider changes
Done
base::OnceCallback<
void(std::optional<std::string> compressed_trace_content,
std::optional<std::string> serialized_system_profile,
base::OnceClosure upload_complete)> callback) = 0;Nit: let's define an alias (OnTraceToUploadCallback) to use here and in derived classes.
Done
db_path = database_dir->Append(FILE_PATH_LITERAL("trace_report.db"));Justin CohenThis is already included in GetLocalTracesDirectory?
Etienne Pierre-DorayIs it? It looks like it GetLocalTracesDirectory() only returns the base directory path (mapping to chrome::DIR_LOCAL_TRACES / android_webview::DIR_LOCAL_TRACES)
Oh sorry, it's included in TraceReportDatabase::OpenDatabase rather:
Done
if (self && !success) {
self->trace_report_manager_.reset();
}Let's keep the existing behavior: if this fails, trace_report_manager_ is left non-null, but every subsequent operation will fail. This prevent trying to reopen the database each time this is called.
Done
if (!database_) {
database_ = {new TraceReportDatabase,
base::OnTaskRunnerDeleter(database_task_runner_)};
}Let's move this in constructor and assume that `database_` is always valid.
Done
std::optional<ClientTraceReport> client_report =
db->GetNextReportPendingUpload();
if (client_report) {
result.pending_report =
std::optional<BaseTraceReport>(*client_report);
}Nit: Does it works to rely on optional conversion?
`result.pending_report = std::move(client_report)`
Done
if (!database_) {
ui_runner->PostTask(
FROM_HERE, base::BindOnce(std::move(on_done), std::nullopt, false));
return;
}Here and throughout file below: we previously assumed that database_ is initialized once and never reset. If initialization fails, it's kept non-null but operation will fail.
So it should be enough to make this `CHECK(database_)`
Done
if (!database || !database->is_initialized()) {Related to comment above: `!database` isn't needed (redundant).
- 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 6 comments![]( "Open in Gerrit")
// We limit uploads of 1 trace per scenario over a period of 7 days. Since
// traces live in the database for longer than 7 days, their TTL doesn't affect
// this unless the database is manually cleared.Let's keep the comment along with the variable (now moved to services/tracing/public/cpp/background_tracing/trace_report_manager.cc)
trace_report, std::move(receive_callback),Can we keep the `receive_callback` running synchronously? Ideally this simply becomes
```
auto on_finalize_complete = base::BindOnce(&BackgroundTracingManagerImpl::OnFinalizeComplete,
weak_factory_.GetWeakPtr());
trace_report_manager_->LoadTraceContent(
uuid,
std::move(receive_callback), // runs synchronously
std::move(on_finalize_complete) // runs on UI task runner
);
```
Which chains GetTraceContent -> receive_callback -> UploadComplete -> OnFinalizeComplete (really which we had coroutines)
And "Tracing.Background.Scenario.Upload" can probably move to OnFinalizeComplete
void Reset();Is this needed, or could we simply rely on in-memory database getting cleaned up?
if (!database->is_initialized()) {
ui_runner->PostTask(
FROM_HERE,
base::BindOnce(std::move(on_done), std::nullopt, false));
return;
}Previously we just dropped it, because we'd already reported failure to initialize database.
#if BUILDFLAG(IS_IOS)
if (privacy_filter_enabled) {
PrivacyFilteringCheck::RemoveBlockedFields(serialized_trace);
}
#endifI'll have to think about this, but since this CL is a move/refactor, I think it makes more sense to introduce in follow-up.
ui_runner->PostTask(
FROM_HERE,
base::BindOnce(std::move(on_done),
std::move(report_to_upload), false));Previously we just dropped it, and thus not reporting an error for quota limitation.
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 7 comments![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
7 comments
// We limit uploads of 1 trace per scenario over a period of 7 days. Since
// traces live in the database for longer than 7 days, their TTL doesn't affect
// this unless the database is manually cleared.Let's keep the comment along with the variable (now moved to services/tracing/public/cpp/background_tracing/trace_report_manager.cc)
Done
trace_report, std::move(receive_callback),Can we keep the `receive_callback` running synchronously? Ideally this simply becomes
```
auto on_finalize_complete = base::BindOnce(&BackgroundTracingManagerImpl::OnFinalizeComplete,
weak_factory_.GetWeakPtr());trace_report_manager_->LoadTraceContent(
uuid,
std::move(receive_callback), // runs synchronously
std::move(on_finalize_complete) // runs on UI task runner
);
```Which chains GetTraceContent -> receive_callback -> UploadComplete -> OnFinalizeComplete (really which we had coroutines)
And "Tracing.Background.Scenario.Upload" can probably move to OnFinalizeComplete
Done
Is this needed, or could we simply rely on in-memory database getting cleaned up?
Done
if (!database->is_initialized()) {
ui_runner->PostTask(
FROM_HERE,
base::BindOnce(std::move(on_done), std::nullopt, false));
return;
}Previously we just dropped it, because we'd already reported failure to initialize database.
Done
#if BUILDFLAG(IS_IOS)
if (privacy_filter_enabled) {
PrivacyFilteringCheck::RemoveBlockedFields(serialized_trace);
}
#endifI'll have to think about this, but since this CL is a move/refactor, I think it makes more sense to introduce in follow-up.
OK -- for now I'm leaving it in this CL. Alternatively I can move it to https://crrev.com/c/7819834 ? Or would you prefer something different.
ui_runner->PostTask(
FROM_HERE,
base::BindOnce(std::move(on_done),
std::move(report_to_upload), false));Previously we just dropped it, and thus not reporting an error for quota limitation.
- 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. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
1 comment
#if BUILDFLAG(IS_IOS)
if (privacy_filter_enabled) {
PrivacyFilteringCheck::RemoveBlockedFields(serialized_trace);
}
#endifJustin CohenI'll have to think about this, but since this CL is a move/refactor, I think it makes more sense to introduce in follow-up.
OK -- for now I'm leaving it in this CL. Alternatively I can move it to https://crrev.com/c/7819834 ? Or would you prefer something different.
I misread -- moving it to the child CL.
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 3 comments![]( "Open in Gerrit")
std::optional<std::string> system_profile) {Nit: This is the only caller of this version of LoadTraceContent() I think, so we could remove system_profile?
database_task_runner_->PostTaskAndReplyWithResult(I meant we can skip the hop back to the "original" task runner (to do CompleteUpload). Something like:
```
auto ui_runner = base::SequencedTaskRunner::GetCurrentDefault();
auto on_upload_complete = base::BindPostTask(database_task_runner_, base::BindOnce([](
tracing::TraceReportDatabase* database,
base::Token uuid
base::OnceCallback<...> on_finalize_complete) {
std::optional<ClientTraceReport> next_report;
if (database->UploadComplete(uuid, base::Time::Now())) {
next_report = database->GetNextReportPendingUpload();
}
std::move(on_finalize_complete).Run(std::move(next_report), true);
}, database_.get(), uuid,base::BindPostTask(ui_runner, std::move(on_finalize_complete))));
auto load_trace_content = base::BindOnce([](
tracing::TraceReportDatabase* database,
base::Token uuid,
base::OnceCallback<...> receive_callback,
base::OnceCallback<...> on_upload_complete) {
auto compressed_trace_content = database->GetTraceContent(uuid);
if (!compressed_trace_content) {
std::move(receive_callback)
.Run(std::nullopt, std::nullopt, base::NullCallback());
} else {
auto serialized_system_profile = database->GetSystemProfile(uuid);
std::move(receive_callback)
.Run(std::move(compressed_trace_content),
std::move(serialized_system_profile), std::move(upload_complete));
}
}, database_.get(), uuid, std::move(receive_callback), std::move(on_upload_complete));
database_task_runner_->PostTask(database_task_runner_, std::move(load_trace_content));
```
[](base::WeakPtr<TraceReportManager> self, base::Token uuid,Nit: I think we can plumb TraceReportDatabase* the same we we do in other functions (and thus don't need a WeakPtrFactory), see above.
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. |