Allow use of metrics url override outside of Chromecast

Allow use of metrics url override outside of Chromecast

This commit adds a command line flag kOverrideMetricsUploadUrl to allow the
metrics endpoint url to be changed from the default kServerUrl value.
Currently only Chromecast is able to utilize this flag.

It adds a test to ensure that the override url is correctly called when
specified.

This commit also updates the AUTHORS file to wildcard include Amazon
contributors.

BUG=451177

[davileen, 2015-01-22 22:09:11]

davileen@amazon.com changed reviewers:
+ asvitkine@chromium.org, damienv@chromium.org, jochen@chromium.org

[davileen, 2015-01-22 22:09:12]

Hi all,

PTAL at this proposed fix for
https://code.google.com/p/chromium/issues/detail?id=451177

asvitkine: changes to components/metrics (and also the gyp file)
damienv: changes to chromecast
jochen: single line change to content to remove unused header

Thanks,
David

[damienv1, 2015-01-22 22:20:05]

damienv@chromium.org changed reviewers:
+ lcwu@chromium.org

[gunsch, 2015-01-22 22:38:46]

gunsch@chromium.org changed reviewers:
- damienv@chromium.org

[gunsch, 2015-01-22 22:39:09]

-damienv@ (already have lcwu1)
+gfhuang to CC

[Alexei Svitkine (slow), 2015-01-23 19:05:42]

We didn't want to make this at the component level, because this will make it
easier for bad actors to disable UMA for desktop Chrome.

Also, I'm curious about the motivation of this change. Presumably, you're
building your own browser based on Chromium source code. If so, it seems strange
that you'd want your metrics reporting system to be set by a command-line flag,
rather than done by the code directly.

[davileen, 2015-01-23 20:13:11]

On 2015/01/23 19:05:42, Alexei Svitkine wrote:
> We didn't want to make this at the component level, because this will make it
> easier for bad actors to disable UMA for desktop Chrome.
> 
> Also, I'm curious about the motivation of this change. Presumably, you're
> building your own browser based on Chromium source code. If so, it seems
strange
> that you'd want your metrics reporting system to be set by a command-line
flag,
> rather than done by the code directly.

Hi Alexei,

Thanks for the insight! We do want to collect metrics on our browser
using our own endpoint, though your concern makes sense. Perhaps
we can find another way instead of a command line argument? For this
CL do you think it is worth putting it behind a DEBUG compile flag? 
It certainly helped me being able to point it at different localhost endpoints.

Do you have any thoughts on making this configurable, perhaps
at compile time? We could potentially maintain a patch internally
that has the necessary change, but of course that would be our
least preferred option.

One thing we were considering was extracting URL constants like
kServerUrl into url_constants.cc files, which can be replaced trivially 
via GYP. There already seems to be somewhat of a
precedent in the codebase for it. It would be a large refactor,
but we would be willing to do the work. Any thoughts?

Thanks,
David.

[Alexei Svitkine (slow), 2015-01-23 22:49:33]

You probably will have to get more buy-in than just me, but having
url_constants.cc sounds good to me.

However, to make that work, you will need to modify the various components
that have hardcoded URLs to accept them as parameters. As url_constants.cc
will live in chrome/ while components/ can't depend on chrome/. So you'd
need to add constructor params or client interfaces to provide these URLs
to the components from the chrome/ layer (which can pass in the url
constants) - so it would be an involved undertaking. But it seems like a
reasonable one to me. If the intention for components/ to be re-usable,
then it makes sense for their endpoint URLs to be configurable.

On Fri, Jan 23, 2015 at 3:13 PM, <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2015/01/23 19:05:42, Alexei Svitkine wrote:
>
>> We didn't want to make this at the component level, because this will
>> make it
>> easier for bad actors to disable UMA for desktop Chrome.
>>
>
>  Also, I'm curious about the motivation of this change. Presumably, you're
>> building your own browser based on Chromium source code. If so, it seems
>>
> strange
>
>> that you'd want your metrics reporting system to be set by a command-line
>>
> flag,
>
>> rather than done by the code directly.
>>
>
> Hi Alexei,
>
> Thanks for the insight! We do want to collect metrics on our browser
> using our own endpoint, though your concern makes sense. Perhaps
> we can find another way instead of a command line argument? For this
> CL do you think it is worth putting it behind a DEBUG compile flag?
> It certainly helped me being able to point it at different localhost
> endpoints.
>
> Do you have any thoughts on making this configurable, perhaps
> at compile time? We could potentially maintain a patch internally
> that has the necessary change, but of course that would be our
> least preferred option.
>
> One thing we were considering was extracting URL constants like
> kServerUrl into url_constants.cc files, which can be replaced trivially
> via GYP. There already seems to be somewhat of a
> precedent in the codebase for it. It would be a large refactor,
> but we would be willing to do the work. Any thoughts?
>
> Thanks,
> David.
>
> https://codereview.chromium.org/866163002/
>

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

[davileen, 2015-01-26 20:22:54]

On 2015/01/23 22:49:33, Alexei Svitkine wrote:
> You probably will have to get more buy-in than just me, but having
> url_constants.cc sounds good to me.
> 
> However, to make that work, you will need to modify the various components
> that have hardcoded URLs to accept them as parameters. As url_constants.cc
> will live in chrome/ while components/ can't depend on chrome/. So you'd
> need to add constructor params or client interfaces to provide these URLs
> to the components from the chrome/ layer (which can pass in the url
> constants) - so it would be an involved undertaking. But it seems like a
> reasonable one to me. If the intention for components/ to be re-usable,
> then it makes sense for their endpoint URLs to be configurable.
> 
> On Fri, Jan 23, 2015 at 3:13 PM, <
> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> 
> > On 2015/01/23 19:05:42, Alexei Svitkine wrote:
> >
> >> We didn't want to make this at the component level, because this will
> >> make it
> >> easier for bad actors to disable UMA for desktop Chrome.
> >>
> >
> >  Also, I'm curious about the motivation of this change. Presumably, you're
> >> building your own browser based on Chromium source code. If so, it seems
> >>
> > strange
> >
> >> that you'd want your metrics reporting system to be set by a command-line
> >>
> > flag,
> >
> >> rather than done by the code directly.
> >>
> >
> > Hi Alexei,
> >
> > Thanks for the insight! We do want to collect metrics on our browser
> > using our own endpoint, though your concern makes sense. Perhaps
> > we can find another way instead of a command line argument? For this
> > CL do you think it is worth putting it behind a DEBUG compile flag?
> > It certainly helped me being able to point it at different localhost
> > endpoints.
> >
> > Do you have any thoughts on making this configurable, perhaps
> > at compile time? We could potentially maintain a patch internally
> > that has the necessary change, but of course that would be our
> > least preferred option.
> >
> > One thing we were considering was extracting URL constants like
> > kServerUrl into url_constants.cc files, which can be replaced trivially
> > via GYP. There already seems to be somewhat of a
> > precedent in the codebase for it. It would be a large refactor,
> > but we would be willing to do the work. Any thoughts?
> >
> > Thanks,
> > David.
> >
> > https://codereview.chromium.org/866163002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

That's good to hear Alexi. We'll have a CL for the url_constants.cc approach out
soon.

For this CL do you think there is value having the flag behind a DEBUG  so it
won't make it into
release builds?

[Alexei Svitkine (slow), 2015-01-26 20:38:04]

I'm not convinced there is a lot of value from it, since someone compiling
a debug build will already be able to easily change the code.

Also, it would create extra complexity because I think the chromecast code
wants to have this flag available even in release builds, which we don't
want in desktop Chrome.

On Mon, Jan 26, 2015 at 3:22 PM, <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2015/01/23 22:49:33, Alexei Svitkine wrote:
>
>> You probably will have to get more buy-in than just me, but having
>> url_constants.cc sounds good to me.
>>
>
>  However, to make that work, you will need to modify the various components
>> that have hardcoded URLs to accept them as parameters. As url_constants.cc
>> will live in chrome/ while components/ can't depend on chrome/. So you'd
>> need to add constructor params or client interfaces to provide these URLs
>> to the components from the chrome/ layer (which can pass in the url
>> constants) - so it would be an involved undertaking. But it seems like a
>> reasonable one to me. If the intention for components/ to be re-usable,
>> then it makes sense for their endpoint URLs to be configurable.
>>
>
>  On Fri, Jan 23, 2015 at 3:13 PM, <
>> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
>>
>
>  > On 2015/01/23 19:05:42, Alexei Svitkine wrote:
>> >
>> >> We didn't want to make this at the component level, because this will
>> >> make it
>> >> easier for bad actors to disable UMA for desktop Chrome.
>> >>
>> >
>> >  Also, I'm curious about the motivation of this change. Presumably,
>> you're
>> >> building your own browser based on Chromium source code. If so, it
>> seems
>> >>
>> > strange
>> >
>> >> that you'd want your metrics reporting system to be set by a
>> command-line
>> >>
>> > flag,
>> >
>> >> rather than done by the code directly.
>> >>
>> >
>> > Hi Alexei,
>> >
>> > Thanks for the insight! We do want to collect metrics on our browser
>> > using our own endpoint, though your concern makes sense. Perhaps
>> > we can find another way instead of a command line argument? For this
>> > CL do you think it is worth putting it behind a DEBUG compile flag?
>> > It certainly helped me being able to point it at different localhost
>> > endpoints.
>> >
>> > Do you have any thoughts on making this configurable, perhaps
>> > at compile time? We could potentially maintain a patch internally
>> > that has the necessary change, but of course that would be our
>> > least preferred option.
>> >
>> > One thing we were considering was extracting URL constants like
>> > kServerUrl into url_constants.cc files, which can be replaced trivially
>> > via GYP. There already seems to be somewhat of a
>> > precedent in the codebase for it. It would be a large refactor,
>> > but we would be willing to do the work. Any thoughts?
>> >
>> > Thanks,
>> > David.
>> >
>> > https://codereview.chromium.org/866163002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> That's good to hear Alexi. We'll have a CL for the url_constants.cc
> approach out
> soon.
>
> For this CL do you think there is value having the flag behind a DEBUG  so
> it
> won't make it into
> release builds?
>
> https://codereview.chromium.org/866163002/
>

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

[davileen, 2015-01-26 21:37:08]

On 2015/01/26 20:38:04, Alexei Svitkine wrote:
> I'm not convinced there is a lot of value from it, since someone compiling
> a debug build will already be able to easily change the code.
> 
> Also, it would create extra complexity because I think the chromecast code
> wants to have this flag available even in release builds, which we don't
> want in desktop Chrome.
> 
> On Mon, Jan 26, 2015 at 3:22 PM, <
> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> 
> > On 2015/01/23 22:49:33, Alexei Svitkine wrote:
> >
> >> You probably will have to get more buy-in than just me, but having
> >> url_constants.cc sounds good to me.
> >>
> >
> >  However, to make that work, you will need to modify the various components
> >> that have hardcoded URLs to accept them as parameters. As url_constants.cc
> >> will live in chrome/ while components/ can't depend on chrome/. So you'd
> >> need to add constructor params or client interfaces to provide these URLs
> >> to the components from the chrome/ layer (which can pass in the url
> >> constants) - so it would be an involved undertaking. But it seems like a
> >> reasonable one to me. If the intention for components/ to be re-usable,
> >> then it makes sense for their endpoint URLs to be configurable.
> >>
> >
> >  On Fri, Jan 23, 2015 at 3:13 PM, <
> >> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> >>
> >
> >  > On 2015/01/23 19:05:42, Alexei Svitkine wrote:
> >> >
> >> >> We didn't want to make this at the component level, because this will
> >> >> make it
> >> >> easier for bad actors to disable UMA for desktop Chrome.
> >> >>
> >> >
> >> >  Also, I'm curious about the motivation of this change. Presumably,
> >> you're
> >> >> building your own browser based on Chromium source code. If so, it
> >> seems
> >> >>
> >> > strange
> >> >
> >> >> that you'd want your metrics reporting system to be set by a
> >> command-line
> >> >>
> >> > flag,
> >> >
> >> >> rather than done by the code directly.
> >> >>
> >> >
> >> > Hi Alexei,
> >> >
> >> > Thanks for the insight! We do want to collect metrics on our browser
> >> > using our own endpoint, though your concern makes sense. Perhaps
> >> > we can find another way instead of a command line argument? For this
> >> > CL do you think it is worth putting it behind a DEBUG compile flag?
> >> > It certainly helped me being able to point it at different localhost
> >> > endpoints.
> >> >
> >> > Do you have any thoughts on making this configurable, perhaps
> >> > at compile time? We could potentially maintain a patch internally
> >> > that has the necessary change, but of course that would be our
> >> > least preferred option.
> >> >
> >> > One thing we were considering was extracting URL constants like
> >> > kServerUrl into url_constants.cc files, which can be replaced trivially
> >> > via GYP. There already seems to be somewhat of a
> >> > precedent in the codebase for it. It would be a large refactor,
> >> > but we would be willing to do the work. Any thoughts?
> >> >
> >> > Thanks,
> >> > David.
> >> >
> >> > https://codereview.chromium.org/866163002/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > That's good to hear Alexi. We'll have a CL for the url_constants.cc
> > approach out
> > soon.
> >
> > For this CL do you think there is value having the flag behind a DEBUG  so
> > it
> > won't make it into
> > release builds?
> >
> > https://codereview.chromium.org/866163002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Thanks Alexi.

The chromecast usage of this flag really threw me but I agree with your
responses. I'll close this CL.

Do you have any suggestions for the future on how I could get valuable feedback
like this sooner rather than later?

David.

[Alexei Svitkine (slow), 2015-01-26 21:54:15]

Getting feedback from the relevant code owners (i.e. people who will be
reviewing your CL and whose code you're changing) early is key. Maybe file
a crbug describing the use case and cc'ing those people as early as
possible.

On Mon, Jan 26, 2015 at 4:37 PM, <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

> On 2015/01/26 20:38:04, Alexei Svitkine wrote:
>
>> I'm not convinced there is a lot of value from it, since someone compiling
>> a debug build will already be able to easily change the code.
>>
>
>  Also, it would create extra complexity because I think the chromecast code
>> wants to have this flag available even in release builds, which we don't
>> want in desktop Chrome.
>>
>
>  On Mon, Jan 26, 2015 at 3:22 PM, <
>> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
>>
>
>  > On 2015/01/23 22:49:33, Alexei Svitkine wrote:
>> >
>> >> You probably will have to get more buy-in than just me, but having
>> >> url_constants.cc sounds good to me.
>> >>
>> >
>> >  However, to make that work, you will need to modify the various
>> components
>> >> that have hardcoded URLs to accept them as parameters. As
>> url_constants.cc
>> >> will live in chrome/ while components/ can't depend on chrome/. So
>> you'd
>> >> need to add constructor params or client interfaces to provide these
>> URLs
>> >> to the components from the chrome/ layer (which can pass in the url
>> >> constants) - so it would be an involved undertaking. But it seems like
>> a
>> >> reasonable one to me. If the intention for components/ to be re-usable,
>> >> then it makes sense for their endpoint URLs to be configurable.
>> >>
>> >
>> >  On Fri, Jan 23, 2015 at 3:13 PM, <
>> >> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
>> >>
>> >
>> >  > On 2015/01/23 19:05:42, Alexei Svitkine wrote:
>> >> >
>> >> >> We didn't want to make this at the component level, because this
>> will
>> >> >> make it
>> >> >> easier for bad actors to disable UMA for desktop Chrome.
>> >> >>
>> >> >
>> >> >  Also, I'm curious about the motivation of this change. Presumably,
>> >> you're
>> >> >> building your own browser based on Chromium source code. If so, it
>> >> seems
>> >> >>
>> >> > strange
>> >> >
>> >> >> that you'd want your metrics reporting system to be set by a
>> >> command-line
>> >> >>
>> >> > flag,
>> >> >
>> >> >> rather than done by the code directly.
>> >> >>
>> >> >
>> >> > Hi Alexei,
>> >> >
>> >> > Thanks for the insight! We do want to collect metrics on our browser
>> >> > using our own endpoint, though your concern makes sense. Perhaps
>> >> > we can find another way instead of a command line argument? For this
>> >> > CL do you think it is worth putting it behind a DEBUG compile flag?
>> >> > It certainly helped me being able to point it at different localhost
>> >> > endpoints.
>> >> >
>> >> > Do you have any thoughts on making this configurable, perhaps
>> >> > at compile time? We could potentially maintain a patch internally
>> >> > that has the necessary change, but of course that would be our
>> >> > least preferred option.
>> >> >
>> >> > One thing we were considering was extracting URL constants like
>> >> > kServerUrl into url_constants.cc files, which can be replaced
>> trivially
>> >> > via GYP. There already seems to be somewhat of a
>> >> > precedent in the codebase for it. It would be a large refactor,
>> >> > but we would be willing to do the work. Any thoughts?
>> >> >
>> >> > Thanks,
>> >> > David.
>> >> >
>> >> > https://codereview.chromium.org/866163002/
>> >> >
>> >>
>> >
>> >  To unsubscribe from this group and stop receiving emails from it, send
>> an
>> >>
>> > email
>> >
>> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
>> >>
>> >
>> > That's good to hear Alexi. We'll have a CL for the url_constants.cc
>> > approach out
>> > soon.
>> >
>> > For this CL do you think there is value having the flag behind a DEBUG
>> so
>> > it
>> > won't make it into
>> > release builds?
>> >
>> > https://codereview.chromium.org/866163002/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> Thanks Alexi.
>
> The chromecast usage of this flag really threw me but I agree with your
> responses. I'll close this CL.
>
> Do you have any suggestions for the future on how I could get valuable
> feedback
> like this sooner rather than later?
>
> David.
>
> https://codereview.chromium.org/866163002/
>

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

[ivansham, 2015-01-31 18:59:09]

On 2015/01/26 21:54:15, Alexei Svitkine wrote:
> Getting feedback from the relevant code owners (i.e. people who will be
> reviewing your CL and whose code you're changing) early is key. Maybe file
> a crbug describing the use case and cc'ing those people as early as
> possible.
> 
> On Mon, Jan 26, 2015 at 4:37 PM, <
> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> 
> > On 2015/01/26 20:38:04, Alexei Svitkine wrote:
> >
> >> I'm not convinced there is a lot of value from it, since someone compiling
> >> a debug build will already be able to easily change the code.
> >>
> >
> >  Also, it would create extra complexity because I think the chromecast code
> >> wants to have this flag available even in release builds, which we don't
> >> want in desktop Chrome.
> >>
> >
> >  On Mon, Jan 26, 2015 at 3:22 PM, <
> >> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> >>
> >
> >  > On 2015/01/23 22:49:33, Alexei Svitkine wrote:
> >> >
> >> >> You probably will have to get more buy-in than just me, but having
> >> >> url_constants.cc sounds good to me.
> >> >>
> >> >
> >> >  However, to make that work, you will need to modify the various
> >> components
> >> >> that have hardcoded URLs to accept them as parameters. As
> >> url_constants.cc
> >> >> will live in chrome/ while components/ can't depend on chrome/. So
> >> you'd
> >> >> need to add constructor params or client interfaces to provide these
> >> URLs
> >> >> to the components from the chrome/ layer (which can pass in the url
> >> >> constants) - so it would be an involved undertaking. But it seems like
> >> a
> >> >> reasonable one to me. If the intention for components/ to be re-usable,
> >> >> then it makes sense for their endpoint URLs to be configurable.
> >> >>
> >> >
> >> >  On Fri, Jan 23, 2015 at 3:13 PM, <
> >> >> mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> >> >>
> >> >
> >> >  > On 2015/01/23 19:05:42, Alexei Svitkine wrote:
> >> >> >
> >> >> >> We didn't want to make this at the component level, because this
> >> will
> >> >> >> make it
> >> >> >> easier for bad actors to disable UMA for desktop Chrome.
> >> >> >>
> >> >> >
> >> >> >  Also, I'm curious about the motivation of this change. Presumably,
> >> >> you're
> >> >> >> building your own browser based on Chromium source code. If so, it
> >> >> seems
> >> >> >>
> >> >> > strange
> >> >> >
> >> >> >> that you'd want your metrics reporting system to be set by a
> >> >> command-line
> >> >> >>
> >> >> > flag,
> >> >> >
> >> >> >> rather than done by the code directly.
> >> >> >>
> >> >> >
> >> >> > Hi Alexei,
> >> >> >
> >> >> > Thanks for the insight! We do want to collect metrics on our browser
> >> >> > using our own endpoint, though your concern makes sense. Perhaps
> >> >> > we can find another way instead of a command line argument? For this
> >> >> > CL do you think it is worth putting it behind a DEBUG compile flag?
> >> >> > It certainly helped me being able to point it at different localhost
> >> >> > endpoints.
> >> >> >
> >> >> > Do you have any thoughts on making this configurable, perhaps
> >> >> > at compile time? We could potentially maintain a patch internally
> >> >> > that has the necessary change, but of course that would be our
> >> >> > least preferred option.
> >> >> >
> >> >> > One thing we were considering was extracting URL constants like
> >> >> > kServerUrl into url_constants.cc files, which can be replaced
> >> trivially
> >> >> > via GYP. There already seems to be somewhat of a
> >> >> > precedent in the codebase for it. It would be a large refactor,
> >> >> > but we would be willing to do the work. Any thoughts?
> >> >> >
> >> >> > Thanks,
> >> >> > David.
> >> >> >
> >> >> > https://codereview.chromium.org/866163002/
> >> >> >
> >> >>
> >> >
> >> >  To unsubscribe from this group and stop receiving emails from it, send
> >> an
> >> >>
> >> > email
> >> >
> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >> >>
> >> >
> >> > That's good to hear Alexi. We'll have a CL for the url_constants.cc
> >> > approach out
> >> > soon.
> >> >
> >> > For this CL do you think there is value having the flag behind a DEBUG
> >> so
> >> > it
> >> > won't make it into
> >> > release builds?
> >> >
> >> > https://codereview.chromium.org/866163002/
> >> >
> >>
> >
> >  To unsubscribe from this group and stop receiving emails from it, send an
> >>
> > email
> >
> >> to mailto:chromium-reviews+unsubscribe@chromium.org.
> >>
> >
> > Thanks Alexi.
> >
> > The chromecast usage of this flag really threw me but I agree with your
> > responses. I'll close this CL.
> >
> > Do you have any suggestions for the future on how I could get valuable
> > feedback
> > like this sooner rather than later?
> >
> > David.
> >
> > https://codereview.chromium.org/866163002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Cr for the proposed url constant change
https://codereview.chromium.org/886173002/