Log aggregated abort metrics when histograms are uploaded

Log aggregated abort metrics when histograms are uploaded

This patch adds a method on NetworkMetricsProvider that logs aggregate ABORT
metrics on the NetworkMetricsProvider::ProvideGeneralMetrics callback. This
gives finer grained abort statistics.

BUG=557430
Committed: https://crrev.com/305ab900c774b5a46f0637dc2638e45e3f3c7905
Cr-Commit-Position: refs/heads/master@{#381780}

[Charlie Harrison, 2016-03-14 20:26:31]

csharrison@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Charlie Harrison, 2016-03-14 20:26:31]

asvitkine@, is this an okay approach?

[Charlie Harrison, 2016-03-16 13:11:41]

csharrison@chromium.org changed reviewers:
+ isherman@chromium.org

[Charlie Harrison, 2016-03-16 13:11:42]

+isherman we discussed this briefly via email.

[Ilya Sherman, 2016-03-16 22:33:41]

https://codereview.chromium.org/1796983002/diff/1/components/metrics/net/netw...
File components/metrics/net/network_metrics_provider.cc (right):

https://codereview.chromium.org/1796983002/diff/1/components/metrics/net/netw...
components/metrics/net/network_metrics_provider.cc:252:
scoped_ptr<base::HistogramSamples> samples = error_codes->SnapshotSamples();
Hmm, I think this will give you the total number of samples seen since the
launch of Chrome, rather than since the last upload.  Have you tested that this
behaves as you'd expect?  (I was kind of thinking that you'd store this count
separately, outside of the histogram, rather than trying to read from the
histogram.  I guess reading from the histogram has the nice benefit that you
don't have to duplicate the storage (and maybe have a bug in that logic), but it
has the disadvantage that you have to keep track of deltas somehow.  Unless
you're intentionally uploading data from the launch of Chrome, rather than
deltas?)

https://codereview.chromium.org/1796983002/diff/1/components/metrics/net/netw...
components/metrics/net/network_metrics_provider.cc:258: (100.0 * aborts) /
total_codes);
Hmm, this looks like an implicit conversion from float to int.  Please make the
conversion explicit.  (Note: I'm referring to the conversion *back* to int, when
emitting to the histogram -- not the upconversion to floating point that comes
from multiplying by 100.0.  Though, I think multiplying by 100 rather than 100.0
would probably give you the desired result.)

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

https://codereview.chromium.org/1796983002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:24934: +<histogram
name="Net.ErrAborted.CountPerUpload" units="counts">
nit: I'd either write something like units="aborts", or just omit the units
entirely.

[Charlie Harrison, 2016-03-17 14:03:20]

Thanks for the review. I think the delta method is more appealing than doing a
thread hop / sharing data between UI / IO to access aggregated abort metrics,
but let me know what you think.

https://codereview.chromium.org/1796983002/diff/1/components/metrics/net/netw...
File components/metrics/net/network_metrics_provider.cc (right):

https://codereview.chromium.org/1796983002/diff/1/components/metrics/net/netw...
components/metrics/net/network_metrics_provider.cc:252:
scoped_ptr<base::HistogramSamples> samples = error_codes->SnapshotSamples();
On 2016/03/16 at 22:33:41, Ilya Sherman wrote:
> Hmm, I think this will give you the total number of samples seen since the
launch of Chrome, rather than since the last upload.  Have you tested that this
behaves as you'd expect?  (I was kind of thinking that you'd store this count
separately, outside of the histogram, rather than trying to read from the
histogram.  I guess reading from the histogram has the nice benefit that you
don't have to duplicate the storage (and maybe have a bug in that logic), but it
has the disadvantage that you have to keep track of deltas somehow.  Unless
you're intentionally uploading data from the launch of Chrome, rather than
deltas?)

Ah I misunderstood and thought that SnapshotSamples only tracked values from the
last upload. This count is maintained on the IO thread so for simplicity I've
used a delta based approach. Is there a simple way to force these methods to
fire for testing?

https://codereview.chromium.org/1796983002/diff/1/components/metrics/net/netw...
components/metrics/net/network_metrics_provider.cc:258: (100.0 * aborts) /
total_codes);
On 2016/03/16 at 22:33:41, Ilya Sherman wrote:
> Hmm, this looks like an implicit conversion from float to int.  Please make
the conversion explicit.  (Note: I'm referring to the conversion *back* to int,
when emitting to the histogram -- not the upconversion to floating point that
comes from multiplying by 100.0.  Though, I think multiplying by 100 rather than
100.0 would probably give you the desired result.)

Changed to a multiplication by 100.

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

https://codereview.chromium.org/1796983002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:24934: +<histogram
name="Net.ErrAborted.CountPerUpload" units="counts">
On 2016/03/16 at 22:33:41, Ilya Sherman wrote:
> nit: I'd either write something like units="aborts", or just omit the units
entirely.

Changed to units="aborts"

[Ilya Sherman, 2016-03-17 16:42:33]

LGTM, thanks.

[Charlie Harrison, 2016-03-17 18:35:42]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2016-03-17 18:36:21]

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

[commit-bot: I haz the power, 2016-03-17 19:51:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-17 19:53:09]

Description was changed from

==========
Log aggregated abort metrics when histograms are uploaded

This patch adds a method on NetworkMetricsProvider that logs aggregate ABORT
metrics on the NetworkMetricsProvider::ProvideGeneralMetrics callback. This
gives finer grained abort statistics.

BUG=557430
==========

to

==========
Log aggregated abort metrics when histograms are uploaded

This patch adds a method on NetworkMetricsProvider that logs aggregate ABORT
metrics on the NetworkMetricsProvider::ProvideGeneralMetrics callback. This
gives finer grained abort statistics.

BUG=557430

Committed: https://crrev.com/305ab900c774b5a46f0637dc2638e45e3f3c7905
Cr-Commit-Position: refs/heads/master@{#381780}
==========

[commit-bot: I haz the power, 2016-03-17 19:53:10]

Patchset 2 (id:??) landed as
https://crrev.com/305ab900c774b5a46f0637dc2638e45e3f3c7905
Cr-Commit-Position: refs/heads/master@{#381780}