Add Quota.TimeSinceOriginEvicted UMA histogram.

Add Quota.TimeSinceOriginEvicted UMA histogram.

This CL adds a UMA histogram that tracks the amount of time since an origin
last had its temporary storage evicted. The last eviction time data is stored
by adding a table to the QuotaManager database which tracks the last eviction
time for an origin.

BUG=542075
Committed: https://crrev.com/9c79f8034ba1029a359be35be1ef0f95bcf72647
Cr-Commit-Position: refs/heads/master@{#354932}

[calamity, 2015-10-12 03:45:16]

calamity@chromium.org changed reviewers:
+ dominickn@chromium.org, isherman@chromium.org, michaeln@chromium.org

[calamity, 2015-10-12 03:45:17]

+dominickn for local review

+michaeln for *storage* OWNERS

[dominickn, 2015-10-12 04:10:24]

https://codereview.chromium.org/1403673002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1403673002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:36399: +  <summary>Time between an
origin's storage evictions.</summary>
Nit: "... origin's temporary storage evictions, recorded each time the origin is
evicted (except for the first eviction)." or the like.

[calamity, 2015-10-13 04:04:43]

https://codereview.chromium.org/1403673002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1403673002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:36399: +  <summary>Time between an
origin's storage evictions.</summary>
On 2015/10/12 04:10:24, dominickn wrote:
> Nit: "... origin's temporary storage evictions, recorded each time the origin
is
> evicted (except for the first eviction)." or the like.

Done.

[Ilya Sherman, 2015-10-13 18:56:18]

LGTM % nits:

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:270: static const char
kTimeSinceOriginEvictedHistogram[];
nit: The other histogram names are inlined, so it probably makes sense to inline
this one too.

https://codereview.chromium.org/1403673002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1403673002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36397: +<histogram
name="Quota.TimeSinceOriginEvicted">
nit: Perhaps "TimeBetweenOriginEvictions"?  That makes it clearer what is the
trigger for this metric being recorded.

[michaeln, 2015-10-13 21:35:36]

https://codereview.chromium.org/1403673002/diff/20001/content/browser/quota/q...
File content/browser/quota/quota_database_unittest.cc (right):

https://codereview.chromium.org/1403673002/diff/20001/content/browser/quota/q...
content/browser/quota/quota_database_unittest.cc:308: EXPECT_EQ(base::Time(),
last_eviction_time);
thnx for the tests!

also test DeleteOriginLastEvictionTime() for an origin that is not present and
make sure it returns true

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:101: false },
do you want an index on the origin column?

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:702: }
do you want an index on the origin column?

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:704: meta_table_->SetVersionNumber(5);
i think you want to to Commit() here

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:170: if
(!database->GetOriginLastEvictionTime(origin, type, &last_eviction_time))
it's important to return false only for error conditions, please ensure the new
Delete and Get methods don't return false when the row is not found

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1477: void
QuotaManager::EvictOriginData(const GURL& origin,
The public DeleteOriginData method and this method can call a common helper
(DeleteOriginDataInternal or something like that) that makes the distinction.

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:207: bool is_eviction,
let's not change the public method sig to add this param

[calamity, 2015-10-15 07:56:06]

Patchset #3 (id:40001) has been deleted

[calamity, 2015-10-15 07:56:43]

https://codereview.chromium.org/1403673002/diff/20001/content/browser/quota/q...
File content/browser/quota/quota_database_unittest.cc (right):

https://codereview.chromium.org/1403673002/diff/20001/content/browser/quota/q...
content/browser/quota/quota_database_unittest.cc:308: EXPECT_EQ(base::Time(),
last_eviction_time);
On 2015/10/13 21:35:36, michaeln wrote:
> thnx for the tests!
> 
> also test DeleteOriginLastEvictionTime() for an origin that is not present and
> make sure it returns true

Done.

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:101: false },
On 2015/10/13 21:35:36, michaeln wrote:
> do you want an index on the origin column?

Hmm. I don't think it will be necessary. We're not really doing queries that
would benefit from an origin-only index. sqlite already generates an auto index
from the unique constraint for (origin, type) which should satisfy our query
needs.

In fact, I'm not sure some of these other indexes here are good ideas either.
The access/modified time indexes are going to incur overhead as they are
constantly written to while the benefit of avoiding a sort at eviction time is
rare. A full scan per eviction is probably more performant than constantly
updating the indexes on every storage access. The other 2 indexes also probably
don't need to be there since the auto index for the tables satisfies their query
needs.

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:702: }
On 2015/10/13 21:35:36, michaeln wrote:
> do you want an index on the origin column?

See above.

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:704: meta_table_->SetVersionNumber(5);
On 2015/10/13 21:35:36, michaeln wrote:
> i think you want to to Commit() here

Done.

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:170: if
(!database->GetOriginLastEvictionTime(origin, type, &last_eviction_time))
On 2015/10/13 21:35:36, michaeln wrote:
> it's important to return false only for error conditions, please ensure the
new
> Delete and Get methods don't return false when the row is not found

That's already the behavior. This only returns false if the statement doesn't
succeed. The test already has the case of returning true when no row is found.

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1477: void
QuotaManager::EvictOriginData(const GURL& origin,
On 2015/10/13 21:35:36, michaeln wrote:
> The public DeleteOriginData method and this method can call a common helper
> (DeleteOriginDataInternal or something like that) that makes the distinction.

Done.

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:207: bool is_eviction,
On 2015/10/13 21:35:36, michaeln wrote:
> let's not change the public method sig to add this param

Done.

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:270: static const char
kTimeSinceOriginEvictedHistogram[];
On 2015/10/13 18:56:18, Ilya Sherman wrote:
> nit: The other histogram names are inlined, so it probably makes sense to
inline
> this one too.

This is used in tests.

https://codereview.chromium.org/1403673002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1403673002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36397: +<histogram
name="Quota.TimeSinceOriginEvicted">
On 2015/10/13 18:56:18, Ilya Sherman wrote:
> nit: Perhaps "TimeBetweenOriginEvictions"?  That makes it clearer what is the
> trigger for this metric being recorded.

Done.

[calamity, 2015-10-15 11:11:06]

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1403673002/diff/20001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:704: meta_table_->SetVersionNumber(5);
On 2015/10/15 07:56:43, calamity wrote:
> On 2015/10/13 21:35:36, michaeln wrote:
> > i think you want to to Commit() here
> 
> Done.

Oops, missed this one. Will do it tomorrow.

[michaeln, 2015-10-15 21:25:23]

+cmumford (another chris) to take a look a the uma stats too

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:82: " UNIQUE(origin, type))"}};
oh! thnx for pointing out the unique constraints create an index. i think your
right that we could drop some of the other indexes, but that's for a different
cl.

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:691: upgraded = true;
super tiny nit: prefer an early return true here

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:705: upgraded = true;
and here

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:170: if
(!database->GetOriginLastEvictionTime(origin, type, &last_eviction_time))
In looking at this closely now, I see there are two other values stored in the
db that might be interesting when it comes to evaluating how good/bad the victim
picking logic is? I think the 'age' of the victim is very interesting. I'm not
sure all of these are good stats to have, but am wondering?

These values are stored in the OriginInfo table:
- last_access_time
- used_count

Interesting uma stats maybe:
- AgeOfEvictedOrigin = (now - last_access_time)
- UsageCountOfEvictedOrigin = used_count

Also, a stat for the delta between the victim's age and the oldest in the db's
age might be interesting, I think that could tell us how often karma picks a
different victim than the current lru algo would have.

- RelativeAgeOfEvictedOrigin = (last_access_time - oldest_last_access_time)

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:175:
UMA_HISTOGRAM_LONG_TIMES(QuotaManager::kTimeBetweenOriginEvictionsHistogram,
To compute a failure rate (where failure is we repeatly evicted something too
quickly) do we also need to keep track of total eviction events?

There's no uma sample for a first time eviction of an origin so thats not
directly computable. Do you want to have add a sample for that case too, or
maybe a separate bool stat that keeps track of the first time vs repeat
evictions?

Idk the answer, just asking the question.

https://codereview.chromium.org/1403673002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1403673002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36392: +<histogram
name="Quota.TimeBetweenOriginEvictions">
maybe put the term repeat or recurring in this name?
TimeBetweenRecurringOriginEviction

[michaeln, 2015-10-15 21:28:10]

michaeln@chromium.org changed reviewers:
+ cmumford@chromium.org

[calamity, 2015-10-16 00:53:04]

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
File storage/browser/quota/quota_database.cc (right):

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:691: upgraded = true;
On 2015/10/15 21:25:23, michaeln wrote:
> super tiny nit: prefer an early return true here

Done.

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_database.cc:705: upgraded = true;
On 2015/10/15 21:25:23, michaeln wrote:
> and here

Done.

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:170: if
(!database->GetOriginLastEvictionTime(origin, type, &last_eviction_time))
On 2015/10/15 21:25:23, michaeln wrote:
> In looking at this closely now, I see there are two other values stored in the
> db that might be interesting when it comes to evaluating how good/bad the
victim
> picking logic is? I think the 'age' of the victim is very interesting. I'm not
> sure all of these are good stats to have, but am wondering?
> 
> These values are stored in the OriginInfo table:
> - last_access_time
> - used_count
> 
> Interesting uma stats maybe:
> - AgeOfEvictedOrigin = (now - last_access_time)
> - UsageCountOfEvictedOrigin = used_count
> 
> Also, a stat for the delta between the victim's age and the oldest in the db's
> age might be interesting, I think that could tell us how often karma picks a
> different victim than the current lru algo would have.
> 
> - RelativeAgeOfEvictedOrigin = (last_access_time - oldest_last_access_time)

Yeah, I agree. I think these are all great things to log. I'll make a follow-up
CL. I'd like to split out this change that involves actual table creation and
stuff from just adding a few extra queries.

https://codereview.chromium.org/1403673002/diff/60001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:175:
UMA_HISTOGRAM_LONG_TIMES(QuotaManager::kTimeBetweenOriginEvictionsHistogram,
On 2015/10/15 21:25:23, michaeln wrote:
> To compute a failure rate (where failure is we repeatly evicted something too
> quickly) do we also need to keep track of total eviction events?
> 
> There's no uma sample for a first time eviction of an origin so thats not
> directly computable. Do you want to have add a sample for that case too, or
> maybe a separate bool stat that keeps track of the first time vs repeat
> evictions?
> 
> Idk the answer, just asking the question.

Good point.

I think we have the total eviction count already logged as the sum of
Quota.NumberOfEvictedOriginsPerRound.

SUM(Quota.NumberOfEvictedOriginsPerRound) -
COUNT(Quota.TimeBetweenRepeatedOriginEvictions) should be what we're looking for
here.

https://codereview.chromium.org/1403673002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1403673002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:36392: +<histogram
name="Quota.TimeBetweenOriginEvictions">
On 2015/10/15 21:25:23, michaeln wrote:
> maybe put the term repeat or recurring in this name?
> TimeBetweenRecurringOriginEviction

Done.

[michaeln, 2015-10-16 20:50:50]

lgtm % couple small comments

https://codereview.chromium.org/1403673002/diff/80001/content/browser/quota/q...
File content/browser/quota/quota_database_unittest.cc (right):

https://codereview.chromium.org/1403673002/diff/80001/content/browser/quota/q...
content/browser/quota/quota_database_unittest.cc:59: void
UpgradeSchemaV2toV3(const base::FilePath& kDbFile) {
Can you alter this to UpgradeSchemaV2toV5()?

The sql::Connection class has a pair of methods that help to verify the schema.
Add a test for your new table existing.

  bool DoesTableExist(const char* table_name) const;
  bool DoesIndexExist(const char* index_name) const;

https://codereview.chromium.org/1403673002/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1403673002/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:183: return true;
could simplify this to
return database->SetBlahBlah();

[calamity, 2015-10-20 00:18:30]

https://codereview.chromium.org/1403673002/diff/80001/content/browser/quota/q...
File content/browser/quota/quota_database_unittest.cc (right):

https://codereview.chromium.org/1403673002/diff/80001/content/browser/quota/q...
content/browser/quota/quota_database_unittest.cc:59: void
UpgradeSchemaV2toV3(const base::FilePath& kDbFile) {
On 2015/10/16 20:50:50, michaeln wrote:
> Can you alter this to UpgradeSchemaV2toV5()?
> 
> The sql::Connection class has a pair of methods that help to verify the
schema.
> Add a test for your new table existing.
> 
>   bool DoesTableExist(const char* table_name) const;
>   bool DoesIndexExist(const char* index_name) const;

Done.

https://codereview.chromium.org/1403673002/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1403673002/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:183: return true;
On 2015/10/16 20:50:50, michaeln wrote:
> could simplify this to
> return database->SetBlahBlah();

Done.

[calamity, 2015-10-20 00:18:35]

The CQ bit was checked by calamity@chromium.org

[calamity, 2015-10-20 00:18:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1403673002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-20 00:19:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403673002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403673002/120001

[commit-bot: I haz the power, 2015-10-20 00:24:52]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-10-20 00:25:44]

Patchset 6 (id:??) landed as
https://crrev.com/9c79f8034ba1029a359be35be1ef0f95bcf72647
Cr-Commit-Position: refs/heads/master@{#354932}