Split a MetricsLogStore object out of MetricsLogManager.

Split a MetricsLogStore object out of MetricsLogManager.

The MetricsLogStore handles all serialized logs, while MetricsLogManager
handles logs which are still being produced.

Creates a LogStore interface for MetricsLogStore and PersistedLogs to
implement.  This will simplify creating a ReportingService that can be reused
by both UKM and UMA.

No functional changes intended.

BUG=693676
Review-Url: https://codereview.chromium.org/2689323010
Cr-Commit-Position: refs/heads/master@{#452356}
Committed: https://chromium.googlesource.com/chromium/src/+/ade6fdf493bec82b248ad76bc1d378f5d3ea99e4

[Steven Holte, 2017-02-17 01:09:32]

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

[commit-bot: I haz the power, 2017-02-17 01:10:41]

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, 2017-02-17 01:22:16]

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

[commit-bot: I haz the power, 2017-02-17 01:22:18]

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

[Steven Holte, 2017-02-17 02:06:57]

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

[commit-bot: I haz the power, 2017-02-17 02:08:11]

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

[Steven Holte, 2017-02-17 03:07:17]

holte@chromium.org changed reviewers:
+ rkaplow@chromium.org

[commit-bot: I haz the power, 2017-02-17 04:11:05]

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

[commit-bot: I haz the power, 2017-02-17 04:11:06]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Steven Holte, 2017-02-17 20:22:07]

Description was changed from

==========
Split a MetricsLogStore object out of MetricsLogManager.

The MetricsLogStore handles all serialized logs, while MetricsLogManager
handles logs which are still being produced.

Creates a LogStore interface for MetricsLogStore and PersistedLogs to
implement.  This will simplify creating a ReportingService that can be reused
by both UKM and UMA.

BUG=
==========

to

==========
Split a MetricsLogStore object out of MetricsLogManager.

The MetricsLogStore handles all serialized logs, while MetricsLogManager
handles logs which are still being produced.

Creates a LogStore interface for MetricsLogStore and PersistedLogs to
implement.  This will simplify creating a ReportingService that can be reused
by both UKM and UMA.

BUG=693676
==========

[Steven Holte, 2017-02-17 20:22:26]

Description was changed from

==========
Split a MetricsLogStore object out of MetricsLogManager.

The MetricsLogStore handles all serialized logs, while MetricsLogManager
handles logs which are still being produced.

Creates a LogStore interface for MetricsLogStore and PersistedLogs to
implement.  This will simplify creating a ReportingService that can be reused
by both UKM and UMA.

BUG=693676
==========

to

==========
Split a MetricsLogStore object out of MetricsLogManager.

The MetricsLogStore handles all serialized logs, while MetricsLogManager
handles logs which are still being produced.

Creates a LogStore interface for MetricsLogStore and PersistedLogs to
implement.  This will simplify creating a ReportingService that can be reused
by both UKM and UMA.

No functional changes intended.

BUG=693676
==========

[rkaplow, 2017-02-20 21:24:14]

may want brian or alexei to take a look as well

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/log_...
File components/metrics/log_store.h (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/log_...
components/metrics/log_store.h:12: // Interface for local storage of logs to be
reported.
nit, since you're organizing this into an interface, might be worth adding a
couple lines of explaination on how this can get used, i.e. what features are
available

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/log_...
components/metrics/log_store.h:30: // The staged_log will remain the same even
if additional logs are added.
can you define a bit more what 'next' means in this context

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_manager_unittest.cc (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_manager_unittest.cc:103: 
should this test be re-thought? We're not checking the type of the final log
anymore. 
I'd also comment on this test, mentioning more what edge case we're worried
about

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_store.cc (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store.cc:30: // The number of bytes each of
initial and ongoing logs that must be stored.
reword first sentance, reads a bit janky to me

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store.cc:33: const size_t
kStorageByteLimitPerLogType = 300000;
prefer something like  = 300 * 1000 // ~300kB

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_store.h (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store.h:20: // Interface for local storage of
logs to be reported.
kind of strange wording, should you instead say this is an implementation using
PersistedLogs?

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store.h:52: // Logs that have not yet been sent.
i'd say worth mentioning what the difference is-  what differentiates them and
why it needs to be seperate

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_store_unittest.cc (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store_unittest.cc:31: list_length =
I would just return pref_service_.Get(...) here, and then remove the else and
just return.

Or use type = log_type == MetricsLog::INITIAL_STABILITY_LOG ?
prefs::kMetricsInitialLogs : prefs::kMetricsOngoing
and return it that way

[Steven Holte, 2017-02-21 21:51:19]

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/log_...
File components/metrics/log_store.h (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/log_...
components/metrics/log_store.h:12: // Interface for local storage of logs to be
reported.
On 2017/02/20 21:24:13, rkaplow wrote:
> nit, since you're organizing this into an interface, might be worth adding a
> couple lines of explaination on how this can get used, i.e. what features are
> available

Done.

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/log_...
components/metrics/log_store.h:30: // The staged_log will remain the same even
if additional logs are added.
On 2017/02/20 21:24:13, rkaplow wrote:
> can you define a bit more what 'next' means in this context

Added a comment saying this is explicitly left to the implementation.

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_manager_unittest.cc (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_manager_unittest.cc:103: 
On 2017/02/20 21:24:13, rkaplow wrote:
> should this test be re-thought? We're not checking the type of the final log
> anymore. 
> I'd also comment on this test, mentioning more what edge case we're worried
> about

I think the test is fine, since the initial/ongoing_log_counts should test the
same thing.  Added comments.

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_store.cc (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store.cc:30: // The number of bytes each of
initial and ongoing logs that must be stored.
On 2017/02/20 21:24:13, rkaplow wrote:
> reword first sentance, reads a bit janky to me

Done.

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store.cc:33: const size_t
kStorageByteLimitPerLogType = 300000;
On 2017/02/20 21:24:13, rkaplow wrote:
> prefer something like  = 300 * 1000 // ~300kB

Done.

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_store.h (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store.h:20: // Interface for local storage of
logs to be reported.
On 2017/02/20 21:24:13, rkaplow wrote:
> kind of strange wording, should you instead say this is an implementation
using
> PersistedLogs?

Wrote a new description.

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store.h:52: // Logs that have not yet been sent.
On 2017/02/20 21:24:13, rkaplow wrote:
> i'd say worth mentioning what the difference is-  what differentiates them and
> why it needs to be seperate

Done.

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_store_unittest.cc (right):

https://codereview.chromium.org/2689323010/diff/20001/components/metrics/metr...
components/metrics/metrics_log_store_unittest.cc:31: list_length =
On 2017/02/20 21:24:13, rkaplow wrote:
> I would just return pref_service_.Get(...) here, and then remove the else and
> just return.
> 
> Or use type = log_type == MetricsLog::INITIAL_STABILITY_LOG ?
> prefs::kMetricsInitialLogs : prefs::kMetricsOngoing
> and return it that way

Done.

[Steven Holte, 2017-02-21 21:53:47]

holte@chromium.org changed reviewers:
+ bcwhite@chromium.org

[Steven Holte, 2017-02-21 21:53:48]

+Brian

[bcwhite, 2017-02-22 16:25:09]

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/log_...
File components/metrics/log_store.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/log_...
components/metrics/log_store.h:18: virtual bool has_unsent_logs() const = 0;
Is "hacker style" still appropriate for these methods?

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
File components/metrics/metrics_log_manager.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_manager.h:22: // track of an progress log and a
paused log.
an in-progress log?

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_manager.h:35: // Closes current_log(), compresses
it, and stores it in the log_store for
|current_log_|
|log_store|

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
File components/metrics/metrics_log_store.cc (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_store.cc:33: const size_t
kStorageByteLimitPerLogType = 300 * 1000;  // ~300kB
How about: 300 << 10  // 300 KiB

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
File components/metrics/metrics_log_store.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_store.h:30: // types we'll be using.
"types being used".

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_store.h:36: // metrics::LogStore implementation
Just metrics::LogStore:

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/pers...
File components/metrics/persisted_logs.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/pers...
components/metrics/persisted_logs.h:46: // LogStore:
metrics::LogStore:

[Steven Holte, 2017-02-22 20:35:50]

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/log_...
File components/metrics/log_store.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/log_...
components/metrics/log_store.h:18: virtual bool has_unsent_logs() const = 0;
On 2017/02/22 16:25:08, bcwhite wrote:
> Is "hacker style" still appropriate for these methods?

I thinks so.  These should still accessors, and the style guide says "Accessors
may be named like variables"
https://google.github.io/styleguide/cppguide.html#Function_Names

There is also
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
but I'm pretty sure that means virtual functions shouldn't be declared inline,
not that they shouldn't be named with unix_hacker_style.

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
File components/metrics/metrics_log_manager.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_manager.h:22: // track of an progress log and a
paused log.
On 2017/02/22 16:25:08, bcwhite wrote:
> an in-progress log?

Done.

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_manager.h:35: // Closes current_log(), compresses
it, and stores it in the log_store for
On 2017/02/22 16:25:09, bcwhite wrote:
> |current_log_|
> |log_store|

Done.

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
File components/metrics/metrics_log_store.cc (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_store.cc:33: const size_t
kStorageByteLimitPerLogType = 300 * 1000;  // ~300kB
On 2017/02/22 16:25:09, bcwhite wrote:
> How about: 300 << 10  // 300 KiB

I don't want to change behavior in this CL.

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
File components/metrics/metrics_log_store.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_store.h:30: // types we'll be using.
On 2017/02/22 16:25:09, bcwhite wrote:
> "types being used".

Replaced this with a simpler comment.

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/metr...
components/metrics/metrics_log_store.h:36: // metrics::LogStore implementation
On 2017/02/22 16:25:09, bcwhite wrote:
> Just metrics::LogStore:

Done.

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/pers...
File components/metrics/persisted_logs.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/pers...
components/metrics/persisted_logs.h:46: // LogStore:
On 2017/02/22 16:25:09, bcwhite wrote:
> metrics::LogStore:

Done.

[bcwhite, 2017-02-22 20:39:24]

lgtm

[Alexei Svitkine (slow), 2017-02-22 20:43:45]

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

[Alexei Svitkine (slow), 2017-02-22 20:43:47]

drive-by lgtm with nits

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/log_...
File components/metrics/log_store.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/log_...
components/metrics/log_store.h:18: virtual bool has_unsent_logs() const = 0;
On 2017/02/22 20:35:50, Steven Holte wrote:
> On 2017/02/22 16:25:08, bcwhite wrote:
> > Is "hacker style" still appropriate for these methods?
> 
> I thinks so.  These should still accessors, and the style guide says
"Accessors
> may be named like variables"
> https://google.github.io/styleguide/cppguide.html#Function_Names
> 
> There is also
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> but I'm pretty sure that means virtual functions shouldn't be declared inline,
> not that they shouldn't be named with unix_hacker_style.

My understanding is that virtual functions should be CamelCase because there's
no guarantee what the implementation class does with them.

Alternatively, you could make these not pure virtual and have concrete
implementations for them and have the relevant state be stored in the base
class.

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/metr...
File components/metrics/metrics_log_store.h (right):

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/metr...
components/metrics/metrics_log_store.h:26: MetricsLogStore(PrefService*
local_state, size_t max_retransmit_size);
Nit: Document the param?

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/metr...
components/metrics/metrics_log_store.h:35: // metrics::LogStore:
Nit: Remove metrics::

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/pers...
File components/metrics/persisted_logs.h (right):

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/pers...
components/metrics/persisted_logs.h:46: // metrics::LogStore:
Nit: This file is in metrics:: namespace so no need for prefix here.

https://codereview.chromium.org/2689323010/diff/60001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2689323010/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:233: if (!persisted_logs_.has_unsent_logs()) {
Nit: No {}'s

[rkaplow, 2017-02-22 20:43:54]

lgtm

[Steven Holte, 2017-02-22 22:15:54]

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/log_...
File components/metrics/log_store.h (right):

https://codereview.chromium.org/2689323010/diff/40001/components/metrics/log_...
components/metrics/log_store.h:18: virtual bool has_unsent_logs() const = 0;
On 2017/02/22 20:43:46, Alexei Svitkine (slow) wrote:
> On 2017/02/22 20:35:50, Steven Holte wrote:
> > On 2017/02/22 16:25:08, bcwhite wrote:
> > > Is "hacker style" still appropriate for these methods?
> > 
> > I thinks so.  These should still accessors, and the style guide says
> "Accessors
> > may be named like variables"
> > https://google.github.io/styleguide/cppguide.html#Function_Names
> > 
> > There is also
> >
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> > but I'm pretty sure that means virtual functions shouldn't be declared
inline,
> > not that they shouldn't be named with unix_hacker_style.
> 
> My understanding is that virtual functions should be CamelCase because there's
> no guarantee what the implementation class does with them.
> 
> Alternatively, you could make these not pure virtual and have concrete
> implementations for them and have the relevant state be stored in the base
> class.

I have to disagree here.  A function being a accessor or not is an interface
detail, not an implementation detail.  It implies that it doesn't modify the
state of the object.  If the implementation didn't do that, it would be
violating the expectations of interface, in the same way that it would if
calling DiscardStagedLog did something completely different.

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/metr...
File components/metrics/metrics_log_store.h (right):

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/metr...
components/metrics/metrics_log_store.h:26: MetricsLogStore(PrefService*
local_state, size_t max_retransmit_size);
On 2017/02/22 20:43:46, Alexei Svitkine (slow) wrote:
> Nit: Document the param?

Done.

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/metr...
components/metrics/metrics_log_store.h:35: // metrics::LogStore:
On 2017/02/22 20:43:46, Alexei Svitkine (slow) wrote:
> Nit: Remove metrics::

Done, also removed the one I added in persisted_logs.h from bcwhite@'s comment,
since both are in metrics namespace.

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/pers...
File components/metrics/persisted_logs.h (right):

https://codereview.chromium.org/2689323010/diff/60001/components/metrics/pers...
components/metrics/persisted_logs.h:46: // metrics::LogStore:
On 2017/02/22 20:43:46, Alexei Svitkine (slow) wrote:
> Nit: This file is in metrics:: namespace so no need for prefix here.

Done.

https://codereview.chromium.org/2689323010/diff/60001/components/ukm/ukm_serv...
File components/ukm/ukm_service.cc (right):

https://codereview.chromium.org/2689323010/diff/60001/components/ukm/ukm_serv...
components/ukm/ukm_service.cc:233: if (!persisted_logs_.has_unsent_logs()) {
On 2017/02/22 20:43:46, Alexei Svitkine (slow) wrote:
> Nit: No {}'s

Done.

[Steven Holte, 2017-02-22 22:16:35]

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

[commit-bot: I haz the power, 2017-02-22 22:17:26]

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, 2017-02-22 23:45:33]

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

[commit-bot: I haz the power, 2017-02-22 23:45:34]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-02-23 01:05:23]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2017-02-23 01:05:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
bcwhite@chromium.org, rkaplow@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2689323010/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-23 01:06:02]

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, 2017-02-23 02:42:43]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487811923641820,
"parent_rev": "c0ff4c0d40a8b9ab8ed809a446aa298450c23462", "commit_rev":
"ade6fdf493bec82b248ad76bc1d378f5d3ea99e4"}

[commit-bot: I haz the power, 2017-02-23 02:43:30]

Description was changed from

==========
Split a MetricsLogStore object out of MetricsLogManager.

The MetricsLogStore handles all serialized logs, while MetricsLogManager
handles logs which are still being produced.

Creates a LogStore interface for MetricsLogStore and PersistedLogs to
implement.  This will simplify creating a ReportingService that can be reused
by both UKM and UMA.

No functional changes intended.

BUG=693676
==========

to

==========
Split a MetricsLogStore object out of MetricsLogManager.

The MetricsLogStore handles all serialized logs, while MetricsLogManager
handles logs which are still being produced.

Creates a LogStore interface for MetricsLogStore and PersistedLogs to
implement.  This will simplify creating a ReportingService that can be reused
by both UKM and UMA.

No functional changes intended.

BUG=693676

Review-Url: https://codereview.chromium.org/2689323010
Cr-Commit-Position: refs/heads/master@{#452356}
Committed:
https://chromium.googlesource.com/chromium/src/+/ade6fdf493bec82b248ad76bc1d3...
==========

[commit-bot: I haz the power, 2017-02-23 02:43:31]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ade6fdf493bec82b248ad76bc1d3...