Move logic for uploading logs into a ReportingService object.

Move logic for uploading logs into a ReportingService object.

Gathers the logic related to handling serialized logs for upload into
a ReportingService object which is specialized for UMA with a MetricsReportingService subclass.

No functional changes intended.

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

[Steven Holte, 2017-02-24 22:34:35]

Description was changed from

==========
Refactor UMA and Rappor to share more uploading logic.
==========

to

==========
Move logic for uploading logs into a ReportingService object.

Gathers the logic related to handling serialized logs for upload into
a ReportingService object which is specialized for UMA with a
MetricsReportingService subclass.

No functional changes intended.

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

[Steven Holte, 2017-02-24 22:46:41]

Description was changed from

==========
Move logic for uploading logs into a ReportingService object.

Gathers the logic related to handling serialized logs for upload into
a ReportingService object which is specialized for UMA with a
MetricsReportingService subclass.

No functional changes intended.

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

to

==========
Gathers the logic related to handling serialized logs for upload into
a ReportingService object which is specialized for UMA with a
MetricsReportingService subclass.

No functional changes intended.

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

[Steven Holte, 2017-02-24 22:47:20]

Description was changed from

==========
Gathers the logic related to handling serialized logs for upload into
a ReportingService object which is specialized for UMA with a
MetricsReportingService subclass.

No functional changes intended.

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

to

==========
Move logic for uploading logs into a ReportingService object.

Gathers the logic related to handling serialized logs for upload into
a ReportingService object which is specialized for UMA with a
MetricsReportingService subclass.

No functional changes intended.

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

[Steven Holte, 2017-02-24 22:49:40]

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

[Steven Holte, 2017-02-27 20:58:44]

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

[commit-bot: I haz the power, 2017-02-27 20:59: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-27 21:07:25]

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

[commit-bot: I haz the power, 2017-02-27 21:07:26]

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

[Steven Holte, 2017-02-27 22:55:22]

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

[commit-bot: I haz the power, 2017-02-27 22:56:23]

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-28 00:15:09]

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

[commit-bot: I haz the power, 2017-02-28 00:15:10]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-02-28 18:03:45]

I am worried this is adding a lot of complexity - via all the relationships
between the classes.

Can you at least add clear comments to the new classes about their purposes?

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/met...
components/metrics/metrics_service.cc:365: return
reporting_service_.metrics_log_store()->has_unsent_logs();
Nit: can you just use log_store().has_unsent_logs() and not have this extra
function?

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
File components/metrics/reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:29: // See metrics_service.cc for a
detailed description.
I think this should have its own description. Or at least a summary of what this
class is from.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:33: // and
Nit: Wrapping is off.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:69: // At startup, prefs needs to be
called with a list of all the pref names and
"prefs needs to be called" isn't very clear to me. Can you rephrase? i.e. "pref
names should be registered".

Is this call meant to be done only once - and not once per subclass? If so,
mention that.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:81: virtual std::string upload_url() = 0;
return by const string ref?

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:86: virtual void
LogActualUploadInterval(base::TimeDelta interval){};
Nit: Add space before {

Same for others.

[Steven Holte, 2017-03-01 02:02:34]

Now with actual class comments!

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/met...
components/metrics/metrics_service.cc:365: return
reporting_service_.metrics_log_store()->has_unsent_logs();
On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> Nit: can you just use log_store().has_unsent_logs() and not have this extra
> function?

Not quite.  I would need a const version of log_store(), so I'd still have an
extra function.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
File components/metrics/reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:29: // See metrics_service.cc for a
detailed description.
On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> I think this should have its own description. Or at least a summary of what
this
> class is from.

Done.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:33: // and
On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> Nit: Wrapping is off.

Done.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:69: // At startup, prefs needs to be
called with a list of all the pref names and
On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> "prefs needs to be called" isn't very clear to me. Can you rephrase? i.e.
"pref
> names should be registered".
> 
> Is this call meant to be done only once - and not once per subclass? If so,
> mention that.

Rephrased (on MetricsService too) and mentioned here that it should only be
called once.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:81: virtual std::string upload_url() = 0;
On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> return by const string ref?

I can return const strings, but not const string refs, since these both
construct the strings.  However, the strings should still only be constructed
once, due to RVO.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:86: virtual void
LogActualUploadInterval(base::TimeDelta interval){};
On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> Nit: Add space before {
> 
> Same for others.

clang-format deletes these spaces

[Alexei Svitkine (slow), 2017-03-01 16:06:30]

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
File components/metrics/reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:81: virtual std::string upload_url() = 0;
On 2017/03/01 02:02:33, Steven Holte wrote:
> On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> > return by const string ref?
> 
> I can return const strings, but not const string refs, since these both
> construct the strings.  However, the strings should still only be constructed
> once, due to RVO.

I don't think returning const strings makes much sense. If it's being returned
by value, the const isn't really useful.

Also upload_url() runs non-trivial code (body of GetMetricsServerUrl()), so
please don't use hacker_style for it.

Whereas upload_mime_type() is returning a char[] constant. So maybe return it by
StringPiece? And change the params where its passed to also use StringPiece.

Also, can the functions be const? (rather than their return types)

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:86: virtual void
LogActualUploadInterval(base::TimeDelta interval){};
On 2017/03/01 02:02:33, Steven Holte wrote:
> On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> > Nit: Add space before {
> > 
> > Same for others.
> 
> clang-format deletes these spaces

Weird.

I noticed you have semicolons at the end - which are unecessary when you have a
body. Can you remove them? Also on line 74. Does clang format still remove the
spaces?

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
File components/metrics/metrics_reporting_service.cc (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
components/metrics/metrics_reporting_service.cc:103:
UMA_HISTOGRAM_COUNTS("UMA.Large Rejected Log was Discarded",
Nit: We now prefer the COUNTS_1M macro which has same semantics as the counts
one.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
File components/metrics/metrics_reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
components/metrics/metrics_reporting_service.h:26: // UMA logs.  It uses a
MetricsLogStore as its LogStore, reports to the UMA
Nit: Remove extra space after the .

Especially since you don't use it below.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
components/metrics/metrics_reporting_service.h:41: MetricsLogStore*
metrics_log_store() { return &metrics_log_store_; }
Nit: Move these above the static.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
components/metrics/metrics_service.h:355: // Sub-Service for uploading logs.
Nit: Don't capitalize service

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
File components/metrics/reporting_service.cc (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
components/metrics/reporting_service.cc:75:
DCHECK(thread_checker_.CalledOnValidThread());
Nit: Add these to all the other functions.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
File components/metrics/reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
components/metrics/reporting_service.h:29: // ReportingService is a abstract
class which uploads serialized logs from a
a -> an

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
components/metrics/reporting_service.h:30: // LogStore to a remote server.  A
concrete implementation of this class must
Nit: Remove extra space after .

[Steven Holte, 2017-03-04 01:35:51]

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
File components/metrics/reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:81: virtual std::string upload_url() = 0;
On 2017/03/01 16:06:29, Alexei Svitkine (very slow) wrote:
> On 2017/03/01 02:02:33, Steven Holte wrote:
> > On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> > > return by const string ref?
> > 
> > I can return const strings, but not const string refs, since these both
> > construct the strings.  However, the strings should still only be
constructed
> > once, due to RVO.
> 
> I don't think returning const strings makes much sense. If it's being returned
> by value, the const isn't really useful.
> 
> Also upload_url() runs non-trivial code (body of GetMetricsServerUrl()), so
> please don't use hacker_style for it.
> 
> Whereas upload_mime_type() is returning a char[] constant. So maybe return it
by
> StringPiece? And change the params where its passed to also use StringPiece.
> 
> Also, can the functions be const? (rather than their return types)

Done.

https://codereview.chromium.org/2608833002/diff/160001/components/metrics/rep...
components/metrics/reporting_service.h:86: virtual void
LogActualUploadInterval(base::TimeDelta interval){};
On 2017/03/01 16:06:29, Alexei Svitkine (very slow) wrote:
> On 2017/03/01 02:02:33, Steven Holte wrote:
> > On 2017/02/28 18:03:45, Alexei Svitkine (slow) wrote:
> > > Nit: Add space before {
> > > 
> > > Same for others.
> > 
> > clang-format deletes these spaces
> 
> Weird.
> 
> I noticed you have semicolons at the end - which are unecessary when you have
a
> body. Can you remove them? Also on line 74. Does clang format still remove the
> spaces?

Ah, yes it was the semicolons.  Fixed.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
File components/metrics/metrics_reporting_service.cc (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
components/metrics/metrics_reporting_service.cc:103:
UMA_HISTOGRAM_COUNTS("UMA.Large Rejected Log was Discarded",
On 2017/03/01 16:06:29, Alexei Svitkine (very slow) wrote:
> Nit: We now prefer the COUNTS_1M macro which has same semantics as the counts
> one.

Done.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
File components/metrics/metrics_reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
components/metrics/metrics_reporting_service.h:26: // UMA logs.  It uses a
MetricsLogStore as its LogStore, reports to the UMA
On 2017/03/01 16:06:29, Alexei Svitkine (very slow) wrote:
> Nit: Remove extra space after the .
> 
> Especially since you don't use it below.

Done.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
components/metrics/metrics_reporting_service.h:41: MetricsLogStore*
metrics_log_store() { return &metrics_log_store_; }
On 2017/03/01 16:06:29, Alexei Svitkine (very slow) wrote:
> Nit: Move these above the static.

Done.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
File components/metrics/metrics_service.h (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/met...
components/metrics/metrics_service.h:355: // Sub-Service for uploading logs.
On 2017/03/01 16:06:29, Alexei Svitkine (very slow) wrote:
> Nit: Don't capitalize service

Done.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
File components/metrics/reporting_service.cc (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
components/metrics/reporting_service.cc:75:
DCHECK(thread_checker_.CalledOnValidThread());
On 2017/03/01 16:06:29, Alexei Svitkine (very slow) wrote:
> Nit: Add these to all the other functions.

Done.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
File components/metrics/reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
components/metrics/reporting_service.h:29: // ReportingService is a abstract
class which uploads serialized logs from a
On 2017/03/01 16:06:30, Alexei Svitkine (very slow) wrote:
> a -> an

Done.

https://codereview.chromium.org/2608833002/diff/180001/components/metrics/rep...
components/metrics/reporting_service.h:30: // LogStore to a remote server.  A
concrete implementation of this class must
On 2017/03/01 16:06:29, Alexei Svitkine (very slow) wrote:
> Nit: Remove extra space after .

Done.

[Alexei Svitkine (slow), 2017-03-06 22:15:22]

lgtm

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
File components/metrics/metrics_reporting_service.cc (right):

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
components/metrics/metrics_reporting_service.cc:72: return
metrics::kDefaultMetricsMimeType;
Nit: No metrics::

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
components/metrics/metrics_reporting_service.cc:77: return
metrics::MetricsLogUploader::UMA;
Nit: No metrics::

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
File components/metrics/metrics_reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
components/metrics/metrics_reporting_service.h:31: // |max_retransmit_size|.
Does not take ownership of the parameters; instead
Nit: Update comment since max_retransmit_size isn't a param here.

[Steven Holte, 2017-03-06 22:31:40]

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
File components/metrics/metrics_reporting_service.cc (right):

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
components/metrics/metrics_reporting_service.cc:72: return
metrics::kDefaultMetricsMimeType;
On 2017/03/06 22:15:21, Alexei Svitkine (very slow) wrote:
> Nit: No metrics::

Done.

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
components/metrics/metrics_reporting_service.cc:77: return
metrics::MetricsLogUploader::UMA;
On 2017/03/06 22:15:21, Alexei Svitkine (very slow) wrote:
> Nit: No metrics::

Done.

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
File components/metrics/metrics_reporting_service.h (right):

https://codereview.chromium.org/2608833002/diff/200001/components/metrics/met...
components/metrics/metrics_reporting_service.h:31: // |max_retransmit_size|.
Does not take ownership of the parameters; instead
On 2017/03/06 22:15:21, Alexei Svitkine (very slow) wrote:
> Nit: Update comment since max_retransmit_size isn't a param here.

Done.

[Steven Holte, 2017-03-08 01:24:23]

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

[commit-bot: I haz the power, 2017-03-08 01:25: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-03-08 02:38:28]

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

[commit-bot: I haz the power, 2017-03-08 02:38:29]

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

[Steven Holte, 2017-03-14 00:50:27]

The CQ bit was checked by holte@chromium.org

[Steven Holte, 2017-03-14 00:50:27]

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/2608833002/#ps260001
(title: "Rebase again")

[commit-bot: I haz the power, 2017-03-14 00:50:51]

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-03-14 02:08:31]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1489452627606840,
"parent_rev": "d8f66a4262f5974c8fa17d1b1f3081227b693191", "commit_rev":
"a3b241168ae7b85d5cc31e6b2f4afc8cd1cbe869"}

[commit-bot: I haz the power, 2017-03-14 02:09:14]

Description was changed from

==========
Move logic for uploading logs into a ReportingService object.

Gathers the logic related to handling serialized logs for upload into
a ReportingService object which is specialized for UMA with a
MetricsReportingService subclass.

No functional changes intended.

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

to

==========
Move logic for uploading logs into a ReportingService object.

Gathers the logic related to handling serialized logs for upload into
a ReportingService object which is specialized for UMA with a
MetricsReportingService subclass.

No functional changes intended.

BUG=693676

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

[commit-bot: I haz the power, 2017-03-14 02:09:15]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/a3b241168ae7b85d5cc31e6b2f4a...