Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1992813004:
Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about… (Closed)
DescriptionQuota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage.
Quota.AgeOfOriginInDays
Quota.AgeOfDataInDays
Quota.QuotaForOrigin
BUG=605753
Committed: https://crrev.com/78bab80f86194d0b4983aa42b5e11ea3fca3d0f2
Cr-Commit-Position: refs/heads/master@{#394970}
Patch Set 1 #
Total comments: 16
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Messages
Total messages: 19 (8 generated)
michaeln@chromium.org changed reviewers: + isherman@chromium.org, jsbell@chromium.org
ptal
https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:334: UMA_HISTOGRAM_MBYTES("Quota.QuotaForOriginIncognito", quota); It's worth double-checking with the privacy team, but I don't think it's generally accepted to record metrics specifically about incognito usage. It kind of goes against the grain of the philosophy of incognito mode. https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:1595: UMA_HISTOGRAM_MBYTES("Quota.AmountOverYearOldData", usage_over_year_old); This is a fairly significant number of similar histograms. It might be okay, but are you sure that you need quite so much granularity? How do you plan to use this data? https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42926: + <owner>micaheln@chromium.org</owner> nit: typo in your username ;) https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43104: + accessed. Logged hourly. Hmm, this is logged hourly... for which origins? All known origins? Open origins? Some other set of origins? https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43112: + irregular intervals. "Logged at irregular intervals" is pretty mysterious. Is there some event that triggers the logging? Or is there a source of randomness involved somehow?
fixed my mispelled name (duh) and added "for all origins with stored data" to a description https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:334: UMA_HISTOGRAM_MBYTES("Quota.QuotaForOriginIncognito", quota); On 2016/05/19 04:06:01, Ilya Sherman wrote: > It's worth double-checking with the privacy team, but I don't think it's > generally accepted to record metrics specifically about incognito usage. It > kind of goes against the grain of the philosophy of incognito mode. hmmm... i see many metrics that are specific to 'incognito'. to my mind, history,cookies,cache, info attributable to specific sites are verboten. the fact that incognito was invoked to view a page is not verboten i could remove this for now because i'm interested in getting the main stream metrics in prior to the branch being cut and bring this one up with the privacy folks in a sperarte cl if need be... but it doesn't seem like it needs to be excluded? https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:1595: UMA_HISTOGRAM_MBYTES("Quota.AmountOverYearOldData", usage_over_year_old); On 2016/05/19 04:06:01, Ilya Sherman wrote: > This is a fairly significant number of similar histograms. It might be okay, > but are you sure that you need quite so much granularity? How do you plan to > use this data? Two reasons for the granularity. - to get a sense of kind of impact "evict everything <xxx> old" would have - to help evaluate if future changes we make to the quota system are hurting or helping, larger amounts of fresher data being preferable. https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:42926: + <owner>micaheln@chromium.org</owner> On 2016/05/19 04:06:01, Ilya Sherman wrote: > nit: typo in your username ;) ooops, done, thnx https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43104: + accessed. Logged hourly. On 2016/05/19 04:06:01, Ilya Sherman wrote: > Hmm, this is logged hourly... for which origins? All known origins? Open > origins? Some other set of origins? For all origins with stored data, updated the commented. https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43112: + irregular intervals. On 2016/05/19 04:06:01, Ilya Sherman wrote: > "Logged at irregular intervals" is pretty mysterious. Is there some event that > triggers the logging? Or is there a source of randomness involved somehow? In the process of writing new data, storage systems consult the quota system for "usage and quota" to determine if there is sufficient space available for the new data. This value is logged at the time of that consultation. The frequency is entirely dependent on how individual websites use the various storage apis. Translation.... "irregular intervals".
doh... wait... https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:1595: UMA_HISTOGRAM_MBYTES("Quota.AmountOverYearOldData", usage_over_year_old); On 2016/05/19 19:09:15, michaeln wrote: > On 2016/05/19 04:06:01, Ilya Sherman wrote: > > This is a fairly significant number of similar histograms. It might be okay, > > but are you sure that you need quite so much granularity? How do you plan to > > use this data? > > Two reasons for the granularity. > > - to get a sense of kind of impact "evict everything <xxx> old" would have > > - to help evaluate if future changes we make to the quota system are hurting or > helping, larger amounts of fresher data being preferable. Oh... I'll redo these to use a single histogram with a bucket for each day and add a sample for each KB of data that is <n> days old. I don't think there's a macro to add multiple samples to a bucket, but i should be able to call AddCount() directly. The OriginAgeInDays hsito counts number of origins. This one will will count "AgeOfMBInDays".
https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:334: UMA_HISTOGRAM_MBYTES("Quota.QuotaForOriginIncognito", quota); On 2016/05/19 19:09:15, michaeln wrote: > On 2016/05/19 04:06:01, Ilya Sherman wrote: > > It's worth double-checking with the privacy team, but I don't think it's > > generally accepted to record metrics specifically about incognito usage. It > > kind of goes against the grain of the philosophy of incognito mode. > > hmmm... i see many metrics that are specific to 'incognito'. to my mind, > history,cookies,cache, info attributable to specific sites are verboten. the > fact that incognito was invoked to view a page is not verboten > > i could remove this for now because i'm interested in getting the main stream > metrics in prior to the branch being cut and bring this one up with the privacy > folks in a sperarte cl if need be... but it doesn't seem like it needs to be > excluded? I'd recommend removing this for now, and checking with the privacy team. We often take a fairly conservative approach with Incognito mode, because subverting some users' expectations is worse than providing some users more privacy than they expected. But, it's something of an art to find the precise balance, and I'll defer to the privacy team on the current best practices. https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:1595: UMA_HISTOGRAM_MBYTES("Quota.AmountOverYearOldData", usage_over_year_old); On 2016/05/19 19:51:24, michaeln wrote: > On 2016/05/19 19:09:15, michaeln wrote: > > On 2016/05/19 04:06:01, Ilya Sherman wrote: > > > This is a fairly significant number of similar histograms. It might be > okay, > > > but are you sure that you need quite so much granularity? How do you plan > to > > > use this data? > > > > Two reasons for the granularity. > > > > - to get a sense of kind of impact "evict everything <xxx> old" would have > > > > - to help evaluate if future changes we make to the quota system are hurting > or > > helping, larger amounts of fresher data being preferable. > > Oh... I'll redo these to use a single histogram with a bucket for each day and > add a sample for each KB of data that is <n> days old. > > I don't think there's a macro to add multiple samples to a bucket, but i should > be able to call AddCount() directly. > > The OriginAgeInDays hsito counts number of origins. This one will will count > "AgeOfMBInDays". Yep, that's a reasonable approach, as long as you're not needing to see the distribution of memory usage within each bucket. Just be aware that outliers can significantly influence the data. https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1992813004/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:43112: + irregular intervals. On 2016/05/19 19:09:15, michaeln wrote: > On 2016/05/19 04:06:01, Ilya Sherman wrote: > > "Logged at irregular intervals" is pretty mysterious. Is there some event > that > > triggers the logging? Or is there a source of randomness involved somehow? > > In the process of writing new data, storage systems consult the quota system for > "usage and quota" to determine if there is sufficient space available for the > new data. This value is logged at the time of that consultation. The frequency > is entirely dependent on how individual websites use the various storage apis. > > Translation.... "irregular intervals". I think it's worth going into this detail in the histogram description. Even if the upshot us nothing more than "irregular intervals", it helps the reader gain confidence that they understand what's behind those irregular intervals, and therefore adjust their analysis accordingly.
https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:334: UMA_HISTOGRAM_MBYTES("Quota.QuotaForOriginIncognito", quota); On 2016/05/19 22:31:16, Ilya Sherman wrote: > On 2016/05/19 19:09:15, michaeln wrote: > > On 2016/05/19 04:06:01, Ilya Sherman wrote: > > > It's worth double-checking with the privacy team, but I don't think it's > > > generally accepted to record metrics specifically about incognito usage. It > > > kind of goes against the grain of the philosophy of incognito mode. > > > > hmmm... i see many metrics that are specific to 'incognito'. to my mind, > > history,cookies,cache, info attributable to specific sites are verboten. the > > fact that incognito was invoked to view a page is not verboten > > > > i could remove this for now because i'm interested in getting the main stream > > metrics in prior to the branch being cut and bring this one up with the > privacy > > folks in a sperarte cl if need be... but it doesn't seem like it needs to be > > excluded? > > I'd recommend removing this for now, and checking with the privacy team. We > often take a fairly conservative approach with Incognito mode, because > subverting some users' expectations is worse than providing some users more > privacy than they expected. But, it's something of an art to find the precise > balance, and I'll defer to the privacy team on the current best practices. Done. https://codereview.chromium.org/1992813004/diff/1/storage/browser/quota/quota... storage/browser/quota/quota_manager.cc:1595: UMA_HISTOGRAM_MBYTES("Quota.AmountOverYearOldData", usage_over_year_old); On 2016/05/19 22:31:16, Ilya Sherman wrote: > On 2016/05/19 19:51:24, michaeln wrote: > > On 2016/05/19 19:09:15, michaeln wrote: > > > On 2016/05/19 04:06:01, Ilya Sherman wrote: > > > > This is a fairly significant number of similar histograms. It might be > > okay, > > > > but are you sure that you need quite so much granularity? How do you plan > > to > > > > use this data? > > > > > > Two reasons for the granularity. > > > > > > - to get a sense of kind of impact "evict everything <xxx> old" would have > > > > > > - to help evaluate if future changes we make to the quota system are hurting > > or > > > helping, larger amounts of fresher data being preferable. > > > > Oh... I'll redo these to use a single histogram with a bucket for each day and > > add a sample for each KB of data that is <n> days old. > > > > I don't think there's a macro to add multiple samples to a bucket, but i > should > > be able to call AddCount() directly. > > > > The OriginAgeInDays hsito counts number of origins. This one will will count > > "AgeOfMBInDays". > > Yep, that's a reasonable approach, as long as you're not needing to see the > distribution of memory usage within each bucket. Just be aware that outliers > can significantly influence the data. Done
Description was changed from ========== Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage. BUG=605753 ========== to ========== Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage. Quota.AgeOfOriginInDays Quota.AgeOfDataInDays Quota.QuotaForOrigin BUG=605753 ==========
Description was changed from ========== Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage. Quota.AgeOfOriginInDays Quota.AgeOfDataInDays Quota.QuotaForOrigin BUG=605753 ========== to ========== Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage. Quota.AgeOfOriginInDays Quota.AgeOfDataInDays Quota.QuotaForOrigin BUG=605753 ==========
LGTM, thanks.
michaeln@chromium.org changed reviewers: + japhet@chromium.org
lgtm
The CQ bit was checked by michaeln@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1992813004/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1992813004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1992813004/100001
Message was sent while issue was closed.
Description was changed from ========== Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage. Quota.AgeOfOriginInDays Quota.AgeOfDataInDays Quota.QuotaForOrigin BUG=605753 ========== to ========== Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage. Quota.AgeOfOriginInDays Quota.AgeOfDataInDays Quota.QuotaForOrigin BUG=605753 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage. Quota.AgeOfOriginInDays Quota.AgeOfDataInDays Quota.QuotaForOrigin BUG=605753 ========== to ========== Quota: Add metrics about the age and amount of data stored in temporary storage, and a metric about the computed quota for an origin using temporary storage. Quota.AgeOfOriginInDays Quota.AgeOfDataInDays Quota.QuotaForOrigin BUG=605753 Committed: https://crrev.com/78bab80f86194d0b4983aa42b5e11ea3fca3d0f2 Cr-Commit-Position: refs/heads/master@{#394970} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/78bab80f86194d0b4983aa42b5e11ea3fca3d0f2 Cr-Commit-Position: refs/heads/master@{#394970} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
