DomStorage: Add DbStatus histograms [chromium/src : main]
Rahul Singh (Gerrit)
Rahul Singh voted and added 4 comments![]( "Open in Gerrit")
Votes added by Rahul Singh
| Commit-Queue | +1 |
4 comments
Rahul SinghPart 2 records results using AsyncDomStorageDatabase operations. Should part 1 do the same instead of recording in both LocalStorageImpl and SessionStorageImpl?
Moved everything to fire in AsyncDomStorageDatabase. Thanks!
WaitForDatabaseOpen();Rahul SinghWhy did we need to add this wait? How does the test fail without it?
Good catch. This wait isn't needed here. It's a holdover from when I was hitting flakes in an earlier attempt at adding a test for this histigram.
histograms.ExpectBucketCount("Storage.LocalStorage.Commit.OnDisk", 0, 1);Rahul Singhnit: Is `ExpectUniqueSample()` a stronger assertion here and elsewhere in this change?
Good idea. Updated!
base::BindOnce(&SessionStorageImpl::OnPutMetadataResult,Rahul SinghDo we want to double count PutMetadata() using both the PutMetadata() result and the commit result or should we record the PutMetdata() result only?
Moving histogram logging to AsyncDomStorageDatabase has also fixed this double counting issue. Commit now simply tracks results from calls to UpdateMaps(). As such, I've renamed that variant to `UpdateMaps`
Related details
- Steve Becker
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker added 15 comments![]( "Open in Gerrit")
Thanks for iterating! A few more suggestions.
new AsyncDomStorageDatabase(storage_type, database_path.empty()));nit: maybe label for the reader --
`/*in_memory=*/database_path.empty()`
base::BindOnce(
[](bool in_memory, std::string histogram_name,
StatusOr<std::map<DomStorageDatabase::Key,
DomStorageDatabase::Value>> result) {
if (result.has_value()) {
DbStatus::OK().Log(histogram_name, in_memory);
} else {
result.error().Log(histogram_name, in_memory);
}
return result;
},nit: Let's make some helper functions to make this pattern less verbose:
```
void AsyncDomStorageDatabase::ReadMapKeyValues(
DomStorageDatabase::MapLocator map_locator,
ReadMapKeyValuesCallback callback) {
RunDatabaseTask(
base::BindOnce(
[](DomStorageDatabase::MapLocator map_locator,
DomStorageDatabase& db) {
return db.ReadMapKeyValues(std::move(map_locator));
},
std::move(map_locator)),
base::BindOnce(
&RecordExpected<
std::map<DomStorageDatabase::Key, DomStorageDatabase::Value>>,
GetHistogram("ReadMapKeyValues"), in_memory_)
.Then(std::move(callback)));
}
```
Helpers include:
RecordExpected:
```
template <typename T>
StatusOr<T> RecordExpected(std::string histogram_name,
bool in_memory,
StatusOr<T> result) {
if (result.has_value()) {
DbStatus::OK().Log(histogram_name, in_memory);
} else {
result.error().Log(histogram_name, in_memory);
}
return result;
}
```
GetHistogram:
```
std::string AsyncDomStorageDatabase::GetHistogram(
std::string_view operation) const {
return base::StrCat({StorageTypeForHistograms(), ".", operation});
}
```
base::BindOnce(
[](bool in_memory, std::string histogram_name, DbStatus status) {
status.Log(histogram_name, in_memory);
return status;
},
in_memory_, base::StrCat({StorageTypeForHistograms(), ".CloneMap"}))
.Then(std::move(callback)));Similarly, let's make some helpers to make this pattern less verbose:
```
void AsyncDomStorageDatabase::CloneMap(
DomStorageDatabase::MapLocator source_map,
DomStorageDatabase::MapLocator target_map,
StatusCallback callback) {
RunDatabaseTask(
base::BindOnce(
[](DomStorageDatabase::MapLocator source_map,
DomStorageDatabase::MapLocator target_map,
DomStorageDatabase& db) {
return db.CloneMap(std::move(source_map), std::move(target_map));
},
std::move(source_map), std::move(target_map)),
base::BindOnce(&RecordStatus, GetHistogram("CloneMap"), in_memory_)
.Then(std::move(callback)));
}
```
RecordStatus:
```
DbStatus RecordStatus(std::string histogram_name,
bool in_memory,
DbStatus status) {
status.Log(histogram_name, in_memory);
return status;
}
```
// TODO: Also implement this for the SQLite backend. histograms.ExpectBucketCount("Storage.LocalStorage.ReadMapKeyValues.OnDisk",Can we use ExpectUniqueSamples here?
// Verify that commit failures were recorded in the histogram.
// Sum > 0 means at least one non-zero (failure) sample was recorded.
EXPECT_GT(histograms.GetTotalSum("Storage.LocalStorage.UpdateMaps.OnDisk"),
0);Does this test produce different results for each run? If not, should we assert the expected results for both the number of success and the number of failures?
// Wait for database setup to complete before starting histogram recording.Could this comment explain why? What do we want to avoid recording?
const SessionStorageMetadata::NamespaceStorageKeyMap&
namespace_storage_key_map = session_storage_impl()
->GetMetadataForTesting()
.namespace_storage_key_map();
auto namespace_it = namespace_storage_key_map.find(namespace_id);
ASSERT_NE(namespace_it, namespace_storage_key_map.end());
auto storage_key_it = namespace_it->second.find(storage_key);
ASSERT_NE(storage_key_it, namespace_it->second.end());
const DomStorageDatabase::MapLocator& map_locator = *storage_key_it->second;
// Wait for the commit to complete by checking the database directly.
std::map<DomStorageDatabase::Key, DomStorageDatabase::Value> map_entries;
EXPECT_TRUE(base::test::RunUntil([&]() {
map_entries.clear();
ReadMapKeyValuesSync(*session_storage_impl()->GetDatabaseForTesting(),
map_locator.Clone(), &map_entries);
return map_entries.size() == 1u;
}));Can we use `test::GetAllSync(area.get());` instead of this code?
// Wait for async PutMetadata triggered by BindStorageArea to complete.
{
base::RunLoop loop;
session_storage_impl()
->GetDatabaseForTesting()
->database()
.PostTaskWithThisObject(base::BindLambdaForTesting(
[&](DomStorageDatabase*) { loop.Quit(); }));
loop.Run();
}Why is this wait needed? Should we move it to before the histogram checking below instead?
histograms.ExpectBucketCount("Storage.SessionStorage.ReadMapKeyValues.OnDisk",Can we use ExpectUniqueSample here?
EXPECT_TRUE(base::test::RunUntil([&]() {Instead of using RunUntil, should we wait on a task posted to the db task runner like the other tests you updated above?
The same feedback applies to the RunUntil() usage below.
return histograms.GetBucketCount("Storage.SessionStorage.UpdateMaps.OnDisk",Can we use ExpectUniqueSample?
// Wait for async CloneMap to complete.
{
base::RunLoop loop;
session_storage_impl()
->GetDatabaseForTesting()
->database()
.PostTaskWithThisObject(base::BindLambdaForTesting(
[&](DomStorageDatabase*) { loop.Quit(); }));
loop.Run();
}Maybe create a function for this duplicated code?
// Verify that commit failures were recorded in the histogram.
// Sum > 0 means at least one non-zero (failure) sample was recorded.
EXPECT_GT(histograms.GetTotalSum("Storage.SessionStorage.UpdateMaps.OnDisk"),
0);Similar to other comment about this, can we assert the specific number of successes and failures?
Related details
- Rahul 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. |
Rahul Singh (Gerrit)
Rahul Singh added 16 comments![]( "Open in Gerrit")
Thanks for taking a look and for the great suggestions! I've addressed almost all of them. Just have 2 open threads I'd like your thoughts on. Thank you!
new AsyncDomStorageDatabase(storage_type, database_path.empty()));nit: maybe label for the reader --
`/*in_memory=*/database_path.empty()`
Done!
base::BindOnce(
[](bool in_memory, std::string histogram_name,
StatusOr<std::map<DomStorageDatabase::Key,
DomStorageDatabase::Value>> result) {
if (result.has_value()) {
DbStatus::OK().Log(histogram_name, in_memory);
} else {
result.error().Log(histogram_name, in_memory);
}
return result;
},Done
base::BindOnce(
[](bool in_memory, std::string histogram_name, DbStatus status) {
status.Log(histogram_name, in_memory);
return status;
},
in_memory_, base::StrCat({StorageTypeForHistograms(), ".CloneMap"}))
.Then(std::move(callback)));Similarly, let's make some helpers to make this pattern less verbose:
```
void AsyncDomStorageDatabase::CloneMap(
DomStorageDatabase::MapLocator source_map,
DomStorageDatabase::MapLocator target_map,
StatusCallback callback) {
RunDatabaseTask(
base::BindOnce(
[](DomStorageDatabase::MapLocator source_map,
DomStorageDatabase::MapLocator target_map,
DomStorageDatabase& db) {
return db.CloneMap(std::move(source_map), std::move(target_map));
},
std::move(source_map), std::move(target_map)),
base::BindOnce(&RecordStatus, GetHistogram("CloneMap"), in_memory_)
.Then(std::move(callback)));
}
```RecordStatus:
```
DbStatus RecordStatus(std::string histogram_name,
bool in_memory,
DbStatus status) {
status.Log(histogram_name, in_memory);
return status;
}
```
Done
// TODO: Also implement this for the SQLite backend.Rahul Singh
Link added. Thanks!
histograms.ExpectBucketCount("Storage.LocalStorage.ReadMapKeyValues.OnDisk",Can we use ExpectUniqueSamples here?
Updated here and everywhere else it makes sense in this change. Thanks!
// Verify that commit failures were recorded in the histogram.
// Sum > 0 means at least one non-zero (failure) sample was recorded.
EXPECT_GT(histograms.GetTotalSum("Storage.LocalStorage.UpdateMaps.OnDisk"),
0);Does this test produce different results for each run? If not, should we assert the expected results for both the number of success and the number of failures?
I double checked and the exact number can vary under heavy load. There appears to be a race between our while loop repeatedly Put'ing and exactly how many callbacks that record the histogram run before we disconnect on exceeding the kCommitErrorThreshold. So I'll leave this as is if that's fine by you. The alternative, would be to rewrite parts of this test to avoid races.
// Wait for database setup to complete before starting histogram recording.Could this comment explain why? What do we want to avoid recording?
Done. Also moved the HistogramTester init in the Cloning testcase to the top of that test case. In its current form, it no longer needs to filter out noisy events from init.
const SessionStorageMetadata::NamespaceStorageKeyMap&
namespace_storage_key_map = session_storage_impl()
->GetMetadataForTesting()
.namespace_storage_key_map();
auto namespace_it = namespace_storage_key_map.find(namespace_id);
ASSERT_NE(namespace_it, namespace_storage_key_map.end());
auto storage_key_it = namespace_it->second.find(storage_key);
ASSERT_NE(storage_key_it, namespace_it->second.end());
const DomStorageDatabase::MapLocator& map_locator = *storage_key_it->second;
// Wait for the commit to complete by checking the database directly.
std::map<DomStorageDatabase::Key, DomStorageDatabase::Value> map_entries;
EXPECT_TRUE(base::test::RunUntil([&]() {
map_entries.clear();
ReadMapKeyValuesSync(*session_storage_impl()->GetDatabaseForTesting(),
map_locator.Clone(), &map_entries);
return map_entries.size() == 1u;
}));Can we use `test::GetAllSync(area.get());` instead of this code?
The original intent here was to ensure that the data gets written to disk which will fire the histogram. That is why I decided against using GetAllSync (which can read from the in memory cache) here. But taking a second look at this, we can simplify things by relying on a simple FlushAreaForTesting() + WaitForDatabaseTasks(). I've made this change. Thanks for the question!
// Wait for async PutMetadata triggered by BindStorageArea to complete.
{
base::RunLoop loop;
session_storage_impl()
->GetDatabaseForTesting()
->database()
.PostTaskWithThisObject(base::BindLambdaForTesting(
[&](DomStorageDatabase*) { loop.Quit(); }));
loop.Run();
}Why is this wait needed? Should we move it to before the histogram checking below instead?
We actually already have a wait at the bottom (before histogram assertions). So this one is no longer needed. I've removed it! Thanks for catching this!
histograms.ExpectBucketCount("Storage.SessionStorage.ReadMapKeyValues.OnDisk",Can we use ExpectUniqueSample here?
Done
EXPECT_TRUE(base::test::RunUntil([&]() {Instead of using RunUntil, should we wait on a task posted to the db task runner like the other tests you updated above?
The same feedback applies to the RunUntil() usage below.
It looks like the area.reset() kicks off async commits. So we can't ensure that the callback will run and the histogram fire by simply waiting on a task to the db thread.
In this specific test case, we can add a FlushAreaForTesting which will queue the commit and then we can just wait. I've made this update.
The other 2 test cases are explicitly testing the Delete APIs. So, doing a FlushAreaForTesting would deviate from that intent. So, in those cases I think we should keep the RunUntils. WDYT?
return histograms.GetBucketCount("Storage.SessionStorage.UpdateMaps.OnDisk",Rahul SinghCan we use ExpectUniqueSample?
This specific callsite can be updated now that I've removed the RunUntil here. The other 2 will keep the RunUntils unless we decide otherwise. So, those will continue to need GetBucketCount to help with the RunUntil polling.
// Wait for async CloneMap to complete.
{
base::RunLoop loop;
session_storage_impl()
->GetDatabaseForTesting()
->database()
.PostTaskWithThisObject(base::BindLambdaForTesting(
[&](DomStorageDatabase*) { loop.Quit(); }));
loop.Run();
}Maybe create a function for this duplicated code?
Done. Good idea!
// Wait for async CloneMap to complete.
{
base::RunLoop loop;
session_storage_impl()
->GetDatabaseForTesting()
->database()
.PostTaskWithThisObject(base::BindLambdaForTesting(
[&](DomStorageDatabase*) { loop.Quit(); }));
loop.Run();
}Maybe create a function for this duplicated code?
Done
// Verify that commit failures were recorded in the histogram.
// Sum > 0 means at least one non-zero (failure) sample was recorded.
EXPECT_GT(histograms.GetTotalSum("Storage.SessionStorage.UpdateMaps.OnDisk"),
0);Similar to other comment about this, can we assert the specific number of successes and failures?
See response on other comment. Resolving this one. We can continue the discussion there.
Related details
- Steve Becker
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. |
Steve Becker (Gerrit)
Steve Becker voted and added 6 comments![]( "Open in Gerrit")
Votes added by Steve Becker
| Code-Review | +1 |
6 comments
// This task is guaranteed to complete (TaskRunner with
// BLOCK_SHUTDOWN trait). So, we log histogram here vs. in the
// Reply.nit: This comment could be a little clearer. We're logging here instead of a future task that might not run because PruneOrigins() runs at shutdown. Hopefully, this histogram will validate that PurgeOriginsForShutdown() runs at shutdown.
std::string histogram_name = GetHistogram("OpenDatabase");nit: Should we use RecordExpected() here?
// Verify that commit failures were recorded in the histogram.
// Sum > 0 means at least one non-zero (failure) sample was recorded.
EXPECT_GT(histograms.GetTotalSum("Storage.LocalStorage.UpdateMaps.OnDisk"),
0);Rahul SinghDoes this test produce different results for each run? If not, should we assert the expected results for both the number of success and the number of failures?
I double checked and the exact number can vary under heavy load. There appears to be a race between our while loop repeatedly Put'ing and exactly how many callbacks that record the histogram run before we disconnect on exceeding the kCommitErrorThreshold. So I'll leave this as is if that's fine by you. The alternative, would be to rewrite parts of this test to avoid races.
Thanks for checking. I don't think we need to modify the test for this.
TEST_P(SessionStorageImplTest, Basic) {nit: Let's try to use a more descriptive test name. Maybe something related to Put?
area.reset();Should we call `test::GetAllSync(area.get());` to verify the Put()?
// because DeleteSessions posts its database task asynchronously, so our waitnit: Is this comment referring to DeleteStorage() above? If so, I'm not sure I follow why WaitForDatabaseTasks() does not work here. Assuming SessionStorageImpl has an open/connected database, DeleteStorage() should call DeleteStorageKeysFromSession() on the same sequence as WaitForDatabaseTasks(). The task from WaitForDatabaseTasks() should complete after the previously queued DeleteStorageKeysFromSession() task.
Related details
- Rahul 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. |
Steve Becker (Gerrit)
Steve Becker added 1 comment![]( "Open in Gerrit")
EXPECT_TRUE(base::test::RunUntil([&]() {Rahul SinghInstead of using RunUntil, should we wait on a task posted to the db task runner like the other tests you updated above?
The same feedback applies to the RunUntil() usage below.
It looks like the area.reset() kicks off async commits. So we can't ensure that the callback will run and the histogram fire by simply waiting on a task to the db thread.
In this specific test case, we can add a FlushAreaForTesting which will queue the commit and then we can just wait. I've made this update.
The other 2 test cases are explicitly testing the Delete APIs. So, doing a FlushAreaForTesting would deviate from that intent. So, in those cases I think we should keep the RunUntils. WDYT?
Done
Rahul Singh (Gerrit)
Rahul Singh added 6 comments![]( "Open in Gerrit")
Thanks for the review Steve! I've addressed your remaining comments. Adding leimy@ for an OWNERS review of the storage/histograms.xml and enums.xml files. Thanks!
// This task is guaranteed to complete (TaskRunner with
// BLOCK_SHUTDOWN trait). So, we log histogram here vs. in the
// Reply.nit: This comment could be a little clearer. We're logging here instead of a future task that might not run because PruneOrigins() runs at shutdown. Hopefully, this histogram will validate that PurgeOriginsForShutdown() runs at shutdown.
Done
std::string histogram_name = GetHistogram("OpenDatabase");nit: Should we use RecordExpected() here?
Done
nit: Let's try to use a more descriptive test name. Maybe something related to Put?
Done
Should we call `test::GetAllSync(area.get());` to verify the Put()?
Good idea. Added.
// because DeleteSessions posts its database task asynchronously, so our waitnit: Is this comment referring to DeleteStorage() above? If so, I'm not sure I follow why WaitForDatabaseTasks() does not work here. Assuming SessionStorageImpl has an open/connected database, DeleteStorage() should call DeleteStorageKeysFromSession() on the same sequence as WaitForDatabaseTasks(). The task from WaitForDatabaseTasks() should complete after the previously queued DeleteStorageKeysFromSession() task.
After doing ShutDownSessionStorage(), we were opening the database and calling DeleteStorage on it. This was hitting the RunWhenConnected path so the WaitForDatabaseTasks would get posted and run before this DeleteStorage.
I've added a new EnsureDatabaseOpen() helper that initializes and waits for the db to be open before we call DeleteStorage() on it. The DeleteStorage() will now run immediately which allows us to simply rely on the WaitForDatabaseTasks() helper before asserting histogram counts.
Related details
- Mingyu Lei
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingyu Lei (Gerrit)
Mingyu Lei voted Code-Review+1![]( "Open in Gerrit")
- Rahul Singh
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rahul Singh (Gerrit)
Rahul Singh voted Commit-Queue+2![]( "Open in Gerrit")
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
DomStorage: Add DbStatus histograms
Adds
- CloneMap
- DeleteSessions
- DeleteStorageKeysFromSession
- PurgeOrigins
- PutMetadata
- ReadAllMetadata
- ReadMapKeyValues
- UpdateMaps
variants to the
Storage.{DomStorageType}.{Operation}.{DomStorageBackingStoreLifetime}
patterned histogram. This enables LevelDB vs SQLite comparison for these
operations for the DomStorage SQLite rollout.
These histograms are logged in AsyncDomStorageDatabase, which gains
`storage_type_` and `in_memory_` members for histogram name
construction.
- M components/services/storage/dom_storage/async_dom_storage_database.cc
- M components/services/storage/dom_storage/async_dom_storage_database.h
- M components/services/storage/dom_storage/dom_storage_database.h
- M components/services/storage/dom_storage/local_storage_impl.cc
- M components/services/storage/dom_storage/local_storage_impl_unittest.cc
- M components/services/storage/dom_storage/session_storage_impl.cc
- M components/services/storage/dom_storage/session_storage_impl_unittest.cc
- M tools/metrics/histograms/metadata/storage/histograms.xml
Code-Review: +1 by Mingyu Lei, +1 by Steve Becker
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |