Avoid some pref writes in MetricsLog. Ensure some stability metrics are cleared.

Avoid some pref writes in MetricsLog. Ensure some stability metrics are cleared.

Also fix a typo.

BUG=642878
Committed: https://crrev.com/54157007c56c2004cfd19014be6035137fb7d70d
Cr-Commit-Position: refs/heads/master@{#415860}

[manzagop (departed), 2016-08-31 16:20:46]

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

[manzagop (departed), 2016-08-31 16:20:47]

Hi asvitkine@,

As per your suggesiton, I broke off up part of
https://codereview.chromium.org/2296543002/.

Thanks!

Pierre

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
File components/metrics/metrics_log.cc (right):

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:207: if (log_type() != INITIAL_STABILITY_LOG)
What is the reason for skipping these stats? Is it storage impact? 

Not skipping them would shorten the reporting delay in the case of an early
crash (because sometimes there is no initial stability report (when there is no
system profile which can happen with an early crash)).

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:213: // TODO(jar): The following are all
optional in SystemProfileProto::Stability,
This todo is still valid because it's about not setting the value in the proto.
I'm guessing it requires some server side changes to ensure a missing value is
interpreted as a 0. 

I'm guessing the motivation for doing this is for server side disk use. Not
setting each value would save ~3bytes (tag+value), for a total of 15 bytes per
initial stability report.

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:222: int breakpad_registration_success_count =
These last 4 metrics are never cleared. Should they be cleared here?
https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?l=521

[Alexei Svitkine (slow), 2016-08-31 16:40:00]

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
File components/metrics/metrics_log.cc (right):

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:207: if (log_type() != INITIAL_STABILITY_LOG)
On 2016/08/31 16:20:47, manzagop wrote:
> What is the reason for skipping these stats? Is it storage impact? 
> 
> Not skipping them would shorten the reporting delay in the case of an early
> crash (because sometimes there is no initial stability report (when there is
no
> system profile which can happen with an early crash)).

I think the logic was that it was not necessary to log them outside of the
initial stability log context. As you point out, that reasoning doesn't make
sense in the case when there's no previous system profile saved. (I don't think
it was considering that case.)

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:213: // TODO(jar): The following are all
optional in SystemProfileProto::Stability,
On 2016/08/31 16:20:47, manzagop wrote:
> This todo is still valid because it's about not setting the value in the
proto.
> I'm guessing it requires some server side changes to ensure a missing value is
> interpreted as a 0. 
> 
> I'm guessing the motivation for doing this is for server side disk use. Not
> setting each value would save ~3bytes (tag+value), for a total of 15 bytes per
> initial stability report.

No server-side change is needed, so the TODO can be removed if you don't set the
fields when the values are 0. Values default to 0 already if not present (unless
there's special custom logic to do something else, of course).

Yeah, this is mainly about saving the bandwidth of transmitting 0's.

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:222: int breakpad_registration_success_count =
On 2016/08/31 16:20:47, manzagop wrote:
> These last 4 metrics are never cleared. Should they be cleared here?
>
https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?l=521

Yes, I think so.

Although, I wonder if anyone ever uses these metrics. Maybe we should just
remove them entirely.

[manzagop (departed), 2016-08-31 19:26:52]

Thanks for the answers! Please have a look.

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
File components/metrics/metrics_log.cc (right):

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:207: if (log_type() != INITIAL_STABILITY_LOG)
On 2016/08/31 16:40:00, Alexei Svitkine (slow) wrote:
> On 2016/08/31 16:20:47, manzagop wrote:
> > What is the reason for skipping these stats? Is it storage impact? 
> > 
> > Not skipping them would shorten the reporting delay in the case of an early
> > crash (because sometimes there is no initial stability report (when there is
> no
> > system profile which can happen with an early crash)).
> 
> I think the logic was that it was not necessary to log them outside of the
> initial stability log context. As you point out, that reasoning doesn't make
> sense in the case when there's no previous system profile saved. (I don't
think
> it was considering that case.)

Ok, removing. Won't cause a storage size increase because also addressing the
TODO to not set 0s in the proto.

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:213: // TODO(jar): The following are all
optional in SystemProfileProto::Stability,
On 2016/08/31 16:40:00, Alexei Svitkine (slow) wrote:
> On 2016/08/31 16:20:47, manzagop wrote:
> > This todo is still valid because it's about not setting the value in the
> proto.
> > I'm guessing it requires some server side changes to ensure a missing value
is
> > interpreted as a 0. 
> > 
> > I'm guessing the motivation for doing this is for server side disk use. Not
> > setting each value would save ~3bytes (tag+value), for a total of 15 bytes
per
> > initial stability report.
> 
> No server-side change is needed, so the TODO can be removed if you don't set
the
> fields when the values are 0. Values default to 0 already if not present
(unless
> there's special custom logic to do something else, of course).
> 
> Yeah, this is mainly about saving the bandwidth of transmitting 0's.

Great! Done.

https://codereview.chromium.org/2294323002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log.cc:222: int breakpad_registration_success_count =
On 2016/08/31 16:40:00, Alexei Svitkine (slow) wrote:
> On 2016/08/31 16:20:47, manzagop wrote:
> > These last 4 metrics are never cleared. Should they be cleared here?
> >
>
https://cs.chromium.org/chromium/src/components/metrics/metrics_service.cc?l=521
> 
> Yes, I think so.
> 
> Although, I wonder if anyone ever uses these metrics. Maybe we should just
> remove them entirely.

Not feeling comfortable making that call / do the cleanup, so I'll stick with
just clearing the values for now if that's ok with you.

[manzagop (departed), 2016-08-31 19:27:01]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 19:27:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-08-31 19:44:34]

LGTM, thanks!

Please file a crbug for this though and set BUG=

You can mark it as Fixed once this lands.

[commit-bot: I haz the power, 2016-08-31 19:45:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 19:45:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[manzagop (departed), 2016-08-31 20:36:59]

Description was changed from

==========
Avoid some pref writes in MetricsLog. Also fix a typo.
==========

to

==========
Avoid some pref writes in MetricsLog. Ensure some stability metrics are cleared.

Also fix a typo.

BUG=642878
==========

[manzagop (departed), 2016-08-31 21:50:27]

The CQ bit was checked by manzagop@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 21:51:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 23:24:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 23:24:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[manzagop (departed), 2016-09-01 02:03:04]

Thanks for the review.

Had to change a unit test. Added a bug.

[manzagop (departed), 2016-09-01 02:03:11]

The CQ bit was checked by manzagop@chromium.org

[manzagop (departed), 2016-09-01 02:03:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2294323002/#ps40001
(title: "Fix unittests")

[commit-bot: I haz the power, 2016-09-01 02:03:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 02:44:15]

Description was changed from

==========
Avoid some pref writes in MetricsLog. Ensure some stability metrics are cleared.

Also fix a typo.

BUG=642878
==========

to

==========
Avoid some pref writes in MetricsLog. Ensure some stability metrics are cleared.

Also fix a typo.

BUG=642878
==========

[commit-bot: I haz the power, 2016-09-01 02:44:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-01 02:46:20]

Description was changed from

==========
Avoid some pref writes in MetricsLog. Ensure some stability metrics are cleared.

Also fix a typo.

BUG=642878
==========

to

==========
Avoid some pref writes in MetricsLog. Ensure some stability metrics are cleared.

Also fix a typo.

BUG=642878

Committed: https://crrev.com/54157007c56c2004cfd19014be6035137fb7d70d
Cr-Commit-Position: refs/heads/master@{#415860}
==========

[commit-bot: I haz the power, 2016-09-01 02:46:21]

Patchset 3 (id:??) landed as
https://crrev.com/54157007c56c2004cfd19014be6035137fb7d70d
Cr-Commit-Position: refs/heads/master@{#415860}