Chromecast: command-line switch to override the UMA metric upload URL.

Chromecast: command-line switch to override the UMA metric upload URL.

R=byungchul@chromium.org,isherman@chromium.org
BUG=None

Committed: https://crrev.com/db879ef83d0b84eeb4d4bc6a77249ace1b2b13ba
Cr-Commit-Position: refs/heads/master@{#292791}

[gunsch, 2014-08-26 17:48:08]

Hi there,
 Looking to get your thoughts on this. We regularly set up a local UMA server to
verify metrics reporting is working end-to-end as expected in Chromecast.

We could easily add this functionality just to our own MetricsServiceClient, but
it seems general enough to be appropriate to add here.

[Ilya Sherman, 2014-08-27 00:33:44]

I'm concerned that this might make it easier for malware to disable UMA
reporting.  I'd prefer not to include this flag, or perhaps to only include it
in developer builds.

[gunsch, 2014-08-28 22:43:58]

gunsch@chromium.org changed reviewers:
+ byungchul@chromium.org
- asvitkine@chromium.org

[gunsch, 2014-08-28 22:44:05]

On 2014/08/28 22:43:58, gunsch wrote:
> mailto:gunsch@chromium.org changed reviewers:
> + mailto:byungchul@chromium.org
> - mailto:asvitkine@chromium.org

That sounds good. Moved behavior to just chromecast/. (+byungchul to review).

[Alexei Svitkine (slow), 2014-08-29 23:45:21]

I agree with Ilya's concern.

Have you considered having this flag only work in debug builds? Or does
that not meet your use case?


On Tue, Aug 26, 2014 at 5:33 PM, <isherman@chromium.org> wrote:

> I'm concerned that this might make it easier for malware to disable UMA
> reporting.  I'd prefer not to include this flag, or perhaps to only
> include it
> in developer builds.
>
> https://codereview.chromium.org/506083003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gunsch, 2014-08-29 23:47:38]

On 2014/08/29 23:45:21, Alexei Svitkine wrote:
> I agree with Ilya's concern.
> 
> Have you considered having this flag only work in debug builds? Or does
> that not meet your use case?
> 
> 
> On Tue, Aug 26, 2014 at 5:33 PM, <mailto:isherman@chromium.org> wrote:
> 
> > I'm concerned that this might make it easier for malware to disable UMA
> > reporting.  I'd prefer not to include this flag, or perhaps to only
> > include it
> > in developer builds.
> >
> > https://codereview.chromium.org/506083003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

The concern doesn't apply to Chromecast, as users are not able to change command
line flags. I fully understand the concern with having the flag in
components/metrics, however.

Other than that, we can't really restrict this feature to Debug builds. Debug
build performance is essentially unusable on the dongles, even for engineering,
due to the performance limitations of the device.

[Alexei Svitkine (slow), 2014-08-29 23:51:06]

Fair enough, I'm OK with having this in the Chromecast code only. (My
comment was about making it a more generic thing.)

By the way (maybe offline) it would be interesting for us (UMA team) to
know how you use this - i.e. what do you do with a custom UMA server.


On Fri, Aug 29, 2014 at 4:47 PM, <gunsch@chromium.org> wrote:

> Reviewers: byungchul, Ilya Sherman, Alexei Svitkine,
>
> Message:
>
> On 2014/08/29 23:45:21, Alexei Svitkine wrote:
>
>> I agree with Ilya's concern.
>>
>
>  Have you considered having this flag only work in debug builds? Or does
>> that not meet your use case?
>>
>
>
>  On Tue, Aug 26, 2014 at 5:33 PM, <mailto:isherman@chromium.org> wrote:
>>
>
>  > I'm concerned that this might make it easier for malware to disable UMA
>> > reporting.  I'd prefer not to include this flag, or perhaps to only
>> > include it
>> > in developer builds.
>> >
>> > https://codereview.chromium.org/506083003/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> The concern doesn't apply to Chromecast, as users are not able to change
> command
> line flags. I fully understand the concern with having the flag in
> components/metrics, however.
>
> Other than that, we can't really restrict this feature to Debug builds.
> Debug
> build performance is essentially unusable on the dongles, even for
> engineering,
> due to the performance limitations of the device.
>
> Description:
> Chromecast: command-line switch to override the UMA metric upload URL.
>
> R=byungchul@chromium.org,isherman@chromium.org
>
> BUG=None
>
> Please review this at https://codereview.chromium.org/506083003/
>
> SVN Base: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+21, -14 lines):
>   M chromecast/chromecast.gyp
>   A + chromecast/common/chromecast_switches.h
>   A + chromecast/common/chromecast_switches.cc
>   M chromecast/metrics/cast_metrics_service_client.cc
>
>
> Index: chromecast/chromecast.gyp
> diff --git a/chromecast/chromecast.gyp b/chromecast/chromecast.gyp
> index 5b7ffe40cbd11f4da04d3e2ee538b3df4f76a7bb..
> da6423aeafd0a6b50da5093c55c1eb73fd575543 100644
> --- a/chromecast/chromecast.gyp
> +++ b/chromecast/chromecast.gyp
> @@ -26,6 +26,8 @@
>          'common/cast_resource_delegate.h',
>          'common/chromecast_config.cc',
>          'common/chromecast_config.h',
> +        'common/chromecast_switches.cc',
> +        'common/chromecast_switches.h',
>          'common/pref_names.cc',
>          'common/pref_names.h',
>        ],
> Index: chromecast/common/chromecast_switches.cc
> diff --git a/athena/common/switches.h b/chromecast/common/
> chromecast_switches.cc
> similarity index 53%
> copy from athena/common/switches.h
> copy to chromecast/common/chromecast_switches.cc
> index ce68164e8125168b1cacfec9d5a3c05d32bb5eba..
> 90b524b4bb4ff086747118a672a61d3af6afbd83 100644
> --- a/athena/common/switches.h
> +++ b/chromecast/common/chromecast_switches.cc
> @@ -2,15 +2,11 @@
>  // Use of this source code is governed by a BSD-style license that can be
>  // found in the LICENSE file.
>
> -#ifndef ATHENA_COMMON_SWITCHES_H_
> -#define ATHENA_COMMON_SWITCHES_H_
> +#include "chromecast/common/chromecast_switches.h"
>
> -namespace athena {
>  namespace switches {
>
> -bool IsDebugAcceleratorsEnabled();
>
> +// Override the URL to which metrics logs are sent for debugging.
> +const char kOverrideMetricsUploadUrl[] = "override-metrics-upload-url";
>
>  }  // namespace switches
> -}  // namespace athena
> -
> -#endif  // ATHENA_COMMON_SWITCHES_H_
> Index: chromecast/common/chromecast_switches.h
> diff --git a/components/search/search_switches.h b/chromecast/common/
> chromecast_switches.h
> similarity index 53%
> copy from components/search/search_switches.h
> copy to chromecast/common/chromecast_switches.h
> index 48b50c142af3b9c645eed596ecfaa820c08c48bb..
> 73b8a67eb631007f574ff754b63ea1eee27d851b 100644
> --- a/components/search/search_switches.h
> +++ b/chromecast/common/chromecast_switches.h
> @@ -2,17 +2,16 @@
>  // Use of this source code is governed by a BSD-style license that can be
>  // found in the LICENSE file.
>
> -#ifndef COMPONENTS_SEARCH_SEARCH_SWITCHES_H_
> -#define COMPONENTS_SEARCH_SEARCH_SWITCHES_H_
> +#ifndef CHROMECAST_COMMON_CHROMECAST_SWITCHES_H_
> +#define CHROMECAST_COMMON_CHROMECAST_SWITCHES_H_
>
>  #include "build/build_config.h"
>
>  namespace switches {
>
> -#if defined(OS_ANDROID)
> -extern const char kEnableEmbeddedSearchAPI[];
> -#endif
> +// Metrics switches
> +extern const char kOverrideMetricsUploadUrl[];
>
>  }  // namespace switches
>
> -#endif  // COMPONENTS_SEARCH_SEARCH_SWITCHES_H_
> +#endif  // CHROMECAST_COMMON_CHROMECAST_SWITCHES_H_
> Index: chromecast/metrics/cast_metrics_service_client.cc
> diff --git a/chromecast/metrics/cast_metrics_service_client.cc
> b/chromecast/metrics/cast_metrics_service_client.cc
> index 80e80eaab1182ec47ac7bb8c63f643239507728d..
> 7ad0204ae8e16ad817c3cee35c6116f6bd32e94d 100644
> --- a/chromecast/metrics/cast_metrics_service_client.cc
> +++ b/chromecast/metrics/cast_metrics_service_client.cc
> @@ -4,8 +4,10 @@
>
>  #include "chromecast/metrics/cast_metrics_service_client.h"
>
> +#include "base/command_line.h"
>  #include "base/i18n/rtl.h"
>  #include "chromecast/common/chromecast_config.h"
> +#include "chromecast/common/chromecast_switches.h"
>  #include "chromecast/metrics/platform_metrics_providers.h"
>  #include "components/metrics/metrics_provider.h"
>  #include "components/metrics/metrics_service.h"
> @@ -69,10 +71,18 @@ CastMetricsServiceClient::CreateUploader(
>      const std::string& server_url,
>      const std::string& mime_type,
>      const base::Callback<void(int)>& on_upload_complete) {
> +  std::string uma_server_url(server_url);
>
> +  if (base::CommandLine::ForCurrentProcess()->
> +      HasSwitch(switches::kOverrideMetricsUploadUrl)) {
> +    uma_server_url.assign(
> +        base::CommandLine::ForCurrentProcess()->
> +            GetSwitchValueASCII(switches::kOverrideMetricsUploadUrl));
> +  }
> +  DCHECK(!uma_server_url.empty());
>    return scoped_ptr< ::metrics::MetricsLogUploader>(
>        new ::metrics::NetMetricsLogUploader(
>            request_context_,
> -          server_url,
> +          uma_server_url,
>            mime_type,
>            on_upload_complete));
>  }
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2014-08-29 23:51:41]

lgtm

https://codereview.chromium.org/506083003/diff/40001/chromecast/metrics/cast_...
File chromecast/metrics/cast_metrics_service_client.cc (right):

https://codereview.chromium.org/506083003/diff/40001/chromecast/metrics/cast_...
chromecast/metrics/cast_metrics_service_client.cc:75: if
(base::CommandLine::ForCurrentProcess()->
Nit: Make a local var for this to avoid calling it twice.

[byungchul, 2014-08-30 00:04:56]

lgtm

[gunsch, 2014-08-30 00:30:23]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-08-30 00:31:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gunsch@chromium.org/506083003/60001

[commit-bot: I haz the power, 2014-08-30 00:43:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/48421)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/53751)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-30 00:47:03]

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

[commit-bot: I haz the power, 2014-08-30 00:47:04]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[gunsch, 2014-08-30 15:19:55]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-08-30 15:20:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gunsch@chromium.org/506083003/80001

[commit-bot: I haz the power, 2014-08-30 16:15:29]

Committed patchset #5 (id:80001) as c986cebc62b254c0d86f35c57ab400c780a8060c

[commit-bot: I haz the power, 2014-09-10 03:14:19]

Patchset 5 (id:??) landed as
https://crrev.com/db879ef83d0b84eeb4d4bc6a77249ace1b2b13ba
Cr-Commit-Position: refs/heads/master@{#292791}