Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2417773005:
Record the time since Chromium is in background for background data use (Closed)
DescriptionRecord the time since Chromium is in background for background data use
DataUse.BackgroundToDataRecievedPerByte.[User|System] histogram is
recorded for each background downstream byte.
DataUse.BackgroundToFirstDownstream.[User|System] histogram is
recorded for the first background downstream byte.
BUG=648808
Committed: https://crrev.com/8766555a6c807796f6c177a3fe4ed81598620324
Cr-Commit-Position: refs/heads/master@{#426386}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed kundaji comments #Patch Set 3 : Fix compile error #
Total comments: 10
Patch Set 4 : Addressed holte comments #
Messages
Total messages: 27 (15 generated)
rajendrant@chromium.org changed reviewers: + kundaji@chromium.org
lgtm https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... components/data_use_measurement/content/data_use_measurement.cc:165: base::TimeTicks::Now() - last_app_background_time_; DCHECK(!last_app_background_time_.is_null()); https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... components/data_use_measurement/content/data_use_measurement.h:159: bool first_read_after_background_; The comment and the variable name imply different things. Variable name implies first read has happened after background. Actual usage reflects the comment. Suggestion renaming to something like: no_reads_since_background_ https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... components/data_use_measurement/content/data_use_measurement_unittest.cc:236: data_use_measurement()->OnApplicationStateChange( Would suggest improving this test by adding request[s] between this and a prior APPLICATION_STATE_HAS_RUNNING_ACTIVITIES. That way test will also confirm that requests made before APPLICATION_STATE_HAS_STOPPED_ACTIVITIES are not counted. https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... components/data_use_measurement/content/data_use_measurement_unittest.cc:249: base::HistogramTester histogram_tester; This and the first test seem exactly the same. Did you mean to add a test for DataUse.BackgroundDownstreamFirstRead.System? This is not testing for the time value in DataUse.BackgroundDownstreamFirstRead.*. You could do it by using helper methods OnApplicationStateChangeInternal() and ReportDataUseUMAInternal() which explicitly take Time::Now() as an argument. However, I don't feel strongly about the value of adding this test, and so leave it to you to decide.
The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rajendrant@chromium.org changed reviewers: + holte@chromium.org
holte: ptal histograms.xml https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... components/data_use_measurement/content/data_use_measurement.cc:165: base::TimeTicks::Now() - last_app_background_time_; On 2016/10/14 23:10:57, kundaji OO until Oct 25 wrote: > DCHECK(!last_app_background_time_.is_null()); Done. https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... File components/data_use_measurement/content/data_use_measurement.h (right): https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... components/data_use_measurement/content/data_use_measurement.h:159: bool first_read_after_background_; On 2016/10/14 23:10:57, kundaji OO until Oct 25 wrote: > The comment and the variable name imply different things. Variable name implies > first read has happened after background. > > Actual usage reflects the comment. Suggestion renaming to something like: > no_reads_since_background_ Done. https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... File components/data_use_measurement/content/data_use_measurement_unittest.cc (right): https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... components/data_use_measurement/content/data_use_measurement_unittest.cc:236: data_use_measurement()->OnApplicationStateChange( On 2016/10/14 23:10:57, kundaji OO until Oct 25 wrote: > Would suggest improving this test by adding request[s] between this and a prior > APPLICATION_STATE_HAS_RUNNING_ACTIVITIES. That way test will also confirm that > requests made before APPLICATION_STATE_HAS_STOPPED_ACTIVITIES are not counted. Done. https://codereview.chromium.org/2417773005/diff/1/components/data_use_measure... components/data_use_measurement/content/data_use_measurement_unittest.cc:249: base::HistogramTester histogram_tester; On 2016/10/14 23:10:57, kundaji OO until Oct 25 wrote: > This and the first test seem exactly the same. Did you mean to add a test for > DataUse.BackgroundDownstreamFirstRead.System? > > This is not testing for the time value in > DataUse.BackgroundDownstreamFirstRead.*. You could do it by using helper methods > OnApplicationStateChangeInternal() and ReportDataUseUMAInternal() which > explicitly take Time::Now() as an argument. However, I don't feel strongly about > the value of adding this test, and so leave it to you to decide. Added a test for DataUse.BackgroundDownstreamFirstRead.System. Yes. I do not feel strongly either. This test just checks that no UMA is recorded when app is in foreground (after foreground-background) transitions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rajendrant@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some naming suggestions, and you are also missing changes to histogram_suffixes. https://codereview.chromium.org/2417773005/diff/40001/components/data_use_mea... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/2417773005/diff/40001/components/data_use_mea... components/data_use_measurement/content/data_use_measurement.cc:53: int64_t value) { Recording a histogram like this is a bit unusual, and value & sample are a bit ambiguous here. Maybe rename value -> count, sample -> time/latency? Maybe name the method IncrementLatencyHistogramByCount? https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8806: +<histogram name="DataUse.BackgroundDownstreamBytes" units="ms"> This is a bit confusingly named, since from the name I would expect it to be just a histogram of #of bytes recorded in background. Maybe BackgroundToDataRecievedPerByte ? https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8826: +<histogram name="DataUse.FirstBackgroundDownstream" units="ms"> Maybe name it BackgroundToFirstDownstream to better indicate that it is a latency histogram. https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8832: + in background. The source of traffic whether from from user browsing or from from? https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8833: + chrome services is added as suffix to this histogram. Missing change to histogram_suffixes?
On 2016/10/17 21:35:16, Steven Holte wrote: > Some naming suggestions, and you are also missing changes to histogram_suffixes. > > https://codereview.chromium.org/2417773005/diff/40001/components/data_use_mea... > File components/data_use_measurement/content/data_use_measurement.cc (right): > > https://codereview.chromium.org/2417773005/diff/40001/components/data_use_mea... > components/data_use_measurement/content/data_use_measurement.cc:53: int64_t > value) { > Recording a histogram like this is a bit unusual, and value & sample are a bit > ambiguous here. > > Maybe rename value -> count, sample -> time/latency? > > Maybe name the method IncrementLatencyHistogramByCount? > > https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:8806: +<histogram > name="DataUse.BackgroundDownstreamBytes" units="ms"> > This is a bit confusingly named, since from the name I would expect it to be > just a histogram of #of bytes recorded in background. > > Maybe BackgroundToDataRecievedPerByte ? > > https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:8826: +<histogram > name="DataUse.FirstBackgroundDownstream" units="ms"> > Maybe name it BackgroundToFirstDownstream to better indicate that it is a > latency histogram. > > https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:8832: + in background. The source of > traffic whether from from user browsing or > from from? > > https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:8833: + chrome services is added as > suffix to this histogram. > Missing change to histogram_suffixes? Thanks for the renaming tips. ptal.
https://codereview.chromium.org/2417773005/diff/40001/components/data_use_mea... File components/data_use_measurement/content/data_use_measurement.cc (right): https://codereview.chromium.org/2417773005/diff/40001/components/data_use_mea... components/data_use_measurement/content/data_use_measurement.cc:53: int64_t value) { On 2016/10/17 21:35:16, Steven Holte wrote: > Recording a histogram like this is a bit unusual, and value & sample are a bit > ambiguous here. > > Maybe rename value -> count, sample -> time/latency? > > Maybe name the method IncrementLatencyHistogramByCount? Done. https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8806: +<histogram name="DataUse.BackgroundDownstreamBytes" units="ms"> On 2016/10/17 21:35:16, Steven Holte wrote: > This is a bit confusingly named, since from the name I would expect it to be > just a histogram of #of bytes recorded in background. > > Maybe BackgroundToDataRecievedPerByte ? Done. https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8826: +<histogram name="DataUse.FirstBackgroundDownstream" units="ms"> On 2016/10/17 21:35:16, Steven Holte wrote: > Maybe name it BackgroundToFirstDownstream to better indicate that it is a > latency histogram. Done. https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8832: + in background. The source of traffic whether from from user browsing or On 2016/10/17 21:35:16, Steven Holte wrote: > from from? Done. https://codereview.chromium.org/2417773005/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8833: + chrome services is added as suffix to this histogram. On 2016/10/17 21:35:16, Steven Holte wrote: > Missing change to histogram_suffixes? Done.
Description was changed from ========== Record the time since Chromium is in background for background data use DataUse.BackgroundDownstreamBytes.[User|System] histogram is recorded for each background downstream byte. DataUse.FirstBackgroundDownstream.[User|System] histogram is recorded for the first background downstream byte. BUG=648808 ========== to ========== Record the time since Chromium is in background for background data use DataUse.BackgroundToDataRecievedPerByte.[User|System] histogram is recorded for each background downstream byte. DataUse.BackgroundToFirstDownstream.[User|System] histogram is recorded for the first background downstream byte. BUG=648808 ==========
histograms lgtm
The CQ bit was checked by rajendrant@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from kundaji@chromium.org Link to the patchset: https://codereview.chromium.org/2417773005/#ps60001 (title: "Addressed holte comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch. On branch working_branch Your branch is up-to-date with 'origin/refs/pending/heads/master'. nothing to commit, working tree clean
Message was sent while issue was closed.
Description was changed from ========== Record the time since Chromium is in background for background data use DataUse.BackgroundToDataRecievedPerByte.[User|System] histogram is recorded for each background downstream byte. DataUse.BackgroundToFirstDownstream.[User|System] histogram is recorded for the first background downstream byte. BUG=648808 ========== to ========== Record the time since Chromium is in background for background data use DataUse.BackgroundToDataRecievedPerByte.[User|System] histogram is recorded for each background downstream byte. DataUse.BackgroundToFirstDownstream.[User|System] histogram is recorded for the first background downstream byte. BUG=648808 Committed: https://crrev.com/8766555a6c807796f6c177a3fe4ed81598620324 Cr-Commit-Position: refs/heads/master@{#426386} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8766555a6c807796f6c177a3fe4ed81598620324 Cr-Commit-Position: refs/heads/master@{#426386}
Message was sent while issue was closed.
On 2016/10/21 13:14:56, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/8766555a6c807796f6c177a3fe4ed81598620324 > Cr-Commit-Position: refs/heads/master@{#426386} FYI, I think Chrome just crashed in the background for me as a result of this change. F/chromium(31165): [FATAL:histogram_base.cc(73)] Check failed: histogram_name() == name (DataUse.BackgroundToFirstDownstream.System vs. DataUse.BackgroundToFirstDownstream.User) Stack was pretty much garbage though. I hit it after I toggled Android's "Don't keep activities" setting in dev options. I "believe" the problem is that the UMA macros cache the values internally and this causes the value to change unexpectedly: UMA_HISTOGRAM_LONG_TIMES( is_user_traffic ? "DataUse.BackgroundToFirstDownstream.User" : "DataUse.BackgroundToFirstDownstream.System", time_since_background); Again no metrics expert, but I think you need to do: if (is_user_traffic) { UMA_HISTOGRAM_LONG_TIMES( "DataUse.BackgroundToFirstDownstream.User", time_since_background); } else { UMA_HISTOGRAM_LONG_TIMES( "DataUse.BackgroundToFirstDownstream.System", time_since_background); }
Message was sent while issue was closed.
On 2016/10/24 18:06:23, Ted C (OOO 10.25 - 30) wrote: > On 2016/10/21 13:14:56, commit-bot: I haz the power wrote: > > Patchset 4 (id:??) landed as > > https://crrev.com/8766555a6c807796f6c177a3fe4ed81598620324 > > Cr-Commit-Position: refs/heads/master@{#426386} > > FYI, I think Chrome just crashed in the background for me as a result > of this change. > > F/chromium(31165): [FATAL:histogram_base.cc(73)] Check failed: histogram_name() > == name (DataUse.BackgroundToFirstDownstream.System vs. > DataUse.BackgroundToFirstDownstream.User) > > Stack was pretty much garbage though. I hit it after I toggled Android's > "Don't keep activities" setting in dev options. > > I "believe" the problem is that the UMA macros cache the values > internally and this causes the value to change unexpectedly: > > UMA_HISTOGRAM_LONG_TIMES( > is_user_traffic ? "DataUse.BackgroundToFirstDownstream.User" > : "DataUse.BackgroundToFirstDownstream.System", > time_since_background); > > Again no metrics expert, but I think you need to do: > > if (is_user_traffic) { > UMA_HISTOGRAM_LONG_TIMES( > "DataUse.BackgroundToFirstDownstream.User", > time_since_background); > } else { > UMA_HISTOGRAM_LONG_TIMES( > "DataUse.BackgroundToFirstDownstream.System", > time_since_background); > } Can confirm, recording two names via a single UMA_HISTOGRAM_LONG_TIMES invocation is a problem. You either need two invocations, or use FactoryGet as in your other histograms. Sorry for not spotting this one. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
