Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1444133002:
Add UMA histograms for data usage tab model (Closed)
DescriptionAdd UMA histograms for data usage tab model
Following histograms are added.
1. Tracking session lifetime from its start to end.
2. How long an old inactive tracking session is maintained in the tab model since it became inactive.
3. How long an expired (active/inactive) tab entry is maintained in the tab model since its latest tracking session.
4. How long an unexpired tab entry is maintained in the tab model.
BUG=557436
Committed: https://crrev.com/78f5f8564bb8e103d9d4e20008a67b7b00b9f217
Cr-Commit-Position: refs/heads/master@{#360458}
Patch Set 1 #
Total comments: 42
Patch Set 2 : Addressed tbansal comments #
Total comments: 6
Patch Set 3 : Addressed tbansal comments #
Total comments: 8
Patch Set 4 : Addressed jwd comments #
Total comments: 6
Patch Set 5 : Rebased & addressed comments #Patch Set 6 : Rebased #
Messages
Total messages: 20 (5 generated)
Description was changed from ========== Add UMA histograms for data usage tab model Following histograms are added. 1. Tracking session lifetime from its start to end. 2. How long an old inactive tracking session is maintained in the tab model since it became inactive. 3. How long an expired (active/inactive) tab entry is maintained in the tab model since its latest tracking session. 4. How long an unexpired tab entry is maintained in the tab model. BUG=547924 ========== to ========== Add UMA histograms for data usage tab model Following histograms are added. 1. Tracking session lifetime from its start to end. 2. How long an old inactive tracking session is maintained in the tab model since it became inactive. 3. How long an expired (active/inactive) tab entry is maintained in the tab model since its latest tracking session. 4. How long an unexpired tab entry is maintained in the tab model. BUG=547924 ==========
rajendrant@chromium.org changed reviewers: + jwd@chromium.org, tbansal@chromium.org
ptal
https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:23: const char kUMAExpiredInactiveTabEntryRemovaltimeSecondsHistogram[] = May be remove Seconds from the variable names (if you end up using UMA_HISTOGRAM_TIMES)? https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:51: base::TimeTicks DataUseTabModel::Now() const { Follow the same order as in .h file. Declare before GetMaxTabEntriesForTests(). https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:189: UMA_HISTOGRAM_COUNTS_1000( Why not UMA_HISTOGRAM_TIMES, or its sibling UMA_HISTOGRAM_LONG_TIMES etc.? Here, and everywhere else in this CL? https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:193: UMA_HISTOGRAM_CUSTOM_COUNTS( Why not UMA_HISTOGRAM_TIMES? https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:197: active_tabs_.erase(tab_entry_iterator); Is this correct? Documentation for erase() says that iterator is invalidated after element is erased. I think what you want to do is if(...isExpired) { .... tab_entry_iterator = active_tabs_.erase(tab_entry_iterator); } else { ++tab_entry_iterator; } https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:217: UMA_HISTOGRAM_COUNTS_1000( Why not UMA_HISTOGRAM_TIMES? https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.h:127: virtual base::TimeTicks Now() const; #include base/time/time.h and remove the include from .cc https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:121: void StartTrackingDataUse(int32_t tab_id, const std::string& label) { It would be easier if DataUseTabModelTest was a derived class of DataUseTabModel. Then you could simply say: using StartTrackingDataUse; (Not sure if parameters need to be specified). https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:129: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; #include the file for SingleThreadTaskRunner. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:349: // Test version of |MockDataUseTabModel|, which permits mocking of calls to Now. I don't understand why this class is needed. Is it not possible to make DataUseTabModelTest (defined above on Line 46), a derived class of DataUseTabModel? Also, mocking has a special meaning in Chromium. "Mocking" term is used when you are using the framework provided by gmock https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/tab_data_use_entry.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/tab_data_use_entry.cc:25: const char kUMATrackingSessionLifetimeSecondsHistogram[] = Remove Seconds/Minutes etc. from the variable names. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/tab_data_use_entry.cc:85: UMA_HISTOGRAM_COUNTS_1000( TIMES? https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/tab_data_use_entry.cc:164: kUMAOldInactiveSessionRemovaltimeSecondsHistogram, TIMES? https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/tab_data_use_entry_unittest.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/tab_data_use_entry_unittest.cc:507: for (size_t session = 1; session < max_sessions_per_tab; session++) { ++session https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6396: +<histogram name="DataUse.TabModel.ExpiredActiveTabEntryRemovaltime" Is it duration or timestamps? If former, then I will call it "RemovalDuration" instead of "Removaltime". https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6435: + When this limit is reached oldest inactive tracking sessions will be removed s/will be/are/ https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6436: + to make room for newer data usage tracking sessions for the same tab. s/to make room for newer data usage tracking sessions for the same tab././ https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6445: + maintained by tab model. This is the interval between the time tracking s/interval/duration/ https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6459: + removing expired tab entries, then oldest unexpired tab entries will be s/will be/are/ https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6460: + removed to make room for newer data usage tracking tab entries. s/to make room for newer data usage tracking tab entries./.
ptal https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:23: const char kUMAExpiredInactiveTabEntryRemovaltimeSecondsHistogram[] = On 2015/11/16 17:17:38, tbansal1 wrote: > May be remove Seconds from the variable names (if you end up using > UMA_HISTOGRAM_TIMES)? Not sure. Each UMA needs different precision. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:51: base::TimeTicks DataUseTabModel::Now() const { On 2015/11/16 17:17:38, tbansal1 wrote: > Follow the same order as in .h file. Declare before GetMaxTabEntriesForTests(). Done. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:189: UMA_HISTOGRAM_COUNTS_1000( On 2015/11/16 17:17:38, tbansal1 wrote: > Why not UMA_HISTOGRAM_TIMES, or its sibling UMA_HISTOGRAM_LONG_TIMES etc.? > Here, and everywhere else in this CL? Tracking expired inactive tab entry removal times in the range 1 second to 16.67 minutes. UMA_HISTOGRAM_TIMES and its variations track in milliseconds and spend a lot of buckets in milliseconds. For tab model millisecond tracking is not needed. So following examples similar to 'Ash.TouchView.TouchViewInactiveTotal', etc. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:193: UMA_HISTOGRAM_CUSTOM_COUNTS( On 2015/11/16 17:17:38, tbansal1 wrote: > Why not UMA_HISTOGRAM_TIMES? Tracking expired active tab entry removal times in the range 1 hour to 5 days. 5 days is the default max removal time - kOpenTabExpirationDurationSeconds. So this UMA could help tune that parameter via field-trial. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:197: active_tabs_.erase(tab_entry_iterator); On 2015/11/16 17:17:38, tbansal1 wrote: > Is this correct? Documentation for erase() says that iterator is invalidated > after element is erased. > > I think what you want to do is > if(...isExpired) { > .... > tab_entry_iterator = active_tabs_.erase(tab_entry_iterator); > } else { > ++tab_entry_iterator; > } Done. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:217: UMA_HISTOGRAM_COUNTS_1000( On 2015/11/16 17:17:38, tbansal1 wrote: > Why not UMA_HISTOGRAM_TIMES? Tracking unexpired tab entry removal times in the range 1 minute to 16.67 hours. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_tab_model.h (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.h:127: virtual base::TimeTicks Now() const; On 2015/11/16 17:17:38, tbansal1 wrote: > #include base/time/time.h > and remove the include from .cc Done. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:121: void StartTrackingDataUse(int32_t tab_id, const std::string& label) { On 2015/11/16 17:17:38, tbansal1 wrote: > It would be easier if DataUseTabModelTest was a derived class of > DataUseTabModel. Then you could simply say: > using StartTrackingDataUse; > (Not sure if parameters need to be specified). DataUseTabModelTest derives from testing::Test. Not sure if its good to derive from DataUseTabModelTest also. I see all tests in components/data_reduction_proxy derive only from testing::Test, and have additional classes that derive from needed base class. These wrapper member functions help avoid the test methods to be FRIEND_TEST_ALL_PREFIXES of DataUseTabModel. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:129: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2015/11/16 17:17:38, tbansal1 wrote: > #include the file for SingleThreadTaskRunner. Done. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:349: // Test version of |MockDataUseTabModel|, which permits mocking of calls to Now. On 2015/11/16 17:17:38, tbansal1 wrote: > I don't understand why this class is needed. Is it not possible to make > DataUseTabModelTest (defined above on Line 46), a derived class of > DataUseTabModel? > > Also, mocking has a special meaning in Chromium. "Mocking" term is used when you > are using the framework provided by gmock Removed the word 'mock', since not using gmock here. Have two classes similar to data_reduction_proxy_debug_ui_manager_unittest.cc TestDataUseTabModel - derives DataUseTabModel to override Now() DataUseTabModelTest - derives testing::Test for all tests. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/tab_data_use_entry.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/tab_data_use_entry.cc:25: const char kUMATrackingSessionLifetimeSecondsHistogram[] = On 2015/11/16 17:17:38, tbansal1 wrote: > Remove Seconds/Minutes etc. from the variable names. Not sure. Each UMA needs different precision. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/tab_data_use_entry.cc:85: UMA_HISTOGRAM_COUNTS_1000( On 2015/11/16 17:17:38, tbansal1 wrote: > TIMES? Tracking session lifetime in the range of 1 second to 16.67 minutes. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/tab_data_use_entry.cc:164: kUMAOldInactiveSessionRemovaltimeSecondsHistogram, On 2015/11/16 17:17:38, tbansal1 wrote: > TIMES? Tracking old inactive tracking session removal times in the range 1 second to 16.67 minutes. https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/tab_data_use_entry_unittest.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/tab_data_use_entry_unittest.cc:507: for (size_t session = 1; session < max_sessions_per_tab; session++) { On 2015/11/16 17:17:38, tbansal1 wrote: > ++session Done. https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6396: +<histogram name="DataUse.TabModel.ExpiredActiveTabEntryRemovaltime" On 2015/11/16 17:17:38, tbansal1 wrote: > Is it duration or timestamps? If former, then I will call it "RemovalDuration" > instead of "Removaltime". Done. https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6435: + When this limit is reached oldest inactive tracking sessions will be removed On 2015/11/16 17:17:39, tbansal1 wrote: > s/will be/are/ Done. https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6436: + to make room for newer data usage tracking sessions for the same tab. On 2015/11/16 17:17:39, tbansal1 wrote: > s/to make room for newer data usage tracking sessions for the same tab././ Done. https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6445: + maintained by tab model. This is the interval between the time tracking On 2015/11/16 17:17:39, tbansal1 wrote: > s/interval/duration/ Done. https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6459: + removing expired tab entries, then oldest unexpired tab entries will be On 2015/11/16 17:17:38, tbansal1 wrote: > s/will be/are/ Done. https://codereview.chromium.org/1444133002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:6460: + removed to make room for newer data usage tracking tab entries. On 2015/11/16 17:17:38, tbansal1 wrote: > s/to make room for newer data usage tracking tab entries./. Done.
https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:189: UMA_HISTOGRAM_COUNTS_1000( On 2015/11/16 20:09:36, Raj wrote: > On 2015/11/16 17:17:38, tbansal1 wrote: > > Why not UMA_HISTOGRAM_TIMES, or its sibling UMA_HISTOGRAM_LONG_TIMES etc.? > > Here, and everywhere else in this CL? > > Tracking expired inactive tab entry removal times in the range 1 second to 16.67 > minutes. > > UMA_HISTOGRAM_TIMES and its variations track in milliseconds and spend a lot of > buckets in milliseconds. > For tab model millisecond tracking is not needed. > So following examples similar to 'Ash.TouchView.TouchViewInactiveTotal', etc. UMA_HISTOGRAM_CUSTOM_TIMES? https://codereview.chromium.org/1444133002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1444133002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:60: // Returns the fake time as Now. s/Returns the fake time as Now./Returns the current time advanced by |now_offset_|./ https://codereview.chromium.org/1444133002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:155: TestDataUseTabModel* test_data_use_tab_model_; Would this cause memory leak? Should this be a scoped_ptr? https://codereview.chromium.org/1444133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1444133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6413: For all these UMA descriptions, it would be nice to specify when the sample is taken (e.g., a sample is taken when a tab entry expires).
ptal https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... File chrome/browser/android/data_usage/data_use_tab_model.cc (right): https://codereview.chromium.org/1444133002/diff/1/chrome/browser/android/data... chrome/browser/android/data_usage/data_use_tab_model.cc:189: UMA_HISTOGRAM_COUNTS_1000( On 2015/11/16 21:09:13, tbansal1 wrote: > On 2015/11/16 20:09:36, Raj wrote: > > On 2015/11/16 17:17:38, tbansal1 wrote: > > > Why not UMA_HISTOGRAM_TIMES, or its sibling UMA_HISTOGRAM_LONG_TIMES etc.? > > > Here, and everywhere else in this CL? > > > > Tracking expired inactive tab entry removal times in the range 1 second to > 16.67 > > minutes. > > > > UMA_HISTOGRAM_TIMES and its variations track in milliseconds and spend a lot > of > > buckets in milliseconds. > > For tab model millisecond tracking is not needed. > > So following examples similar to 'Ash.TouchView.TouchViewInactiveTotal', etc. > > UMA_HISTOGRAM_CUSTOM_TIMES? Done. https://codereview.chromium.org/1444133002/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_tab_model_unittest.cc (right): https://codereview.chromium.org/1444133002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:60: // Returns the fake time as Now. On 2015/11/16 21:09:13, tbansal1 wrote: > s/Returns the fake time as Now./Returns the current time advanced by > |now_offset_|./ Done. https://codereview.chromium.org/1444133002/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_tab_model_unittest.cc:155: TestDataUseTabModel* test_data_use_tab_model_; On 2015/11/16 21:09:13, tbansal1 wrote: > Would this cause memory leak? Should this be a scoped_ptr? test_data_use_tab_model_ and data_use_tab_model_ point to the same object. So scoped_ptr of data_use_tab_model_ would delete the object. Removed test_data_use_tab_model_ member variable for clarity. https://codereview.chromium.org/1444133002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1444133002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6413: On 2015/11/16 21:09:13, tbansal1 wrote: > For all these UMA descriptions, it would be nice to specify when the sample is > taken (e.g., a sample is taken when a tab entry expires). Done.
lgtm
https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6433: + tracking session. Does a tracking session have a start and end? if so, is this since the end of the session or start? https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6433: + tracking session. I would separate the description of what's being logged, and the description of the type of tab entry it's being logged for. Something like: The duration in seconds from a tab entry's latest data usage tracking session, till when the entry is expired and removed from the tab model. This is for tab entries that are still actively tracking data usage for a chrome tab. https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6435: + Tab model maintains the tab entries each pertaining to tracking sessions of nit: "tab model" isn't a proper noun right? If not, can you refer to it as "the tab model" https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6435: + Tab model maintains the tab entries each pertaining to tracking sessions of nit: "entries, each" I think.
ptal https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6433: + tracking session. On 2015/11/17 20:55:22, Jesse Doherty wrote: > Does a tracking session have a start and end? if so, is this since the end of > the session or start? Done. https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6433: + tracking session. On 2015/11/17 20:55:22, Jesse Doherty wrote: > I would separate the description of what's being logged, and the description of > the type of tab entry it's being logged for. > Something like: > The duration in seconds from a tab entry's latest data usage tracking session, > till when the entry is expired and removed from the tab model. This is for tab > entries that are still actively tracking data usage for a chrome tab. Done. https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6435: + Tab model maintains the tab entries each pertaining to tracking sessions of On 2015/11/17 20:55:22, Jesse Doherty wrote: > nit: "tab model" isn't a proper noun right? If not, can you refer to it as "the > tab model" Done. https://codereview.chromium.org/1444133002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6435: + Tab model maintains the tab entries each pertaining to tracking sessions of On 2015/11/17 20:55:22, Jesse Doherty wrote: > nit: "entries, each" I think. Done.
Description was changed from ========== Add UMA histograms for data usage tab model Following histograms are added. 1. Tracking session lifetime from its start to end. 2. How long an old inactive tracking session is maintained in the tab model since it became inactive. 3. How long an expired (active/inactive) tab entry is maintained in the tab model since its latest tracking session. 4. How long an unexpired tab entry is maintained in the tab model. BUG=547924 ========== to ========== Add UMA histograms for data usage tab model Following histograms are added. 1. Tracking session lifetime from its start to end. 2. How long an old inactive tracking session is maintained in the tab model since it became inactive. 3. How long an expired (active/inactive) tab entry is maintained in the tab model since its latest tracking session. 4. How long an unexpired tab entry is maintained in the tab model. BUG=557436 ==========
lgtm % nits. https://codereview.chromium.org/1444133002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1444133002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6427: + units="seconds"> UMA_HISTOGRAM_CUSTOM_TIMES is always in milliseconds. https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... So, s/seconds/milliseconds/ here and in other histograms below. https://codereview.chromium.org/1444133002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6431: + The duration in seconds from the start time of a tab entry's latest data Remove "in seconds" from the description since the units are already specified separately. Here, and in other descriptions below. https://codereview.chromium.org/1444133002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6434: + usage for a Chrome tab. s/Chrome/Chromium Here, and below
ptal, thanks in advance. https://codereview.chromium.org/1444133002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1444133002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6427: + units="seconds"> On 2015/11/18 06:59:57, tbansal1 wrote: > UMA_HISTOGRAM_CUSTOM_TIMES is always in milliseconds. > https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo... > > So, s/seconds/milliseconds/ > here and in other histograms below. Done. https://codereview.chromium.org/1444133002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6431: + The duration in seconds from the start time of a tab entry's latest data On 2015/11/18 06:59:58, tbansal1 wrote: > Remove "in seconds" from the description since the units are already specified > separately. > Here, and in other descriptions below. Done. https://codereview.chromium.org/1444133002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:6434: + usage for a Chrome tab. On 2015/11/18 06:59:58, tbansal1 wrote: > s/Chrome/Chromium > Here, and below Done.
lgtm
lgtm
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 Link to the patchset: https://codereview.chromium.org/1444133002/#ps100001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1444133002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1444133002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/78f5f8564bb8e103d9d4e20008a67b7b00b9f217 Cr-Commit-Position: refs/heads/master@{#360458} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
