Allow the X-Chrome-Variations header to transmit to all ports.

Allow the X-Chrome-Variations header to transmit to all ports.

This is to make internal testing simpler.

BUG=145886
TEST=Navigate to a *.google.<TLD> URL with a port other than 80 (or 443 on https). Ensure that the X-Chrome-Variations header is transmitted as part of that request.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155095

[SteveT, 2012-08-31 01:31:25]

Hi Ilya - mind taking a look at this?

I still have to manually test this to make sure it works... Will hopefully do
that tomorrow when I have connection I can RD with.

[Ilya Sherman, 2012-08-31 03:05:41]

Please add unit test coverage for this change as well as manually testing it. 
Otherwise looks good, modulo some small tweaks.

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
File chrome/browser/google/google_util.h (right):

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:62: // ports (80 for http, 443 for https),
or if it allows all port numbers.
This isn't quite right -- http://www.google.com:80/ will currently fail the
IsGoogleDomainUrl check, since the code rejects any explicit ports, even if
they're the default ones.

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:65: ALLOW_STANDARD_PORTS,
nit: Perhaps these should be ALLOW_EXPLICIT_PORTS and DISALLOW_EXPLICIT_PORTS?

[SteveT, 2012-08-31 13:29:31]

Some responses inline (no code changes yet)... PTAL.

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
File chrome/browser/google/google_util.h (right):

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:62: // ports (80 for http, 443 for https),
or if it allows all port numbers.
So the GURL code actually parses out port 80 on http URLs, and port 443 on https
URLs. That is to say, even if they're listed explicitly, they're treated as not
being there (the GURL::port() accessor returns an empty string).

So we're actually good to go with http://www.google.com:80/ (see tests... one of
the _old_ test cases tested this)

Does that seem OK? Perhaps I should change the comment in IsGoogleDomainUrl to
be more specific about this?

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:65: ALLOW_STANDARD_PORTS,
See comments above... they sort of explain why I used ALLOW_ALL_PORTS and
ALLOW_STANDARD_PORTS instead of the word EXPLICIT.

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:71: // disallow explicit ports.
I guess we could change this comment up if the word explicit is misleading, due
to how GURL handles port 80 and 443.

[Ilya Sherman, 2012-08-31 19:26:52]

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
File chrome/browser/google/google_util.h (right):

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:62: // ports (80 for http, 443 for https),
or if it allows all port numbers.
On 2012/08/31 13:29:31, SteveT wrote:
> So the GURL code actually parses out port 80 on http URLs, and port 443 on
https
> URLs. That is to say, even if they're listed explicitly, they're treated as
not
> being there (the GURL::port() accessor returns an empty string).
> 
> So we're actually good to go with http://www.google.com:80/ (see tests... one
of
> the _old_ test cases tested this)

Ah, ok, lovely :)

> Does that seem OK? Perhaps I should change the comment in IsGoogleDomainUrl to
> be more specific about this?

Yeah, I think we should change the comment not to talk about explicit ports
then.

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:65: ALLOW_STANDARD_PORTS,
On 2012/08/31 13:29:31, SteveT wrote:
> See comments above... they sort of explain why I used ALLOW_ALL_PORTS and
> ALLOW_STANDARD_PORTS instead of the word EXPLICIT.

Ok.  I still think it's useful to frame this as allowing or disallowing a
potentially dangerous permission, so perhaps ALLOW_NON_STANDARD_PORTS and
DISALLOW_NON_STANDARD_PORTS?

[SteveT, 2012-08-31 21:58:16]

Back to you. Thanks for the comments!

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
File chrome/browser/google/google_util.h (right):

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:62: // ports (80 for http, 443 for https),
or if it allows all port numbers.
Sounds good!

https://chromiumcodereview.appspot.com/10908028/diff/7001/chrome/browser/goog...
chrome/browser/google/google_util.h:65: ALLOW_STANDARD_PORTS,
On 2012/08/31 19:26:52, Ilya Sherman wrote:
> On 2012/08/31 13:29:31, SteveT wrote:
> > See comments above... they sort of explain why I used ALLOW_ALL_PORTS and
> > ALLOW_STANDARD_PORTS instead of the word EXPLICIT.
> 
> Ok.  I still think it's useful to frame this as allowing or disallowing a
> potentially dangerous permission, so perhaps ALLOW_NON_STANDARD_PORTS and
> DISALLOW_NON_STANDARD_PORTS?

Done.

[Ilya Sherman, 2012-08-31 22:18:48]

LGTM, thanks :)

[commit-bot: I haz the power, 2012-09-01 18:43:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10908028/14001

[commit-bot: I haz the power, 2012-09-01 18:43:51]

Failed to apply patch for chrome/browser/google/google_util.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/google/google_util.cc
Hunk #1 FAILED at 198.
Hunk #2 succeeded at 228 (offset 14 lines).
Hunk #3 succeeded at 237 (offset 14 lines).
Hunk #4 succeeded at 254 (offset 14 lines).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/google/google_util.cc.rej

[commit-bot: I haz the power, 2012-09-01 19:08:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10908028/7

[commit-bot: I haz the power, 2012-09-01 19:08:46]

Presubmit check for 10908028-7 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[SteveT, 2012-09-01 19:12:07]

Hey Nico - mind taking a look for chrome/OWNERS approval when you have the
chance? Thanks!!

[Nico, 2012-09-04 15:34:03]

I read the linked bug, web results for "X-Chrome-Variations", the original bug
where that was added, and the linked internal buganizer bug. I don't understand
what this header is for. Sending it at all sounds like a bad idea to me.

[SteveT, 2012-09-04 15:48:08]

Sorry - should have provided you with more context about this header.

This is used to transmit the IDs of running experiments of the local Chrome
client to Google services (such as but not limited to Search) to facilitate
server-side analysis of these experiments.

We've gone through privacy review for the original addition of this header, and
also had this change approved by Dominic from the privacy team as well. I can
give you more background on the privacy implications if you're interested.

[Nico, 2012-09-04 15:58:12]

On Tue, Sep 4, 2012 at 8:48 AM, <stevet@chromium.org> wrote:

> Sorry - should have provided you with more context about this header.
>
> This is used to transmit the IDs of running experiments of the local Chrome
> client to Google services (such as but not limited to Search) to facilitate
> server-side analysis of these experiments.
>

Can't you send some kind of ping with the experiment IDs every now and then
in a dedicated message? Adding a potentially unbounded header to every
single request to google.com seems pretty heavy.


>
> We've gone through privacy review for the original addition of this
> header, and
> also had this change approved by Dominic from the privacy team as well. I
> can
> give you more background on the privacy implications if you're interested.
>
>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>

[Steve, 2012-09-04 16:22:03]

The reason we attach this to each request is because we need a close
correlation between the Variations seen on the Chrome client and the
requests they make to the server. For example, we may want to know how
changes to the Omnibox might affect their interaction with the search
results page.

It's true that the header is potentially unbounded - we keep a close eye on
the experiments that use this header (not all FieldTrials send their IDs
up), and will ensure it doesn't get unruly. You raise an excellent point,
though, and I will chat with the Search team to see if we can set some
server side monitoring up to ensure that this header never gets large
enough to significanty affect latency.

Happy to discuss this more offline if you need deeper details.


On Tue, Sep 4, 2012 at 11:58 AM, Nico Weber <thakis@chromium.org> wrote:

> On Tue, Sep 4, 2012 at 8:48 AM, <stevet@chromium.org> wrote:
>
>> Sorry - should have provided you with more context about this header.
>>
>> This is used to transmit the IDs of running experiments of the local
>> Chrome
>> client to Google services (such as but not limited to Search) to
>> facilitate
>> server-side analysis of these experiments.
>>
>
> Can't you send some kind of ping with the experiment IDs every now and
> then in a dedicated message? Adding a potentially unbounded header to every
> single request to google.com seems pretty heavy.
>
>
>>
>> We've gone through privacy review for the original addition of this
>> header, and
>> also had this change approved by Dominic from the privacy team as well. I
>> can
>> give you more background on the privacy implications if you're interested.
>>
>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>
>
>

[Nico, 2012-09-04 17:27:11]

google.com is more than just search. This is also sent for all xhrs etc.

Maybe I'm overly paranoid though, so I'm cc'ing a random net/ owner, and if he's
fine with this then so am I. willchan: For Finch experiment evaluation, folks
want to add a http header that sends a list of all active finch experiment ids
with every http request to google.com. To me, that sounds like a bad idea, but
I'm not an http wizard. You are, do you have an opinion on this?

[willchan no longer on Chromium, 2012-09-04 22:13:50]

Nico, that's a good question, and I realize now that I didn't review this
as closely the first time this X-Chrome-Variations came up. For
https://requests, this is not an issue since we use SPDY, which gives
us header
compression. Since we're sending the same value for this header all the
time, this will compress nicely. For http://, it's a bigger concern. I
should also note that this probably isn't too big of a deal, as long as the
header value stays small, because we're *usually* not upstream
bottlenecked, although that can be more the case for mobile ports of
Chrome. That said, we *do* see cases (cnn.com?) where large request headers
(like cookies) lead to significant performance costs. That said, Google
websites are generally pretty good about minimizing requests in the first
place, but it's good to try to minimize this as much as possible. So I
recommend trying to keep a cap on the number.


On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:

> google.com is more than just search. This is also sent for all xhrs etc.
>
> Maybe I'm overly paranoid though, so I'm cc'ing a random net/ owner, and
> if he's
> fine with this then so am I. willchan: For Finch experiment evaluation,
> folks
> want to add a http header that sends a list of all active finch experiment
> ids
> with every http request to google.com. To me, that sounds like a bad
> idea, but
> I'm not an http wizard. You are, do you have an opinion on this?
>
>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>

[Steve, 2012-09-05 14:43:29]

What do you prefer that we do here? Like I said in an earlier response, I'm
happy to chat with the GWS folks and see if we can put server-side measures
of the header in place.

As for the client side, do we want to stick in a DCHECK that caps the
X-Chrome-Variations string to some byte-length?


On Tue, Sep 4, 2012 at 6:13 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Nico, that's a good question, and I realize now that I didn't review this
> as closely the first time this X-Chrome-Variations came up. For
https://requests, this is not an issue since we use SPDY, which gives us header
> compression. Since we're sending the same value for this header all the
> time, this will compress nicely. For http://, it's a bigger concern. I
> should also note that this probably isn't too big of a deal, as long as the
> header value stays small, because we're *usually* not upstream
> bottlenecked, although that can be more the case for mobile ports of
> Chrome. That said, we *do* see cases (cnn.com?) where large request
> headers (like cookies) lead to significant performance costs. That said,
> Google websites are generally pretty good about minimizing requests in the
> first place, but it's good to try to minimize this as much as possible. So
> I recommend trying to keep a cap on the number.
>
>
> On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:
>
>> google.com is more than just search. This is also sent for all xhrs etc.
>>
>> Maybe I'm overly paranoid though, so I'm cc'ing a random net/ owner, and
>> if he's
>> fine with this then so am I. willchan: For Finch experiment evaluation,
>> folks
>> want to add a http header that sends a list of all active finch
>> experiment ids
>> with every http request to google.com. To me, that sounds like a bad
>> idea, but
>> I'm not an http wizard. You are, do you have an opinion on this?
>>
>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>
>
>

[willchan no longer on Chromium, 2012-09-05 18:35:29]

You're just base64 encoding the proto buffer, right? Can we simply have a
hard limit on the number of ids? And if we get too many, we simply don't
send X-Chrome-Variations? I'd rather fail a bit harder so people notice
that we've stopped sending X-Chrome-Variations, which means something is
probably buggy. And yes, the GWS folks should also add monitoring.


On Wed, Sep 5, 2012 at 7:43 AM, Steve Truong <stevet@google.com> wrote:

> What do you prefer that we do here? Like I said in an earlier response,
> I'm happy to chat with the GWS folks and see if we can put server-side
> measures of the header in place.
>
> As for the client side, do we want to stick in a DCHECK that caps the
> X-Chrome-Variations string to some byte-length?
>
>
> On Tue, Sep 4, 2012 at 6:13 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
>
>> Nico, that's a good question, and I realize now that I didn't review this
>> as closely the first time this X-Chrome-Variations came up. For
https://requests, this is not an issue since we use SPDY, which gives us header
>> compression. Since we're sending the same value for this header all the
>> time, this will compress nicely. For http://, it's a bigger concern. I
>> should also note that this probably isn't too big of a deal, as long as the
>> header value stays small, because we're *usually* not upstream
>> bottlenecked, although that can be more the case for mobile ports of
>> Chrome. That said, we *do* see cases (cnn.com?) where large request
>> headers (like cookies) lead to significant performance costs. That said,
>> Google websites are generally pretty good about minimizing requests in the
>> first place, but it's good to try to minimize this as much as possible. So
>> I recommend trying to keep a cap on the number.
>>
>>
>> On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:
>>
>>> google.com is more than just search. This is also sent for all xhrs etc.
>>>
>>> Maybe I'm overly paranoid though, so I'm cc'ing a random net/ owner, and
>>> if he's
>>> fine with this then so am I. willchan: For Finch experiment evaluation,
>>> folks
>>> want to add a http header that sends a list of all active finch
>>> experiment ids
>>> with every http request to google.com. To me, that sounds like a bad
>>> idea, but
>>> I'm not an http wizard. You are, do you have an opinion on this?
>>>
>>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>>
>>
>>
>

[Steve, 2012-09-05 18:44:09]

On Wed, Sep 5, 2012 at 2:35 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> You're just base64 encoding the proto buffer, right?


That's right.


> Can we simply have a hard limit on the number of ids?


Sure, this sounds fine... Though what that hard limit might be is hard to
define? Currently, we send up about 8 IDs. We don't have plans to add many
more in the next release or so.


> And if we get too many, we simply don't send X-Chrome-Variations? I'd
> rather fail a bit harder so people notice that we've stopped sending
> X-Chrome-Variations, which means something is probably buggy.


My problem with this is that there isn't much of an explanation as to why
the X-Chrome-Variations header stops showing up (it looks like it could be
some other bug), where as a DCHECK can direct the developer to the actual
problem (too many IDs).

Though I guess if a developer writes a FieldTrial with a new Variation ID
and never visits a google.* property with that code... they wouldn't see
the DCHECK.


> And yes, the GWS folks should also add monitoring.


Happy to talk to the server folks about this.

As for the client side restriction described above... is this something we
can table for a separate CL?


>
>
> On Wed, Sep 5, 2012 at 7:43 AM, Steve Truong <stevet@google.com> wrote:
>
>> What do you prefer that we do here? Like I said in an earlier response,
>> I'm happy to chat with the GWS folks and see if we can put server-side
>> measures of the header in place.
>>
>> As for the client side, do we want to stick in a DCHECK that caps the
>> X-Chrome-Variations string to some byte-length?
>>
>>
>> On Tue, Sep 4, 2012 at 6:13 PM, William Chan (陈智昌) <willchan@chromium.org
>> > wrote:
>>
>>> Nico, that's a good question, and I realize now that I didn't review
>>> this as closely the first time this X-Chrome-Variations came up. For
>>> https:// requests, this is not an issue since we use SPDY, which gives
>>> us header compression. Since we're sending the same value for this header
>>> all the time, this will compress nicely. For http://, it's a bigger
>>> concern. I should also note that this probably isn't too big of a deal, as
>>> long as the header value stays small, because we're *usually* not upstream
>>> bottlenecked, although that can be more the case for mobile ports of
>>> Chrome. That said, we *do* see cases (cnn.com?) where large request
>>> headers (like cookies) lead to significant performance costs. That said,
>>> Google websites are generally pretty good about minimizing requests in the
>>> first place, but it's good to try to minimize this as much as possible. So
>>> I recommend trying to keep a cap on the number.
>>>
>>>
>>> On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:
>>>
>>>> google.com is more than just search. This is also sent for all xhrs
>>>> etc.
>>>>
>>>> Maybe I'm overly paranoid though, so I'm cc'ing a random net/ owner,
>>>> and if he's
>>>> fine with this then so am I. willchan: For Finch experiment evaluation,
>>>> folks
>>>> want to add a http header that sends a list of all active finch
>>>> experiment ids
>>>> with every http request to google.com. To me, that sounds like a bad
>>>> idea, but
>>>> I'm not an http wizard. You are, do you have an opinion on this?
>>>>
>>>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>>>
>>>
>>>
>>
>

[willchan no longer on Chromium, 2012-09-05 19:21:54]

On Wed, Sep 5, 2012 at 11:43 AM, Steve Truong <stevet@google.com> wrote:

>
>
>
> On Wed, Sep 5, 2012 at 2:35 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
>
>> You're just base64 encoding the proto buffer, right?
>
>
> That's right.
>
>
>> Can we simply have a hard limit on the number of ids?
>
>
> Sure, this sounds fine... Though what that hard limit might be is hard to
> define? Currently, we send up about 8 IDs. We don't have plans to add many
> more in the next release or so.
>

Let's cap at 20. That should be plenty. But GWS monitoring should keep it
lower than that.


>
>
>> And if we get too many, we simply don't send X-Chrome-Variations? I'd
>> rather fail a bit harder so people notice that we've stopped sending
>> X-Chrome-Variations, which means something is probably buggy.
>
>
> My problem with this is that there isn't much of an explanation as to why
> the X-Chrome-Variations header stops showing up (it looks like it could be
> some other bug), where as a DCHECK can direct the developer to the actual
> problem (too many IDs).
>
> Though I guess if a developer writes a FieldTrial with a new Variation ID
> and never visits a google.* property with that code... they wouldn't see
> the DCHECK.
>

Who controls these IDs? Are they baked into the Chrome binary? I thought
the server was controlling this? If it's baked into the Chrome binary, then
a DCHECK is indeed appropriate and my suggested limit of 20 should be
lower, like 10. If it's server controlled, then I don't believe in
DCHECK'ing on a limit that is server controlled.


>
>
>> And yes, the GWS folks should also add monitoring.
>
>
> Happy to talk to the server folks about this.
>
> As for the client side restriction described above... is this something we
> can table for a separate CL?
>

Sure.


>
>
>>
>>
>> On Wed, Sep 5, 2012 at 7:43 AM, Steve Truong <stevet@google.com> wrote:
>>
>>> What do you prefer that we do here? Like I said in an earlier response,
>>> I'm happy to chat with the GWS folks and see if we can put server-side
>>> measures of the header in place.
>>>
>>> As for the client side, do we want to stick in a DCHECK that caps the
>>> X-Chrome-Variations string to some byte-length?
>>>
>>>
>>> On Tue, Sep 4, 2012 at 6:13 PM, William Chan (陈智昌) <
>>> willchan@chromium.org> wrote:
>>>
>>>> Nico, that's a good question, and I realize now that I didn't review
>>>> this as closely the first time this X-Chrome-Variations came up. For
>>>> https:// requests, this is not an issue since we use SPDY, which gives
>>>> us header compression. Since we're sending the same value for this header
>>>> all the time, this will compress nicely. For http://, it's a bigger
>>>> concern. I should also note that this probably isn't too big of a deal, as
>>>> long as the header value stays small, because we're *usually* not upstream
>>>> bottlenecked, although that can be more the case for mobile ports of
>>>> Chrome. That said, we *do* see cases (cnn.com?) where large request
>>>> headers (like cookies) lead to significant performance costs. That said,
>>>> Google websites are generally pretty good about minimizing requests in the
>>>> first place, but it's good to try to minimize this as much as possible. So
>>>> I recommend trying to keep a cap on the number.
>>>>
>>>>
>>>> On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:
>>>>
>>>>> google.com is more than just search. This is also sent for all xhrs
>>>>> etc.
>>>>>
>>>>> Maybe I'm overly paranoid though, so I'm cc'ing a random net/ owner,
>>>>> and if he's
>>>>> fine with this then so am I. willchan: For Finch experiment
>>>>> evaluation, folks
>>>>> want to add a http header that sends a list of all active finch
>>>>> experiment ids
>>>>> with every http request to google.com. To me, that sounds like a bad
>>>>> idea, but
>>>>> I'm not an http wizard. You are, do you have an opinion on this?
>>>>>
>>>>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>>>>
>>>>
>>>>
>>>
>>
>

[Steve, 2012-09-05 19:41:03]

On Wed, Sep 5, 2012 at 3:21 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Sep 5, 2012 at 11:43 AM, Steve Truong <stevet@google.com> wrote:
>
>>
>>
>>
>> On Wed, Sep 5, 2012 at 2:35 PM, William Chan (陈智昌) <willchan@chromium.org
>> > wrote:
>>
>>> You're just base64 encoding the proto buffer, right?
>>
>>
>> That's right.
>>
>>
>>> Can we simply have a hard limit on the number of ids?
>>
>>
>> Sure, this sounds fine... Though what that hard limit might be is hard to
>> define? Currently, we send up about 8 IDs. We don't have plans to add many
>> more in the next release or so.
>>
>
> Let's cap at 20. That should be plenty. But GWS monitoring should keep it
> lower than that.
>

SG.


>
>
>>
>>
>>> And if we get too many, we simply don't send X-Chrome-Variations? I'd
>>> rather fail a bit harder so people notice that we've stopped sending
>>> X-Chrome-Variations, which means something is probably buggy.
>>
>>
>> My problem with this is that there isn't much of an explanation as to why
>> the X-Chrome-Variations header stops showing up (it looks like it could be
>> some other bug), where as a DCHECK can direct the developer to the actual
>> problem (too many IDs).
>>
>> Though I guess if a developer writes a FieldTrial with a new Variation ID
>> and never visits a google.* property with that code... they wouldn't see
>> the DCHECK.
>>
>
> Who controls these IDs? Are they baked into the Chrome binary? I thought
> the server was controlling this?
>

Both. Trials are allowed to hard code their IDs... or they can assign them
from the server. This means that the client code does not give the
definitive list of possible/active IDs.


> If it's baked into the Chrome binary, then a DCHECK is indeed appropriate
> and my suggested limit of 20 should be lower, like 10. If it's server
> controlled, then I don't believe in DCHECK'ing on a limit that is server
> controlled.
>

What if we DCHECK on 10, and stop transmitting altogether on 20?


>
>
>>
>>
>>> And yes, the GWS folks should also add monitoring.
>>
>>
>> Happy to talk to the server folks about this.
>>
>> As for the client side restriction described above... is this something
>> we can table for a separate CL?
>>
>
> Sure.
>

I can implement the above in a small follow-up patch to this one.


>
>
>>
>>
>>>
>>>
>>> On Wed, Sep 5, 2012 at 7:43 AM, Steve Truong <stevet@google.com> wrote:
>>>
>>>> What do you prefer that we do here? Like I said in an earlier response,
>>>> I'm happy to chat with the GWS folks and see if we can put server-side
>>>> measures of the header in place.
>>>>
>>>> As for the client side, do we want to stick in a DCHECK that caps the
>>>> X-Chrome-Variations string to some byte-length?
>>>>
>>>>
>>>> On Tue, Sep 4, 2012 at 6:13 PM, William Chan (陈智昌) <
>>>> willchan@chromium.org> wrote:
>>>>
>>>>> Nico, that's a good question, and I realize now that I didn't review
>>>>> this as closely the first time this X-Chrome-Variations came up. For
>>>>> https:// requests, this is not an issue since we use SPDY, which
>>>>> gives us header compression. Since we're sending the same value for this
>>>>> header all the time, this will compress nicely. For http://, it's a
>>>>> bigger concern. I should also note that this probably isn't too big of a
>>>>> deal, as long as the header value stays small, because we're *usually*
not
>>>>> upstream bottlenecked, although that can be more the case for mobile
ports
>>>>> of Chrome. That said, we *do* see cases (cnn.com?) where large
>>>>> request headers (like cookies) lead to significant performance costs.
That
>>>>> said, Google websites are generally pretty good about minimizing requests
>>>>> in the first place, but it's good to try to minimize this as much as
>>>>> possible. So I recommend trying to keep a cap on the number.
>>>>>
>>>>>
>>>>> On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:
>>>>>
>>>>>> google.com is more than just search. This is also sent for all xhrs
>>>>>> etc.
>>>>>>
>>>>>> Maybe I'm overly paranoid though, so I'm cc'ing a random net/ owner,
>>>>>> and if he's
>>>>>> fine with this then so am I. willchan: For Finch experiment
>>>>>> evaluation, folks
>>>>>> want to add a http header that sends a list of all active finch
>>>>>> experiment ids
>>>>>> with every http request to google.com. To me, that sounds like a bad
>>>>>> idea, but
>>>>>> I'm not an http wizard. You are, do you have an opinion on this?
>>>>>>
>>>>>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[willchan no longer on Chromium, 2012-09-05 20:17:34]

On Wed, Sep 5, 2012 at 12:40 PM, Steve Truong <stevet@google.com> wrote:

>
>
>
> On Wed, Sep 5, 2012 at 3:21 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
>
>> On Wed, Sep 5, 2012 at 11:43 AM, Steve Truong <stevet@google.com> wrote:
>>
>>>
>>>
>>>
>>> On Wed, Sep 5, 2012 at 2:35 PM, William Chan (陈智昌) <
>>> willchan@chromium.org> wrote:
>>>
>>>> You're just base64 encoding the proto buffer, right?
>>>
>>>
>>> That's right.
>>>
>>>
>>>> Can we simply have a hard limit on the number of ids?
>>>
>>>
>>> Sure, this sounds fine... Though what that hard limit might be is hard
>>> to define? Currently, we send up about 8 IDs. We don't have plans to add
>>> many more in the next release or so.
>>>
>>
>> Let's cap at 20. That should be plenty. But GWS monitoring should keep it
>> lower than that.
>>
>
> SG.
>
>
>>
>>
>>>
>>>
>>>> And if we get too many, we simply don't send X-Chrome-Variations? I'd
>>>> rather fail a bit harder so people notice that we've stopped sending
>>>> X-Chrome-Variations, which means something is probably buggy.
>>>
>>>
>>> My problem with this is that there isn't much of an explanation as to
>>> why the X-Chrome-Variations header stops showing up (it looks like it could
>>> be some other bug), where as a DCHECK can direct the developer to the
>>> actual problem (too many IDs).
>>>
>>> Though I guess if a developer writes a FieldTrial with a new Variation
>>> ID and never visits a google.* property with that code... they wouldn't see
>>> the DCHECK.
>>>
>>
>> Who controls these IDs? Are they baked into the Chrome binary? I thought
>> the server was controlling this?
>>
>
> Both. Trials are allowed to hard code their IDs... or they can assign them
> from the server. This means that the client code does not give the
> definitive list of possible/active IDs.
>
>
>> If it's baked into the Chrome binary, then a DCHECK is indeed appropriate
>> and my suggested limit of 20 should be lower, like 10. If it's server
>> controlled, then I don't believe in DCHECK'ing on a limit that is server
>> controlled.
>>
>
> What if we DCHECK on 10, and stop transmitting altogether on 20?
>

OK, just make sure that you don't end up DoS'ing all developer debug
builds. If GWS messes up and assigns too many IDs, we don't want all
developer debug builds to always crash (I find it very likely that we have
a google.XX request on startup).


>
>
>>
>>
>>>
>>>
>>>> And yes, the GWS folks should also add monitoring.
>>>
>>>
>>> Happy to talk to the server folks about this.
>>>
>>> As for the client side restriction described above... is this something
>>> we can table for a separate CL?
>>>
>>
>> Sure.
>>
>
> I can implement the above in a small follow-up patch to this one.
>
>
>>
>>
>>>
>>>
>>>>
>>>>
>>>> On Wed, Sep 5, 2012 at 7:43 AM, Steve Truong <stevet@google.com> wrote:
>>>>
>>>>> What do you prefer that we do here? Like I said in an earlier
>>>>> response, I'm happy to chat with the GWS folks and see if we can put
>>>>> server-side measures of the header in place.
>>>>>
>>>>> As for the client side, do we want to stick in a DCHECK that caps the
>>>>> X-Chrome-Variations string to some byte-length?
>>>>>
>>>>>
>>>>> On Tue, Sep 4, 2012 at 6:13 PM, William Chan (陈智昌) <
>>>>> willchan@chromium.org> wrote:
>>>>>
>>>>>> Nico, that's a good question, and I realize now that I didn't review
>>>>>> this as closely the first time this X-Chrome-Variations came up. For
>>>>>> https:// requests, this is not an issue since we use SPDY, which
>>>>>> gives us header compression. Since we're sending the same value for this
>>>>>> header all the time, this will compress nicely. For http://, it's a
>>>>>> bigger concern. I should also note that this probably isn't too big of a
>>>>>> deal, as long as the header value stays small, because we're *usually*
not
>>>>>> upstream bottlenecked, although that can be more the case for mobile
ports
>>>>>> of Chrome. That said, we *do* see cases (cnn.com?) where large
>>>>>> request headers (like cookies) lead to significant performance costs.
That
>>>>>> said, Google websites are generally pretty good about minimizing
requests
>>>>>> in the first place, but it's good to try to minimize this as much as
>>>>>> possible. So I recommend trying to keep a cap on the number.
>>>>>>
>>>>>>
>>>>>> On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:
>>>>>>
>>>>>>> google.com is more than just search. This is also sent for all xhrs
>>>>>>> etc.
>>>>>>>
>>>>>>> Maybe I'm overly paranoid though, so I'm cc'ing a random net/ owner,
>>>>>>> and if he's
>>>>>>> fine with this then so am I. willchan: For Finch experiment
>>>>>>> evaluation, folks
>>>>>>> want to add a http header that sends a list of all active finch
>>>>>>> experiment ids
>>>>>>> with every http request to google.com. To me, that sounds like a
>>>>>>> bad idea, but
>>>>>>> I'm not an http wizard. You are, do you have an opinion on this?
>>>>>>>
>>>>>>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[Steve, 2012-09-05 20:57:15]

On Wed, Sep 5, 2012 at 4:17 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Sep 5, 2012 at 12:40 PM, Steve Truong <stevet@google.com> wrote:
>
>>
>>
>>
>> On Wed, Sep 5, 2012 at 3:21 PM, William Chan (陈智昌) <willchan@chromium.org
>> > wrote:
>>
>>> On Wed, Sep 5, 2012 at 11:43 AM, Steve Truong <stevet@google.com> wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Wed, Sep 5, 2012 at 2:35 PM, William Chan (陈智昌) <
>>>> willchan@chromium.org> wrote:
>>>>
>>>>> You're just base64 encoding the proto buffer, right?
>>>>
>>>>
>>>> That's right.
>>>>
>>>>
>>>>> Can we simply have a hard limit on the number of ids?
>>>>
>>>>
>>>> Sure, this sounds fine... Though what that hard limit might be is hard
>>>> to define? Currently, we send up about 8 IDs. We don't have plans to add
>>>> many more in the next release or so.
>>>>
>>>
>>> Let's cap at 20. That should be plenty. But GWS monitoring should keep
>>> it lower than that.
>>>
>>
>> SG.
>>
>>
>>>
>>>
>>>>
>>>>
>>>>> And if we get too many, we simply don't send X-Chrome-Variations? I'd
>>>>> rather fail a bit harder so people notice that we've stopped sending
>>>>> X-Chrome-Variations, which means something is probably buggy.
>>>>
>>>>
>>>> My problem with this is that there isn't much of an explanation as to
>>>> why the X-Chrome-Variations header stops showing up (it looks like it
could
>>>> be some other bug), where as a DCHECK can direct the developer to the
>>>> actual problem (too many IDs).
>>>>
>>>> Though I guess if a developer writes a FieldTrial with a new Variation
>>>> ID and never visits a google.* property with that code... they wouldn't
see
>>>> the DCHECK.
>>>>
>>>
>>> Who controls these IDs? Are they baked into the Chrome binary? I thought
>>> the server was controlling this?
>>>
>>
>> Both. Trials are allowed to hard code their IDs... or they can assign
>> them from the server. This means that the client code does not give the
>> definitive list of possible/active IDs.
>>
>>
>>> If it's baked into the Chrome binary, then a DCHECK is indeed
>>> appropriate and my suggested limit of 20 should be lower, like 10. If it's
>>> server controlled, then I don't believe in DCHECK'ing on a limit that is
>>> server controlled.
>>>
>>
>> What if we DCHECK on 10, and stop transmitting altogether on 20?
>>
>
> OK, just make sure that you don't end up DoS'ing all developer debug
> builds. If GWS messes up and assigns too many IDs, we don't want all
> developer debug builds to always crash (I find it very likely that we have
> a google.XX request on startup).
>

Fair enough. Could yourself or Nico continue the review of this patch for
chrome/ OWNERS approval?


>
>
>>
>>
>>>
>>>
>>>>
>>>>
>>>>> And yes, the GWS folks should also add monitoring.
>>>>
>>>>
>>>> Happy to talk to the server folks about this.
>>>>
>>>> As for the client side restriction described above... is this something
>>>> we can table for a separate CL?
>>>>
>>>
>>> Sure.
>>>
>>
>> I can implement the above in a small follow-up patch to this one.
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 5, 2012 at 7:43 AM, Steve Truong <stevet@google.com>wrote:
>>>>>
>>>>>> What do you prefer that we do here? Like I said in an earlier
>>>>>> response, I'm happy to chat with the GWS folks and see if we can put
>>>>>> server-side measures of the header in place.
>>>>>>
>>>>>> As for the client side, do we want to stick in a DCHECK that caps the
>>>>>> X-Chrome-Variations string to some byte-length?
>>>>>>
>>>>>>
>>>>>> On Tue, Sep 4, 2012 at 6:13 PM, William Chan (陈智昌) <
>>>>>> willchan@chromium.org> wrote:
>>>>>>
>>>>>>> Nico, that's a good question, and I realize now that I didn't review
>>>>>>> this as closely the first time this X-Chrome-Variations came up. For
>>>>>>> https:// requests, this is not an issue since we use SPDY, which
>>>>>>> gives us header compression. Since we're sending the same value for
this
>>>>>>> header all the time, this will compress nicely. For http://, it's a
>>>>>>> bigger concern. I should also note that this probably isn't too big of
a
>>>>>>> deal, as long as the header value stays small, because we're *usually*
not
>>>>>>> upstream bottlenecked, although that can be more the case for mobile
ports
>>>>>>> of Chrome. That said, we *do* see cases (cnn.com?) where large
>>>>>>> request headers (like cookies) lead to significant performance costs.
That
>>>>>>> said, Google websites are generally pretty good about minimizing
requests
>>>>>>> in the first place, but it's good to try to minimize this as much as
>>>>>>> possible. So I recommend trying to keep a cap on the number.
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:
>>>>>>>
>>>>>>>> google.com is more than just search. This is also sent for all
>>>>>>>> xhrs etc.
>>>>>>>>
>>>>>>>> Maybe I'm overly paranoid though, so I'm cc'ing a random net/
>>>>>>>> owner, and if he's
>>>>>>>> fine with this then so am I. willchan: For Finch experiment
>>>>>>>> evaluation, folks
>>>>>>>> want to add a http header that sends a list of all active finch
>>>>>>>> experiment ids
>>>>>>>> with every http request to google.com. To me, that sounds like a
>>>>>>>> bad idea, but
>>>>>>>> I'm not an http wizard. You are, do you have an opinion on this?
>>>>>>>>
>>>>>>>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[willchan no longer on Chromium, 2012-09-05 20:59:08]

That'd be Nico.


On Wed, Sep 5, 2012 at 1:56 PM, Steve Truong <stevet@google.com> wrote:

>
>
>
> On Wed, Sep 5, 2012 at 4:17 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:
>
>> On Wed, Sep 5, 2012 at 12:40 PM, Steve Truong <stevet@google.com> wrote:
>>
>>>
>>>
>>>
>>> On Wed, Sep 5, 2012 at 3:21 PM, William Chan (陈智昌) <
>>> willchan@chromium.org> wrote:
>>>
>>>> On Wed, Sep 5, 2012 at 11:43 AM, Steve Truong <stevet@google.com>wrote:
>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 5, 2012 at 2:35 PM, William Chan (陈智昌) <
>>>>> willchan@chromium.org> wrote:
>>>>>
>>>>>> You're just base64 encoding the proto buffer, right?
>>>>>
>>>>>
>>>>> That's right.
>>>>>
>>>>>
>>>>>> Can we simply have a hard limit on the number of ids?
>>>>>
>>>>>
>>>>> Sure, this sounds fine... Though what that hard limit might be is hard
>>>>> to define? Currently, we send up about 8 IDs. We don't have plans to add
>>>>> many more in the next release or so.
>>>>>
>>>>
>>>> Let's cap at 20. That should be plenty. But GWS monitoring should keep
>>>> it lower than that.
>>>>
>>>
>>> SG.
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>> And if we get too many, we simply don't send X-Chrome-Variations? I'd
>>>>>> rather fail a bit harder so people notice that we've stopped sending
>>>>>> X-Chrome-Variations, which means something is probably buggy.
>>>>>
>>>>>
>>>>> My problem with this is that there isn't much of an explanation as to
>>>>> why the X-Chrome-Variations header stops showing up (it looks like it
could
>>>>> be some other bug), where as a DCHECK can direct the developer to the
>>>>> actual problem (too many IDs).
>>>>>
>>>>> Though I guess if a developer writes a FieldTrial with a new Variation
>>>>> ID and never visits a google.* property with that code... they wouldn't
see
>>>>> the DCHECK.
>>>>>
>>>>
>>>> Who controls these IDs? Are they baked into the Chrome binary? I
>>>> thought the server was controlling this?
>>>>
>>>
>>> Both. Trials are allowed to hard code their IDs... or they can assign
>>> them from the server. This means that the client code does not give the
>>> definitive list of possible/active IDs.
>>>
>>>
>>>> If it's baked into the Chrome binary, then a DCHECK is indeed
>>>> appropriate and my suggested limit of 20 should be lower, like 10. If it's
>>>> server controlled, then I don't believe in DCHECK'ing on a limit that is
>>>> server controlled.
>>>>
>>>
>>> What if we DCHECK on 10, and stop transmitting altogether on 20?
>>>
>>
>> OK, just make sure that you don't end up DoS'ing all developer debug
>> builds. If GWS messes up and assigns too many IDs, we don't want all
>> developer debug builds to always crash (I find it very likely that we have
>> a google.XX request on startup).
>>
>
> Fair enough. Could yourself or Nico continue the review of this patch for
> chrome/ OWNERS approval?
>
>
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>> And yes, the GWS folks should also add monitoring.
>>>>>
>>>>>
>>>>> Happy to talk to the server folks about this.
>>>>>
>>>>> As for the client side restriction described above... is this
>>>>> something we can table for a separate CL?
>>>>>
>>>>
>>>> Sure.
>>>>
>>>
>>> I can implement the above in a small follow-up patch to this one.
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Sep 5, 2012 at 7:43 AM, Steve Truong <stevet@google.com>wrote:
>>>>>>
>>>>>>> What do you prefer that we do here? Like I said in an earlier
>>>>>>> response, I'm happy to chat with the GWS folks and see if we can put
>>>>>>> server-side measures of the header in place.
>>>>>>>
>>>>>>> As for the client side, do we want to stick in a DCHECK that caps
>>>>>>> the X-Chrome-Variations string to some byte-length?
>>>>>>>
>>>>>>>
>>>>>>> On Tue, Sep 4, 2012 at 6:13 PM, William Chan (陈智昌) <
>>>>>>> willchan@chromium.org> wrote:
>>>>>>>
>>>>>>>> Nico, that's a good question, and I realize now that I didn't
>>>>>>>> review this as closely the first time this X-Chrome-Variations came up.
For
>>>>>>>> https:// requests, this is not an issue since we use SPDY, which
>>>>>>>> gives us header compression. Since we're sending the same value for
this
>>>>>>>> header all the time, this will compress nicely. For http://, it's
>>>>>>>> a bigger concern. I should also note that this probably isn't too big
of a
>>>>>>>> deal, as long as the header value stays small, because we're *usually*
not
>>>>>>>> upstream bottlenecked, although that can be more the case for mobile
ports
>>>>>>>> of Chrome. That said, we *do* see cases (cnn.com?) where large
>>>>>>>> request headers (like cookies) lead to significant performance costs.
That
>>>>>>>> said, Google websites are generally pretty good about minimizing
requests
>>>>>>>> in the first place, but it's good to try to minimize this as much as
>>>>>>>> possible. So I recommend trying to keep a cap on the number.
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Sep 4, 2012 at 10:27 AM, <thakis@chromium.org> wrote:
>>>>>>>>
>>>>>>>>> google.com is more than just search. This is also sent for all
>>>>>>>>> xhrs etc.
>>>>>>>>>
>>>>>>>>> Maybe I'm overly paranoid though, so I'm cc'ing a random net/
>>>>>>>>> owner, and if he's
>>>>>>>>> fine with this then so am I. willchan: For Finch experiment
>>>>>>>>> evaluation, folks
>>>>>>>>> want to add a http header that sends a list of all active finch
>>>>>>>>> experiment ids
>>>>>>>>> with every http request to google.com. To me, that sounds like a
>>>>>>>>> bad idea, but
>>>>>>>>> I'm not an http wizard. You are, do you have an opinion on this?
>>>>>>>>>
>>>>>>>>>
https://chromiumcodereview.**appspot.com/10908028/<https://chromiumcodereview...
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[Nico, 2012-09-05 21:24:21]

lgtm then

[SteveT, 2012-09-05 23:30:14]

Thanks!

Manual tests looking good. Committing shortly..

[commit-bot: I haz the power, 2012-09-05 23:30:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10908028/4005

[commit-bot: I haz the power, 2012-09-06 01:54:57]

Change committed as 155095