Change crash keys to be registered with a maximum length instead of number of chunks.

Change crash keys to be registered with a maximum length instead of number of chunks.

Most users won't have an idea of what a "chunk" is, and performing calculations
with them is difficult because the chunk length differs by platform.

This CL switches to registering based on a maximum value length. For Chrome keys,
it provides sensible length constants with COMPILE_ASSERT'd guarantees about the
chunkiness.

BUG=77656
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186267

[Scott Hess - ex-Googler, 2013-02-08 00:35:08]

I think I'm good on this overall, but I'm a little nervous about having the
chunking factor change between platforms for the same key.  Right now, I think
OSX effectively provides 4x as much space for chunked keys as other platforms,
which is fine because it also feels less sensitive to size issues.  I wouldn't
want to end up with people accidentally specifying massive value support on
Windows and blowing something out.

One option would be to make the size an enum, so that you explicitly set
"small", "medium", and "large", and cannot set anything else.  Another option
would be to simply assert that OSX multi-chunk values are chunked at 64 bytes,
while allowing single-chunk values to still take up to 256 bytes on that
platform.

[Robert Sesek, 2013-02-08 19:58:27]

On 2013/02/08 00:35:08, shess wrote:
> I think I'm good on this overall, but I'm a little nervous about having the
> chunking factor change between platforms for the same key.  Right now, I think
> OSX effectively provides 4x as much space for chunked keys as other platforms,
> which is fine because it also feels less sensitive to size issues.  I wouldn't
> want to end up with people accidentally specifying massive value support on
> Windows and blowing something out.

Each platform also has a limit on the maximum number of key-value pairs, but I
couldn't find usable constants for those. I can either look harder or add them
and then DCHECK that the return value of InitCrashKeys is <= that. I think that
will resolve this concern.

> One option would be to make the size an enum, so that you explicitly set
> "small", "medium", and "large", and cannot set anything else.  Another option
> would be to simply assert that OSX multi-chunk values are chunked at 64 bytes,
> while allowing single-chunk values to still take up to 256 bytes on that
> platform.

I don't think base/ should know about the enum -- it only cares about strings
and lengths. Perhaps it's something that can be added to the chrome/ layer, but
for now I think just using lengths is sufficient. This could change after I
manually convert child_process_logging.

[Robert Sesek, 2013-02-19 20:43:42]

ping

[Scott Hess - ex-Googler, 2013-02-19 23:16:10]

[Looping in Eric to get a second opinion.  I feel like my opinion is being
lamely-represented by me.]

On 2013/02/08 19:58:27, rsesek wrote:
> On 2013/02/08 00:35:08, shess wrote:
> > One option would be to make the size an enum, so that you explicitly set
> > "small", "medium", and "large", and cannot set anything else.  Another
option
> > would be to simply assert that OSX multi-chunk values are chunked at 64
bytes,
> > while allowing single-chunk values to still take up to 256 bytes on that
> > platform.
> 
> I don't think base/ should know about the enum -- it only cares about strings
> and lengths. Perhaps it's something that can be added to the chrome/ layer,
but
> for now I think just using lengths is sufficient. This could change after I
> manually convert child_process_logging.

I still am bothered by the optional chunking based on platform considerations. 
I think that has potential to confuse developers.  I think in the end I like "As
much as possible in one chunk" and "Always chunk", with the latter having a
maximum length (this could be coded as 0 or kNoChunking for "no chunking" and
non-0 for "chunk up to this length").  OSX will just have fewer chunks than
Windows, but the presence/absence of chunking would be consistent across
platforms.

https://codereview.chromium.org/12211080/diff/4001/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/12211080/diff/4001/chrome/common/crash_keys.c...
chrome/common/crash_keys.cc:23: // used sparangly.
Sparingly.

[Robert Sesek, 2013-02-19 23:22:43]

On 2013/02/19 23:16:10, shess wrote:
> [Looping in Eric to get a second opinion.  I feel like my opinion is being
> lamely-represented by me.]
> 
> On 2013/02/08 19:58:27, rsesek wrote:
> > On 2013/02/08 00:35:08, shess wrote:
> > > One option would be to make the size an enum, so that you explicitly set
> > > "small", "medium", and "large", and cannot set anything else.  Another
> option
> > > would be to simply assert that OSX multi-chunk values are chunked at 64
> bytes,
> > > while allowing single-chunk values to still take up to 256 bytes on that
> > > platform.
> > 
> > I don't think base/ should know about the enum -- it only cares about
strings
> > and lengths. Perhaps it's something that can be added to the chrome/ layer,
> but
> > for now I think just using lengths is sufficient. This could change after I
> > manually convert child_process_logging.
> 
> I still am bothered by the optional chunking based on platform considerations.

> I think that has potential to confuse developers.  I think in the end I like
"As
> much as possible in one chunk" and "Always chunk", with the latter having a
> maximum length (this could be coded as 0 or kNoChunking for "no chunking" and
> non-0 for "chunk up to this length").  OSX will just have fewer chunks than
> Windows, but the presence/absence of chunking would be consistent across
> platforms.

I like that, too, but it still relies on platform specifics. Because as you
pointed out, OS X has more space per chunk. At the very least, this proposal
hides those platform differences a bit better.

[Scott Hess - ex-Googler, 2013-02-19 23:46:36]

On 2013/02/19 23:22:43, rsesek wrote:
> On 2013/02/19 23:16:10, shess wrote:
> > [Looping in Eric to get a second opinion.  I feel like my opinion is being
> > lamely-represented by me.]
> > 
> > On 2013/02/08 19:58:27, rsesek wrote:
> > > On 2013/02/08 00:35:08, shess wrote:
> > > > One option would be to make the size an enum, so that you explicitly set
> > > > "small", "medium", and "large", and cannot set anything else.  Another
> > option
> > > > would be to simply assert that OSX multi-chunk values are chunked at 64
> > bytes,
> > > > while allowing single-chunk values to still take up to 256 bytes on that
> > > > platform.
> > > 
> > > I don't think base/ should know about the enum -- it only cares about
> strings
> > > and lengths. Perhaps it's something that can be added to the chrome/
layer,
> > but
> > > for now I think just using lengths is sufficient. This could change after
I
> > > manually convert child_process_logging.
> > 
> > I still am bothered by the optional chunking based on platform
considerations.
> 
> > I think that has potential to confuse developers.  I think in the end I like
> "As
> > much as possible in one chunk" and "Always chunk", with the latter having a
> > maximum length (this could be coded as 0 or kNoChunking for "no chunking"
and
> > non-0 for "chunk up to this length").  OSX will just have fewer chunks than
> > Windows, but the presence/absence of chunking would be consistent across
> > platforms.
> 
> I like that, too, but it still relies on platform specifics. Because as you
> pointed out, OS X has more space per chunk. At the very least, this proposal
> hides those platform differences a bit better.

In my experience, the amount of data stored as the value is almost never
interesting.  Either it's enough, and all are happy, or it's not enough, and
it's obvious that something needs to be done to address that.  But having the
operative key be |mykey| on one platform or |mykey-0| on another _is_ a problem.
 If you have automated queries/script, they need to adjust to different
platforms in a new way.  If you're searching the dashboard for info, you
potentially will accidentally miss crashes because you're searching for the
wrong key.

Did that make sense?

[eroman, 2013-02-20 00:04:40]

I think this approach is fine. I am much less concerned than shess about the
differences in key names, since in my experience actually doing any sort of
useful dremel queries across chunked values is too hard anyway. I have instead
been relying on chromecrash processor to join chunked values into a single one
which can be worked with (for instance the URL field).

I see this problem more as a limitation of the crash tooling. In my opinion if
this is enough of a nuisance, we need to get crash-team to do these
normalizations directly on the processing end (should be pretty straight forward
for them to join fields ending in *-[0-9]+

https://codereview.chromium.org/12211080/diff/4001/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/12211080/diff/4001/chrome/common/crash_keys.c...
chrome/common/crash_keys.cc:30: static const size_t kSingleChunkLength =
There is a bit of a mismatch between what the meaning of "length" is:

On Windows breakpad the value can be up to kValueMaxLength-1 characters. As the
final character is required to be NULL.

I think it would be clearer if all the lengths here were defined in terms of
actual data, and not including NULLs. For instance, with three "chunks" you
could fit 63*3 worth of data, compared to this constant suggesting you can fit
64*3.

[eroman, 2013-02-20 00:08:43]

https://codereview.chromium.org/12211080/diff/4001/base/debug/crash_logging.cc
File base/debug/crash_logging.cc (right):

https://codereview.chromium.org/12211080/diff/4001/base/debug/crash_logging.c...
base/debug/crash_logging.cc:166: std::string value_string = value.substr(0,
crash_key.max_length).as_string();
Is this restriction necessary? Assuming implementations use fixed sized buffers
for the chunks, may as well truncate at a multiple of the chunk size.

[Robert Sesek, 2013-02-27 18:52:48]

PTAL. I agree with Eric that our backend infrastructure should do a better job
of handling the chunked keys so that humans and scripts don't have to do it
themselves. I also checked the backend code and there does not appear to be an
actual key/value length limit. It may make sense, then, to have all the
platforms use the same chunk size. In this patchset, I'm keeping registration
using lengths at the base/ layer, rather than in terms of number of chunks. But
at the chrome/ layer, I think the guarantees of the kSize contants match what
you want, Scott.

https://codereview.chromium.org/12211080/diff/4001/base/debug/crash_logging.cc
File base/debug/crash_logging.cc (right):

https://codereview.chromium.org/12211080/diff/4001/base/debug/crash_logging.c...
base/debug/crash_logging.cc:166: std::string value_string = value.substr(0,
crash_key.max_length).as_string();
On 2013/02/20 00:08:43, eroman wrote:
> Is this restriction necessary? Assuming implementations use fixed sized
buffers
> for the chunks, may as well truncate at a multiple of the chunk size.

At this layer, I think it makes more sense to deal in lengths rather than chunk
sizes since this doesn't know about Breakpad, and thus the sizes, at all. I do
expect in practice that lengths will be multiples of the chunk sizes, which is
why those constants exist in crash_keys.cc.

https://codereview.chromium.org/12211080/diff/4001/chrome/common/crash_keys.cc
File chrome/common/crash_keys.cc (right):

https://codereview.chromium.org/12211080/diff/4001/chrome/common/crash_keys.c...
chrome/common/crash_keys.cc:23: // used sparangly.
On 2013/02/19 23:16:10, shess wrote:
> Sparingly.

Done.

https://codereview.chromium.org/12211080/diff/4001/chrome/common/crash_keys.c...
chrome/common/crash_keys.cc:30: static const size_t kSingleChunkLength =
On 2013/02/20 00:04:40, eroman wrote:
> There is a bit of a mismatch between what the meaning of "length" is:
> 
> On Windows breakpad the value can be up to kValueMaxLength-1 characters. As
the
> final character is required to be NULL.
> 
> I think it would be clearer if all the lengths here were defined in terms of
> actual data, and not including NULLs. For instance, with three "chunks" you
> could fit 63*3 worth of data, compared to this constant suggesting you can fit
> 64*3.

Done. Same issue on Mac as well.

[Robert Sesek, 2013-02-27 18:52:49]

PTAL. I agree with Eric that our backend infrastructure should do a better job
of handling the chunked keys so that humans and scripts don't have to do it
themselves. I also checked the backend code and there does not appear to be an
actual key/value length limit. It may make sense, then, to have all the
platforms use the same chunk size. In this patchset, I'm keeping registration
using lengths at the base/ layer, rather than in terms of number of chunks. But
at the chrome/ layer, I think the guarantees of the kSize contants match what
you want, Scott.

[Scott Hess - ex-Googler, 2013-02-27 20:28:34]

On 2013/02/27 18:52:49, rsesek wrote:
> PTAL. I agree with Eric that our backend infrastructure should do a better job
> of handling the chunked keys so that humans and scripts don't have to do it
> themselves. I also checked the backend code and there does not appear to be an
> actual key/value length limit. It may make sense, then, to have all the
> platforms use the same chunk size.

I am with you, except that I'm not optimistic that this is likely to change
anytime soon, so whatever situation this code results in may pertain for years.

> In this patchset, I'm keeping registration
> using lengths at the base/ layer, rather than in terms of number of chunks.
But
> at the chrome/ layer, I think the guarantees of the kSize contants match what
> you want, Scott.

AFAICT, this still allows the case that one platform calls it "key" and the
other platforms call it "key-0", "key-1", "key-2" and "key-3".  My concern is
not about how much data fits in the values, it's about whether people use the
presence or absence of the keys as indicators of some result, without realizing
that the value-size they specified can itself affect the key naming, and without
realizing the that UI may not be showing them results because they didn't phrase
their queries to account for this.  For example, if you were to search for
custom_name:"url", you will not find custom_name:"url-chunk-1", you have to
instead use custom_name.contains:"url" (which is a substring search, so still
not great).  I do not disagree with you about the sensibility of using sizes in
the code, and letting the code break things up, that makes sense.  But the key
names are also an aspect of the API, and this code does not deal with how those
are exposed.

As I mentioned earlier, I think having the choices be "Never chunked, size is
best effort" versus "Chunked, truncated to this length" handles this adequately.
 If you say "Never chunked" in your code, then you always search for "key".  If
you say "chunked up to this length", then you always search for "key-0". 
Platforms which support longer values may degenerate to having the truncated
value fit within a single chunk, which is fine.

That said, I'm willing to LGTM, as we are debating hypothetical issues.  It will
either be a problem or it won't, and if it is we can change it.  More-or-less.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging.cc
File base/debug/crash_logging.cc (right):

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:141: total_keys += keys[i].max_length /
chunk_max_length;
I think it might be worthwhile to have some sort of sanity check, here.  While
this code is striving to express these things in terms of abstract string
lengths, the fact of the matter is if somehow has max_length == 4096, that's
going to be a problem.  I'd rather see that here, where I could make sense of
the error, versus deep in breakpad where it's disconnected from the cause.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging.h
File base/debug/crash_logging.h (right):

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.h:36: // items will be encoded, since breakpad limits
values to 255 bytes.
While you're editing, may want to drop the specific numeric references in this
comment.

[eroman, 2013-03-01 07:25:24]

LGTM after answering my integer rounding question.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging.cc
File base/debug/crash_logging.cc (right):

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:53: i < crash_key->max_length / g_chunk_max_length_;
I believe you want to round the integer division up and not down here.

For instance, if max_length=12 and g_chunk_max_length=5 there would be 3 chunks
to clear not 2.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:77: for (size_t i = 0; i < crash_key->max_length /
g_chunk_max_length_; ++i) {
Same comment as above -- I believe this need to round up.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:141: total_keys += keys[i].max_length /
chunk_max_length;
See comment above -- I believe this should be rounding division up.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:171: offset += chunk.length();
[optional] How about moving this increment into the for loop:
  offset += chunk_max_length

[Scott Hess - ex-Googler, 2013-03-01 15:16:49]

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging.cc
File base/debug/crash_logging.cc (right):

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:53: i < crash_key->max_length / g_chunk_max_length_;
On 2013/03/01 07:25:24, eroman wrote:
> I believe you want to round the integer division up and not down here.
> 
> For instance, if max_length=12 and g_chunk_max_length=5 there would be 3
chunks
> to clear not 2.

I was fine with this as-is, but ... if you do go this route, it probably is
worth spinning up a helper function to do this, so that future editors don't go
awry on the calculation.

[Robert Sesek, 2013-03-01 20:02:34]

On 2013/02/27 20:28:34, shess wrote:
> On 2013/02/27 18:52:49, rsesek wrote:
> > PTAL. I agree with Eric that our backend infrastructure should do a better
job
> > of handling the chunked keys so that humans and scripts don't have to do it
> > themselves. I also checked the backend code and there does not appear to be
an
> > actual key/value length limit. It may make sense, then, to have all the
> > platforms use the same chunk size.
> 
> I am with you, except that I'm not optimistic that this is likely to change
> anytime soon, so whatever situation this code results in may pertain for
years.
> 
> > In this patchset, I'm keeping registration
> > using lengths at the base/ layer, rather than in terms of number of chunks.
> But
> > at the chrome/ layer, I think the guarantees of the kSize contants match
what
> > you want, Scott.
> 
> AFAICT, this still allows the case that one platform calls it "key" and the
> other platforms call it "key-0", "key-1", "key-2" and "key-3".  My concern is
> not about how much data fits in the values, it's about whether people use the
> presence or absence of the keys as indicators of some result, without
realizing
> that the value-size they specified can itself affect the key naming, and
without
> realizing the that UI may not be showing them results because they didn't
phrase
> their queries to account for this.  For example, if you were to search for
> custom_name:"url", you will not find custom_name:"url-chunk-1", you have to
> instead use custom_name.contains:"url" (which is a substring search, so still
> not great).  I do not disagree with you about the sensibility of using sizes
in
> the code, and letting the code break things up, that makes sense.  But the key
> names are also an aspect of the API, and this code does not deal with how
those
> are exposed.
> 
> As I mentioned earlier, I think having the choices be "Never chunked, size is
> best effort" versus "Chunked, truncated to this length" handles this
adequately.
>  If you say "Never chunked" in your code, then you always search for "key". 
If
> you say "chunked up to this length", then you always search for "key-0". 
> Platforms which support longer values may degenerate to having the truncated
> value fit within a single chunk, which is fine.
> 
> That said, I'm willing to LGTM, as we are debating hypothetical issues.  It
will
> either be a problem or it won't, and if it is we can change it.  More-or-less.

The "Never chunked" is an option at the moment, I just call it kSmallSize. But
the documentation and a COMPILE_ASSERT guarantee that it fits into exactly one
chunk, always.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging.cc
File base/debug/crash_logging.cc (right):

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:53: i < crash_key->max_length / g_chunk_max_length_;
On 2013/03/01 15:16:49, shess wrote:
> On 2013/03/01 07:25:24, eroman wrote:
> > I believe you want to round the integer division up and not down here.
> > 
> > For instance, if max_length=12 and g_chunk_max_length=5 there would be 3
> chunks
> > to clear not 2.
> 
> I was fine with this as-is, but ... if you do go this route, it probably is
> worth spinning up a helper function to do this, so that future editors don't
go
> awry on the calculation.

Done.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:53: i < crash_key->max_length / g_chunk_max_length_;
On 2013/03/01 07:25:24, eroman wrote:
> I believe you want to round the integer division up and not down here.
> 
> For instance, if max_length=12 and g_chunk_max_length=5 there would be 3
chunks
> to clear not 2.

Done.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:77: for (size_t i = 0; i < crash_key->max_length /
g_chunk_max_length_; ++i) {
On 2013/03/01 07:25:24, eroman wrote:
> Same comment as above -- I believe this need to round up.

Done.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:141: total_keys += keys[i].max_length /
chunk_max_length;
On 2013/02/27 20:28:34, shess wrote:
> I think it might be worthwhile to have some sort of sanity check, here.  While
> this code is striving to express these things in terms of abstract string
> lengths, the fact of the matter is if somehow has max_length == 4096, that's
> going to be a problem.  I'd rather see that here, where I could make sense of
> the error, versus deep in breakpad where it's disconnected from the cause.

Done. I chose 1024.

https://codereview.chromium.org/12211080/diff/17001/base/debug/crash_logging....
base/debug/crash_logging.cc:141: total_keys += keys[i].max_length /
chunk_max_length;
On 2013/03/01 07:25:24, eroman wrote:
> See comment above -- I believe this should be rounding division up.

Done.

[Scott Hess - ex-Googler, 2013-03-01 20:32:25]

On Fri, Mar 1, 2013 at 12:02 PM,  <rsesek@chromium.org> wrote:
> The "Never chunked" is an option at the moment, I just call it kSmallSize.
But
> the documentation and a COMPILE_ASSERT guarantee that it fits into exactly
> one chunk, always.

What I hear you saying for my concerns is "But it's silly encoding
that kind of implementation detail into the API" while at the same
time saying "But you can already do that by adhering to a convention".
 My preference is to expose relevant limitations in the API in a way
that makes them easy to avoid.  Hiding them does not make them
irrelevant.

Still, LGTM, because I think we can just agree to disagree on this.
You're actually making the changes that I've wished someone would make
for a couple years, and if we ever get many users for this code we'll
have an entire different set of problems anyhow.

-scott

[Robert Sesek, 2013-03-01 20:50:26]

On 2013/03/01 20:32:25, shess wrote:
> On Fri, Mar 1, 2013 at 12:02 PM,  <mailto:rsesek@chromium.org> wrote:
> > The "Never chunked" is an option at the moment, I just call it kSmallSize.
> But
> > the documentation and a COMPILE_ASSERT guarantee that it fits into exactly
> > one chunk, always.
> 
> What I hear you saying for my concerns is "But it's silly encoding
> that kind of implementation detail into the API" while at the same
> time saying "But you can already do that by adhering to a convention".
>  My preference is to expose relevant limitations in the API in a way
> that makes them easy to avoid.  Hiding them does not make them
> irrelevant.

I think we both agree that we need something that says "don't chunk." But I
think your concern is how that appears on the backend, whereas I think the
backend should just be smart enough to merge chunks automatically. The concern I
have for "don't chunk" is so that the user knows how many of the preciously
limited slots the data will reserve. And I do have a preference for using
explicit lengths rather than a magic value 0 to indicate no chunking.

Obviously we can revisit depending on need -- this CL is in fact revisiting an
earlier decision :). It is also on my list to look into getting our crash
processor to merge chunked keys.

[Robert Sesek, 2013-03-04 21:56:03]

+brettw for chrome/ and base/ OWNERS

[brettw, 2013-03-05 06:43:20]

owners lgtm

[commit-bot: I haz the power, 2013-03-05 16:58:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/12211080/26001

[commit-bot: I haz the power, 2013-03-05 18:51:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-05 19:35:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/12211080/35002

[commit-bot: I haz the power, 2013-03-05 19:43:29]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[Robert Sesek, 2013-03-05 21:56:08]

Committed patchset #5 manually as r186267 (presubmit successful).