Add experimental feature info to certificate reports

Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment. This will allow us to measure the results
of the network time service via cert reports data, not just via UMA.

BUG=659179, 589700
TBR=battre@chromium.org

Committed: https://crrev.com/40d0800b0956677f82cb7ba32085ac2907f792af
Cr-Commit-Position: refs/heads/master@{#431688}

[estark, 2016-10-25 22:09:06]

Description was changed from

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment.

BUG=589700
==========

to

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment.

BUG=659179,589700
==========

[estark, 2016-10-25 22:09:20]

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

[estark, 2016-10-25 22:09:51]

Description was changed from

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment.

BUG=659179,589700
==========

to

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment. This will allow us to measure the results
of the network time service via cert reports data, not just via UMA.

BUG=659179,589700
==========

[commit-bot: I haz the power, 2016-10-25 22:10:04]

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

[estark, 2016-10-25 22:10:35]

estark@chromium.org changed reviewers:
+ meacer@chromium.org

[estark, 2016-10-25 22:10:35]

meacer, another one for you to review, please.

[commit-bot: I haz the power, 2016-10-25 23:07:55]

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

[commit-bot: I haz the power, 2016-10-25 23:07:56]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-10-26 20:20:31]

LGTM. Do we need a privacy review for this change?

https://codereview.chromium.org/2448943004/diff/1/components/certificate_repo...
File components/certificate_reporting/error_report_unittest.cc (right):

https://codereview.chromium.org/2448943004/diff/1/components/certificate_repo...
components/certificate_reporting/error_report_unittest.cc:201:
field_trial_list.reset();
Needed?

[estark, 2016-10-28 00:02:34]

> LGTM. Do we need a privacy review for this change?

Per email thread with battre, no concerns from privacy.

Thanks meacer!

https://codereview.chromium.org/2448943004/diff/1/components/certificate_repo...
File components/certificate_reporting/error_report_unittest.cc (right):

https://codereview.chromium.org/2448943004/diff/1/components/certificate_repo...
components/certificate_reporting/error_report_unittest.cc:201:
field_trial_list.reset();
On 2016/10/26 20:20:31, Mustafa Emre Acer wrote:
> Needed?

Done.

[estark, 2016-10-28 00:03:43]

estark@chromium.org changed reviewers:
+ jwd@chromium.org

[estark, 2016-10-28 00:03:44]

jwd, can you please review for the addition of +components/variations to DEPS?

[estark, 2016-10-28 00:04:07]

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

[commit-bot: I haz the power, 2016-10-28 00:04:43]

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-10-28 01:34:47]

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

[commit-bot: I haz the power, 2016-10-28 01:34:49]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jwd, 2016-11-02 18:05:22]

https://codereview.chromium.org/2448943004/diff/20001/components/certificate_...
File components/certificate_reporting/error_report.h (right):

https://codereview.chromium.org/2448943004/diff/20001/components/certificate_...
components/certificate_reporting/error_report.h:73: void AddFeature(const
base::Feature& feature);
Can you have this take in a list of the params you care about? That way it's
clear what params are being included.

[estark, 2016-11-02 20:54:33]

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

[commit-bot: I haz the power, 2016-11-02 20:55:12]

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

[estark, 2016-11-02 20:55:12]

(Also note that I will want to add a browser test for this, but will wait until
https://codereview.chromium.org/2449193002/ settles down/lands first, because
that'll make it easier to do so.)

https://codereview.chromium.org/2448943004/diff/20001/components/certificate_...
File components/certificate_reporting/error_report.h (right):

https://codereview.chromium.org/2448943004/diff/20001/components/certificate_...
components/certificate_reporting/error_report.h:73: void AddFeature(const
base::Feature& feature);
On 2016/11/02 18:05:22, jwd wrote:
> Can you have this take in a list of the params you care about? That way it's
> clear what params are being included.

Done.

[estark, 2016-11-02 20:58:28]

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

[commit-bot: I haz the power, 2016-11-02 20:58:58]

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-11-02 23:37:18]

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

[commit-bot: I haz the power, 2016-11-02 23:37:19]

Dry run: This issue passed the CQ dry run.

[jwd, 2016-11-03 15:15:34]

https://codereview.chromium.org/2448943004/diff/100001/components/certificate...
File components/certificate_reporting/error_report.h (right):

https://codereview.chromium.org/2448943004/diff/100001/components/certificate...
components/certificate_reporting/error_report.h:76: const std::map<std::string,
std::string>& params);
Hmmm, I actually liked passing the feature itself more. I was thinking of
passing a list of the param keys, letting AddFeature get the values based on the
given feature. Thoughts?

https://codereview.chromium.org/2448943004/diff/100001/components/network_tim...
File components/network_time/network_time_tracker.h (right):

https://codereview.chromium.org/2448943004/diff/100001/components/network_tim...
components/network_time/network_time_tracker.h:50: // time queries can be issued
on-demand.
Seeing as I've asked you to reduce the magicness of what params get reported,
could you add a comment somewhere that makes sense (maybe here?) that new
parameters need to be added in cert_report_helper for them to be included in
cert reports?

[estark, 2016-11-03 15:21:41]

You know, I'm thinking this might be easier for everyone if I change this to
just record the single feature + parameter that I care about, in
error_report.cc. jwd, do you think so? So the proto change (in
pseudo-proto-syntax) would be:

message FinchFeatureInfo {
  bool is_network_time_querying_enabled;
  enum NetworkTimeFetchBehavior {
    ON_DEMAND_ONLY,
    BACKGROUND_ONLY,
    ON_DEMAND_AND_IN_BACKGROUND
  };
  NetworkTimeFetchBehavior fetch_behavior;
}

That could alleviate some of jwd's magic-ness concerns, would simplify the
review for the server-side version of this proto, and Mustafa pointed out the
other day that it would make for easier queries on the data as well.

It would be more annoying to add information about other Finch
features/parameters later, but not really that annoying.

jwd, how does that sound to you?

[jwd, 2016-11-03 18:16:25]

On 2016/11/03 15:21:41, estark wrote:
> You know, I'm thinking this might be easier for everyone if I change this to
> just record the single feature + parameter that I care about, in
> error_report.cc. jwd, do you think so? So the proto change (in
> pseudo-proto-syntax) would be:
> 
> message FinchFeatureInfo {
>   bool is_network_time_querying_enabled;
>   enum NetworkTimeFetchBehavior {
>     ON_DEMAND_ONLY,
>     BACKGROUND_ONLY,
>     ON_DEMAND_AND_IN_BACKGROUND
>   };
>   NetworkTimeFetchBehavior fetch_behavior;
> }
> 
> That could alleviate some of jwd's magic-ness concerns, would simplify the
> review for the server-side version of this proto, and Mustafa pointed out the
> other day that it would make for easier queries on the data as well.
> 
> It would be more annoying to add information about other Finch
> features/parameters later, but not really that annoying.
> 
> jwd, how does that sound to you?

That sounds perfect.

[estark, 2016-11-03 22:35:03]

On 2016/11/03 18:16:25, jwd wrote:
> On 2016/11/03 15:21:41, estark wrote:
> > You know, I'm thinking this might be easier for everyone if I change this to
> > just record the single feature + parameter that I care about, in
> > error_report.cc. jwd, do you think so? So the proto change (in
> > pseudo-proto-syntax) would be:
> > 
> > message FinchFeatureInfo {
> >   bool is_network_time_querying_enabled;
> >   enum NetworkTimeFetchBehavior {
> >     ON_DEMAND_ONLY,
> >     BACKGROUND_ONLY,
> >     ON_DEMAND_AND_IN_BACKGROUND
> >   };
> >   NetworkTimeFetchBehavior fetch_behavior;
> > }
> > 
> > That could alleviate some of jwd's magic-ness concerns, would simplify the
> > review for the server-side version of this proto, and Mustafa pointed out
the
> > other day that it would make for easier queries on the data as well.
> > 
> > It would be more annoying to add information about other Finch
> > features/parameters later, but not really that annoying.
> > 
> > jwd, how does that sound to you?
> 
> That sounds perfect.

Alright, I went ahead and did that, PTAL. Mustafa -- you might want to take
another look too because things changed around a bit since you LG'ed. Thanks!

(Also note that I rebased on https://codereview.chromium.org/2449193002/ which
makes this a bit easier to test.)

[meacer, 2016-11-04 18:44:22]

Still lgtm.

https://codereview.chromium.org/2448943004/diff/180001/components/certificate...
File components/certificate_reporting/error_report.cc (right):

https://codereview.chromium.org/2448943004/diff/180001/components/certificate...
components/certificate_reporting/error_report.cc:134:
network_time_tracker->GetFetchBehavior();
nit: Inline this in the switch condition?

switch(network_time_tracker->GetFetchBehavior()) {
...
}

https://codereview.chromium.org/2448943004/diff/180001/components/network_tim...
File components/network_time/network_time_tracker.h (right):

https://codereview.chromium.org/2448943004/diff/180001/components/network_tim...
components/network_time/network_time_tracker.h:82: // Only used in case of an
unrecognize Finch experiment parameter.
unrecognize -> unrecognized

[jwd, 2016-11-04 21:44:26]

LGTM

[estark, 2016-11-11 20:13:49]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-11 20:13:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2448943004/#ps220001
(title: "remove redundant parameter checking code")

[commit-bot: I haz the power, 2016-11-11 20:14:20]

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-11-11 20:19:15]

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

[commit-bot: I haz the power, 2016-11-11 20:19:16]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[estark, 2016-11-11 20:31:58]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-11 20:31:58]

The patchset sent to the CQ was uploaded after l-g-t-m from jwd@chromium.org,
meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2448943004/#ps240001
(title: "add dep to BUILD.gn")

[commit-bot: I haz the power, 2016-11-11 20:32:44]

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-11-11 20:45:24]

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

[commit-bot: I haz the power, 2016-11-11 20:45:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-11-11 20:52:27]

estark@chromium.org changed reviewers:
+ battre@chromium.org

[estark, 2016-11-11 20:52:27]

+battre for addition of components/prefs to
components/certificate_reporting/DEPS

(this is the CL we discussed over email, adding information about a Finch
experiment to Safe Browsing Extended Reporting certificate reports)

[battre (please use the other), 2016-11-11 21:32:07]

battre@google.com changed reviewers:
+ battre@google.com

[battre (please use the other), 2016-11-11 21:32:08]

LGTM to adding the dependency. Note that the try bots are still unhappy.

https://codereview.chromium.org/2448943004/diff/240001/components/certificate...
File components/certificate_reporting/cert_logger.proto (right):

https://codereview.chromium.org/2448943004/diff/240001/components/certificate...
components/certificate_reporting/cert_logger.proto:66: optional
NetworkTimeQueryingInfo network_time_querying_info = 2;
Nit: = 1?

[estark, 2016-11-11 22:24:38]

Thanks. I fixed the problem that the windows bots were unhappy about.

https://codereview.chromium.org/2448943004/diff/240001/components/certificate...
File components/certificate_reporting/cert_logger.proto (right):

https://codereview.chromium.org/2448943004/diff/240001/components/certificate...
components/certificate_reporting/cert_logger.proto:66: optional
NetworkTimeQueryingInfo network_time_querying_info = 2;
On 2016/11/11 21:32:08, battre (please use the other) wrote:
> Nit: = 1?

1 is assigned to |network_time_queries_enabled| on line 52.

[estark, 2016-11-11 22:24:55]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-11 22:24:55]

The patchset sent to the CQ was uploaded after l-g-t-m from battre@google.com,
jwd@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2448943004/#ps260001
(title: "fix Windows compile problem")

[commit-bot: I haz the power, 2016-11-11 22:25:43]

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

[estark, 2016-11-11 22:27:05]

The CQ bit was unchecked by estark@chromium.org

[estark, 2016-11-11 22:28:01]

On 2016/11/11 22:24:38, estark (slow thru Nov 18) wrote:
> Thanks. I fixed the problem that the windows bots were unhappy about.
> 
>
https://codereview.chromium.org/2448943004/diff/240001/components/certificate...
> File components/certificate_reporting/cert_logger.proto (right):
> 
>
https://codereview.chromium.org/2448943004/diff/240001/components/certificate...
> components/certificate_reporting/cert_logger.proto:66: optional
> NetworkTimeQueryingInfo network_time_querying_info = 2;
> On 2016/11/11 21:32:08, battre (please use the other) wrote:
> > Nit: = 1?
> 
> 1 is assigned to |network_time_queries_enabled| on line 52.

Nope, I'm bad at reading. :) Fixed.

[estark, 2016-11-11 22:29:24]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-11 22:29:24]

The patchset sent to the CQ was uploaded after l-g-t-m from battre@google.com,
jwd@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2448943004/#ps280001
(title: "battre comment")

[commit-bot: I haz the power, 2016-11-11 22:30:16]

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-11-11 22:40:46]

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

[commit-bot: I haz the power, 2016-11-11 22:40:48]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estark, 2016-11-11 22:48:54]

Description was changed from

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment. This will allow us to measure the results
of the network time service via cert reports data, not just via UMA.

BUG=659179,589700
==========

to

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment. This will allow us to measure the results
of the network time service via cert reports data, not just via UMA.

BUG=659179,589700
TBR=battre@chromium.org
==========

[estark, 2016-11-11 22:51:54]

TBRing battre@chromium.org because he lgtm'ed from his other account

[estark, 2016-11-11 22:52:04]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-11-11 22:52:31]

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-11-12 00:14:26]

Description was changed from

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment. This will allow us to measure the results
of the network time service via cert reports data, not just via UMA.

BUG=659179,589700
TBR=battre@chromium.org
==========

to

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment. This will allow us to measure the results
of the network time service via cert reports data, not just via UMA.

BUG=659179,589700
TBR=battre@chromium.org
==========

[commit-bot: I haz the power, 2016-11-12 00:14:31]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-11-12 00:16:57]

Description was changed from

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment. This will allow us to measure the results
of the network time service via cert reports data, not just via UMA.

BUG=659179,589700
TBR=battre@chromium.org
==========

to

==========
Add experimental feature info to certificate reports

In poking around certificate reports and UMA, I've noticed that
certificate reports give somewhat different data than UMA about how
often the bad clock interstitial is shown. (This is likely due to the
fact that users have to connect to Google over HTTPS to report UMA data,
whereas certificate reports are sent encrypted over HTTP.)

This CL adds a field to the cert report to contain information about
Finch features that affect certificate validation warnings, such as the
network time service experiment. This will allow us to measure the results
of the network time service via cert reports data, not just via UMA.

BUG=659179,589700
TBR=battre@chromium.org

Committed: https://crrev.com/40d0800b0956677f82cb7ba32085ac2907f792af
Cr-Commit-Position: refs/heads/master@{#431688}
==========

[commit-bot: I haz the power, 2016-11-12 00:16:59]

Patchset 15 (id:??) landed as
https://crrev.com/40d0800b0956677f82cb7ba32085ac2907f792af
Cr-Commit-Position: refs/heads/master@{#431688}