Job Controller: clean up metrics logging about the job racing result

Job Controller: clean up metrics logging about the job racing result

BUG=605398
Committed: https://crrev.com/7935a5593b82798f2f054dcde0f1007405a0e911
Cr-Commit-Position: refs/heads/master@{#421059}

[Zhongyi Shi, 2016-09-21 19:31:28]

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

[commit-bot: I haz the power, 2016-09-21 19:32:36]

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

[Zhongyi Shi, 2016-09-21 19:32:54]

zhongyi@chromium.org changed reviewers:
+ rch@chromium.org, tbansal@chromium.org

[Zhongyi Shi, 2016-09-21 19:32:55]

Moving metrics logging to controller. And try to minimize the code duplication
when logging. ptal!

[commit-bot: I haz the power, 2016-09-21 20:37:05]

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

[commit-bot: I haz the power, 2016-09-21 20:37:06]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2016-09-22 17:53:03]

Thanks for doing this.

https://codereview.chromium.org/2359803003/diff/1/net/http/http_server_proper...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2359803003/diff/1/net/http/http_server_proper...
net/http/http_server_properties.cc:26: default:
Add NOTREACHED() in default: so an error is triggered immediately in debug
builds.

https://codereview.chromium.org/2359803003/diff/1/net/http/http_server_proper...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2359803003/diff/1/net/http/http_server_proper...
net/http/http_server_properties.h:64: AlternativeProxyUsage
ConvertProtocolUsageToProxyUsage(
Can this all can go in anonymous namespace in http_server_properties.cc file?

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:1053: DCHECK(main_job_ &&
alternative_job_);
Curious why use a weaker DCHECK?
Is it possible that DCHECK(main_job_.get() == job || alternative_job_.get() ==
job); is actually false?

[Ryan Hamilton, 2016-09-22 21:46:33]

Thanks for doing this. Just on comment in a addition to tbansal's

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:1069: }
nit: I think we can use some early returns:

if (job == main_job_) {
 HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_LOST_RACE,
                                 proxy_server_used);
  return;
}

DCHECK_EQ(alternative_job_.get(), job);
if (job->using_existing_quic_session()) {
  HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE, 
                                  proxy_server_used);
  return;
}

HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_WON_RACE,
                                proxy_server_used);

[Zhongyi Shi, 2016-09-22 23:56:53]

Nits addressed, ptal!

https://codereview.chromium.org/2359803003/diff/1/net/http/http_server_proper...
File net/http/http_server_properties.cc (right):

https://codereview.chromium.org/2359803003/diff/1/net/http/http_server_proper...
net/http/http_server_properties.cc:26: default:
On 2016/09/22 17:53:02, tbansal1 wrote:
> Add NOTREACHED() in default: so an error is triggered immediately in debug
> builds.

Done.

https://codereview.chromium.org/2359803003/diff/1/net/http/http_server_proper...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2359803003/diff/1/net/http/http_server_proper...
net/http/http_server_properties.h:64: AlternativeProxyUsage
ConvertProtocolUsageToProxyUsage(
On 2016/09/22 17:53:02, tbansal1 wrote:
> Can this all can go in anonymous namespace in http_server_properties.cc file?

Done.

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:1053: DCHECK(main_job_ &&
alternative_job_);
On 2016/09/22 17:53:02, tbansal1 wrote:
> Curious why use a weaker DCHECK?
> Is it possible that DCHECK(main_job_.get() == job || alternative_job_.get() ==
> job); is actually false?

The new DCHECK was to make sure both jobs are alive when we figure out
reporting. And the if and DCHECK @1066 enforces:
DCHECK(main_job_.get() == job || alternative_job_.get() ==
job).

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:1069: }
On 2016/09/22 21:46:33, Ryan Hamilton wrote:
> nit: I think we can use some early returns:
> 
> if (job == main_job_) {
>  HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_LOST_RACE,
>                                  proxy_server_used);
>   return;
> }
> 
> DCHECK_EQ(alternative_job_.get(), job);
> if (job->using_existing_quic_session()) {
>   HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_NO_RACE, 
>                                   proxy_server_used);
>   return;
> }
> 
> HistogramAlternateProtocolUsage(ALTERNATE_PROTOCOL_USAGE_WON_RACE,
>                                 proxy_server_used);

Done.

[tbansal1, 2016-09-23 00:02:07]

lgtm % 1 nit. Thanks for doing this.

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
File net/http/http_stream_factory_impl_job_controller.cc (right):

https://codereview.chromium.org/2359803003/diff/1/net/http/http_stream_factor...
net/http/http_stream_factory_impl_job_controller.cc:1053: DCHECK(main_job_ &&
alternative_job_);
On 2016/09/22 23:56:53, Zhongyi Shi wrote:
> On 2016/09/22 17:53:02, tbansal1 wrote:
> > Curious why use a weaker DCHECK?
> > Is it possible that DCHECK(main_job_.get() == job || alternative_job_.get()
==
> > job); is actually false?
> 
> The new DCHECK was to make sure both jobs are alive when we figure out
> reporting. And the if and DCHECK @1066 enforces:
> DCHECK(main_job_.get() == job || alternative_job_.get() ==
> job).
>  

Acknowledged.

https://codereview.chromium.org/2359803003/diff/20001/net/http/http_server_pr...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/2359803003/diff/20001/net/http/http_server_pr...
net/http/http_server_properties.h:53: enum AlternativeProxyUsage {
nit: this enum can also move to anonymous namespace in .cc file.

[Ryan Hamilton, 2016-09-23 19:20:47]

lgtm

[Zhongyi Shi, 2016-09-26 20:01:08]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2016-09-26 20:01:09]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org,
tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2359803003/#ps40001
(title: "move proxy-server specific enum to anonymous namespace")

[commit-bot: I haz the power, 2016-09-26 20:02:25]

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-26 20:06:47]

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

[commit-bot: I haz the power, 2016-09-26 20:06:48]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Zhongyi Shi, 2016-09-26 22:13:01]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2016-09-26 22:13:01]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org,
tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2359803003/#ps60001
(title: "sync")

[commit-bot: I haz the power, 2016-09-26 22:13:43]

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-26 23:24:20]

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

[commit-bot: I haz the power, 2016-09-26 23:24:21]

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

[Zhongyi Shi, 2016-09-26 23:44:50]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2016-09-26 23:45:39]

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-27 00:57:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-27 01:04:43]

Description was changed from

==========
Job Controller: clean up metrics logging about the job racing result

BUG=605398
==========

to

==========
Job Controller: clean up metrics logging about the job racing result

BUG=605398

Committed: https://crrev.com/7935a5593b82798f2f054dcde0f1007405a0e911
Cr-Commit-Position: refs/heads/master@{#421059}
==========

[commit-bot: I haz the power, 2016-09-27 01:04:44]

Patchset 4 (id:??) landed as
https://crrev.com/7935a5593b82798f2f054dcde0f1007405a0e911
Cr-Commit-Position: refs/heads/master@{#421059}