Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1582043002:
Add histograms for data usage (Closed)
DescriptionAdd histograms for data usage
The following UMA are added:
DataUsage.MatchingRulesCount.Valid
DataUsage.MatchingRulesCount.Invalid
- The number of valid and invalid matching rules fetched.
DataUsage.Perf.URLRegexMatchDuration
- Time taken for a regular expression to process an URL.
DataUsage.Perf.ReportSubmissionDuration
- Time taken for submitting the data use reports.
DataUsage.Perf.MatchingRuleFetchDuration
- Time taken for fetching the matching rules.
BUG=574880
Committed: https://crrev.com/b582452daa579977cb9200d398d53b4de8cc4ae5
Cr-Commit-Position: refs/heads/master@{#369900}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed comments #
Total comments: 12
Patch Set 3 : Addressed comments #
Total comments: 14
Patch Set 4 : Addressed nits #
Total comments: 2
Patch Set 5 : Addressed holte comments #
Messages
Total messages: 23 (9 generated)
rajendrant@chromium.org changed reviewers: + tbansal@chromium.org
ptal, thanks
https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_matcher.cc (right): https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_matcher.cc:85: UMA_HISTOGRAM_COUNTS_100("DataUsage.MatchingRulesCount", Can you change this to a enumerated histogram with 2 buckets: (1) Valid (2) Invalid Invalid would be incremented if pattern->ok() returned false above. This would be more useful because if invalid patterns are non-zero, then that's a bad sign (interface with external observer is broken). At the same time, count of valid patterns would tell us that we are on the right track. wdyt? https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_matcher.cc:109: base::TimeTicks begin = base::TimeTicks::Now(); This might be more useful if it records the duration against all regexes (that's what we want to minimize). So, may be move after the for loop. https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/external_data_use_observer_bridge.cc (right): https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:124: FROM_HERE, base::Bind(&ExternalDataUseObserver::OnFetchMatchingRulesDone, What are we trying to measure here? The way it is instrumented now, it will also take into account time taken for 2 thread hops, which is not useful. May be the purpose is to measure the time it takes since the start of Chrome to until the matching rules are first fetched (because all bytes during that time would be lost -- and we want to know that)? I guess after the first fetch, it is not important how much time the subsequent fetches take (e.g., even if they take 1 minute, we are still okay because rules are valid for much longer duration).
https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_matcher.cc (right): https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_matcher.cc:85: UMA_HISTOGRAM_COUNTS_100("DataUsage.MatchingRulesCount", On 2016/01/13 18:06:52, tbansal1 wrote: > Can you change this to a enumerated histogram with 2 buckets: > (1) Valid (2) Invalid > Invalid would be incremented if pattern->ok() returned false above. This would > be more useful because if invalid patterns are non-zero, then that's a bad sign > (interface with external observer is broken). At the same time, count of valid > patterns would tell us that we are on the right track. wdyt? Sorry, better to create 2 histograms: DataUsage.MatchingRulesCount.Valid and DataUsage.MatchingRulesCount.Invalid instead of enumerated histogram.
Description was changed from ========== Add histograms for data usage The following UMA are added: DataUsage.MatchingRulesCount - The number of matching rules fetched. DataUsage.Perf.URLRegexMatchDuration - Time taken for a regular expression to process an URL. DataUsage.Perf.ReportSubmissionDuration - Time taken for submitting the data use reports. DataUsage.Perf.MatchingRuleFetchDuration - Time taken for fetching the matching rules. BUG=574880 ========== to ========== Add histograms for data usage The following UMA are added: DataUsage.MatchingRulesCount.Valid DataUsage.MatchingRulesCount.Invalid - The number of valid and invalid matching rules fetched. DataUsage.Perf.URLRegexMatchDuration - Time taken for a regular expression to process an URL. DataUsage.Perf.ReportSubmissionDuration - Time taken for submitting the data use reports. DataUsage.Perf.MatchingRuleFetchDuration - Time taken for fetching the matching rules. BUG=574880 ==========
ptal, thanks https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_matcher.cc (right): https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_matcher.cc:85: UMA_HISTOGRAM_COUNTS_100("DataUsage.MatchingRulesCount", On 2016/01/13 18:44:24, tbansal1 wrote: > On 2016/01/13 18:06:52, tbansal1 wrote: > > Can you change this to a enumerated histogram with 2 buckets: > > (1) Valid (2) Invalid > > Invalid would be incremented if pattern->ok() returned false above. This would > > be more useful because if invalid patterns are non-zero, then that's a bad > sign > > (interface with external observer is broken). At the same time, count of valid > > patterns would tell us that we are on the right track. wdyt? > > Sorry, better to create 2 histograms: > DataUsage.MatchingRulesCount.Valid and > DataUsage.MatchingRulesCount.Invalid > instead of enumerated histogram. Done. https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_matcher.cc:109: base::TimeTicks begin = base::TimeTicks::Now(); On 2016/01/13 18:06:52, tbansal1 wrote: > This might be more useful if it records the duration against all regexes (that's > what we want to minimize). So, may be move after the for loop. If there are multiple regexes, the total duration is probably going to be in its multiple, and we won't be able to compare the performance. If we report for each regex, we would find distinct difference for bad regexes and try to finetune them. WDYT ? https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/external_data_use_observer.cc:208: UMA_HISTOGRAM_TIMES( Here again, the duration includes 2 thread hops. Should we need to only log pure report submission timing ? Or thread hop is negligible ? https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/external_data_use_observer_bridge.cc (right): https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:124: FROM_HERE, base::Bind(&ExternalDataUseObserver::OnFetchMatchingRulesDone, On 2016/01/13 18:06:52, tbansal1 wrote: > What are we trying to measure here? The way it is instrumented now, it will also > take into account time taken for 2 thread hops, which is not useful. > > May be the purpose is to measure the time it takes since the start of Chrome to > until the matching rules are first fetched (because all bytes during that time > would be lost -- and we want to know that)? > > I guess after the first fetch, it is not important how much time the subsequent > fetches take (e.g., even if they take 1 minute, we are still okay because rules > are valid for much longer duration). Agreed. I guess duration for first fetch is more valuable, than for subsequent fetches. But I thought the thread hops are negligible, and adding up those times don't matter?
please add tests. https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1582043002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/external_data_use_observer.cc:208: UMA_HISTOGRAM_TIMES( On 2016/01/13 23:33:51, Raj wrote: > Here again, the duration includes 2 thread hops. > Should we need to only log pure report submission timing ? > Or thread hop is negligible ? Delay due to thread hop can sometimes be large. I think here it is okay to include thread hop because EDUO can submit next report only when the previous report callback has come back. So, we are measuring the right metric. In the other place, you were adding an extra unnecessary thread hop just for measuring purpose, so that was not measuring the right metric. https://codereview.chromium.org/1582043002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer_bridge.cc (right): https://codereview.chromium.org/1582043002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:87: if (matching_rules_first_fetch_time_.is_null()) make it a const variable, and set it in the construtor? Then, we will know how much time it takes from ~start of chrome to first fetch? https://codereview.chromium.org/1582043002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:126: base::TimeTicks::Now() - matching_rules_first_fetch_time_); How do we know this is the first fetch? Seems like this will get recorded every 15 minutes. https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6573: + The number of invalid matching rules fetched from the platform external data Please comment on when this UMA is recorded. Something like: "A sample is recorded everytime..." Same for other UMA. https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6587: +<histogram name="DataUsage.Perf.MatchingRuleFirstFetchDuration" units="ms"> Say milliseconds to avoid ambiguity with microseconds. Here, and below. https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6592: + platform external data use observer. This will be the duration from the time s/will be the/is/ OR measures? Similarly for other UMA in this file. https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6603: + to the time report submitted asynchronous callback was received. Is this recorded only when callback indicates success or it the callback result does not matter?
rajendrant@chromium.org changed reviewers: + asvitkine@chromium.org
ptal, thanks https://codereview.chromium.org/1582043002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer_bridge.cc (right): https://codereview.chromium.org/1582043002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:87: if (matching_rules_first_fetch_time_.is_null()) On 2016/01/14 06:59:55, tbansal1 wrote: > make it a const variable, and set it in the construtor? Then, we will know how > much time it takes from ~start of chrome to first fetch? Done. https://codereview.chromium.org/1582043002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer_bridge.cc:126: base::TimeTicks::Now() - matching_rules_first_fetch_time_); On 2016/01/14 06:59:55, tbansal1 wrote: > How do we know this is the first fetch? Seems like this will get recorded every > 15 minutes. Fixed https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6573: + The number of invalid matching rules fetched from the platform external data On 2016/01/14 06:59:55, tbansal1 wrote: > Please comment on when this UMA is recorded. Something like: "A sample is > recorded everytime..." > > Same for other UMA. Done. https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6587: +<histogram name="DataUsage.Perf.MatchingRuleFirstFetchDuration" units="ms"> On 2016/01/14 06:59:55, tbansal1 wrote: > Say milliseconds to avoid ambiguity with microseconds. Here, and below. Done. https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6592: + platform external data use observer. This will be the duration from the time On 2016/01/14 06:59:55, tbansal1 wrote: > s/will be the/is/ OR > measures? > > Similarly for other UMA in this file. Done. https://codereview.chromium.org/1582043002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6603: + to the time report submitted asynchronous callback was received. On 2016/01/14 06:59:55, tbansal1 wrote: > Is this recorded only when callback indicates success or it the callback result > does not matter? Callback result does not matter.
lgtm % nits. https://codereview.chromium.org/1582043002/diff/40001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_matcher_unittest.cc (right): https://codereview.chromium.org/1582043002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_matcher_unittest.cc:105: int expect_valid_rules; s/expect_valid_rules/expect_count_valid_rules/ https://codereview.chromium.org/1582043002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_matcher_unittest.cc:106: int expect_url_matches; s/expect_url_matches/expect_count_url_match_duration_samples/ https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6574: + use observer. A sample is recorded everytime fetch done callback was called. nit: Either s/is/was/ OR s/was/is/ It seems s/was/is/ is better. Here, and below. https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6594: + start time of Chrome to the time the rules are returned asynchronously. A s/Chrome/Chromium/ https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6595: + sample is recorded once when fetch done callback was called. s/once when fetch/when the first fetch/ https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6604: + use observer. This measures the duration from the time reports are submitted s/reports are submitted/report submission/ https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6614: + The time taken for a regular expression to process an URL. A sample is s/process/parse/ s/processing/parsing/
rajendrant@chromium.org changed reviewers: + holte@chromium.org
holte: ptal histograms.xml Thanks https://codereview.chromium.org/1582043002/diff/40001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_matcher_unittest.cc (right): https://codereview.chromium.org/1582043002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_matcher_unittest.cc:105: int expect_valid_rules; On 2016/01/15 17:11:19, tbansal1 wrote: > s/expect_valid_rules/expect_count_valid_rules/ Done. https://codereview.chromium.org/1582043002/diff/40001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_matcher_unittest.cc:106: int expect_url_matches; On 2016/01/15 17:11:18, tbansal1 wrote: > s/expect_url_matches/expect_count_url_match_duration_samples/ Done. https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6574: + use observer. A sample is recorded everytime fetch done callback was called. On 2016/01/15 17:11:19, tbansal1 wrote: > nit: Either s/is/was/ OR s/was/is/ > It seems s/was/is/ is better. > > Here, and below. Done. https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6594: + start time of Chrome to the time the rules are returned asynchronously. A On 2016/01/15 17:11:19, tbansal1 wrote: > s/Chrome/Chromium/ Done. https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6595: + sample is recorded once when fetch done callback was called. On 2016/01/15 17:11:19, tbansal1 wrote: > s/once when fetch/when the first fetch/ Done. https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6604: + use observer. This measures the duration from the time reports are submitted On 2016/01/15 17:11:19, tbansal1 wrote: > s/reports are submitted/report submission/ Done. https://codereview.chromium.org/1582043002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6614: + The time taken for a regular expression to process an URL. A sample is On 2016/01/15 17:11:19, tbansal1 wrote: > s/process/parse/ > s/processing/parsing/ Done.
Please update units, otherwise lgtm. https://codereview.chromium.org/1582043002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1582043002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6588: + units="millsecond"> units="ms"
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by rajendrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/1582043002/#ps100001 (title: "Addressed holte comments")
Addressed nits https://codereview.chromium.org/1582043002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1582043002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6588: + units="millsecond"> On 2016/01/15 23:17:28, Steven Holte wrote: > units="ms" Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1582043002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1582043002/100001
Message was sent while issue was closed.
Description was changed from ========== Add histograms for data usage The following UMA are added: DataUsage.MatchingRulesCount.Valid DataUsage.MatchingRulesCount.Invalid - The number of valid and invalid matching rules fetched. DataUsage.Perf.URLRegexMatchDuration - Time taken for a regular expression to process an URL. DataUsage.Perf.ReportSubmissionDuration - Time taken for submitting the data use reports. DataUsage.Perf.MatchingRuleFetchDuration - Time taken for fetching the matching rules. BUG=574880 ========== to ========== Add histograms for data usage The following UMA are added: DataUsage.MatchingRulesCount.Valid DataUsage.MatchingRulesCount.Invalid - The number of valid and invalid matching rules fetched. DataUsage.Perf.URLRegexMatchDuration - Time taken for a regular expression to process an URL. DataUsage.Perf.ReportSubmissionDuration - Time taken for submitting the data use reports. DataUsage.Perf.MatchingRuleFetchDuration - Time taken for fetching the matching rules. BUG=574880 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add histograms for data usage The following UMA are added: DataUsage.MatchingRulesCount.Valid DataUsage.MatchingRulesCount.Invalid - The number of valid and invalid matching rules fetched. DataUsage.Perf.URLRegexMatchDuration - Time taken for a regular expression to process an URL. DataUsage.Perf.ReportSubmissionDuration - Time taken for submitting the data use reports. DataUsage.Perf.MatchingRuleFetchDuration - Time taken for fetching the matching rules. BUG=574880 ========== to ========== Add histograms for data usage The following UMA are added: DataUsage.MatchingRulesCount.Valid DataUsage.MatchingRulesCount.Invalid - The number of valid and invalid matching rules fetched. DataUsage.Perf.URLRegexMatchDuration - Time taken for a regular expression to process an URL. DataUsage.Perf.ReportSubmissionDuration - Time taken for submitting the data use reports. DataUsage.Perf.MatchingRuleFetchDuration - Time taken for fetching the matching rules. BUG=574880 Committed: https://crrev.com/b582452daa579977cb9200d398d53b4de8cc4ae5 Cr-Commit-Position: refs/heads/master@{#369900} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b582452daa579977cb9200d398d53b4de8cc4ae5 Cr-Commit-Position: refs/heads/master@{#369900} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
