UMA: Document that trying to use an enum with just 1 value will crash |
|||||
Issue description
Version: 55
OS: All
What steps will reproduce the problem?
(1) Call UMA_HISTOGRAM_ENUMERATION("...", 0, 1)
What is the expected output?
An enum with a single bucket (or more, I don't care), with all the stats going into bucket 0.
What do you see instead?
[FATAL:histogram.cc(839)] Check failed: valid_arguments.
This fails in InspectConstructionArguments because there is a check:
if (*bucket_count < 3)
return false;
bucket_count = boundary + 1
(In this case, boundary is 1, so bucket_count = 2)
Workaround: Pass 2 as the boundary argument instead of 1.
I shouldn't have to do this. If the histograms system requires a bucket count of at least 3, the UMA_HISTOGRAM_ENUMERATION macro should just bump up the bucket count if necessary (leaving an extra unused bucket). The Java version, RecordHistogram.recordEnumeratedHistogram, should also be updated.
Failing that, the documentation should reflect that there is a minimum boundary value of 2 (i.e., all enums are required to have at least 2 values).
Reason I want this: I want to create an enum that counts calls to various methods. Right now there is only one method, hence a single-value enum. But I want it to remain future-proof to adding more methods later on.
,
Sep 9 2016
> This is working as intended, and is already documented in some places, though > could probably be documented even more thoroughly. In that case, I might send out a CL documenting it right above UMA_HISTOGRAM_ENUMERATION. > UMA_HISTOGRAM_BOOLEAN is a convenient shorthand But I want to start my own enum so I can expand it later. > though we generally discourage histograms that only have a single bucket, > since it's typically advisable to build in a baseline for any given metric. I don't know what you mean by this. I just want a global count of methods called. > We certainly could remove the CHECK, but I believe that it catches bugs every > once in a while, so I'd rather not just remove it Oh we can't remove the CHECK. I tried and then I got a subsequent check fail of -2147483648 >= 0. So the system can't cope with fewer than three buckets. But I think UMA_HISTOGRAM_ENUMERATION should be able to work around it. > -- there's an easy workaround It's easy, but messy: https://codereview.chromium.org/2228403002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/webshare/ShareServiceImpl.java?column_width=100 see line 28--30. It's quite non-intuitive why I am claiming a count of 2 even though there is only one enum value. If someone adds a second value, they might think they have to increment the count to 3 for some reason, so I have to add a comment explaining that it is a kludge. > and it's kind of a feature that writing a single-bucket histogram requires the > author to think a bit harder about what they're doing. So is a single-bucket histogram a legitimate thing to do, or not? I haven't seen any clear reason why it isn't, and if I used an always-false Boolean or an always-0 int, I wouldn't have been forced to think about this. It seems fine. And I don't think it's a good trade-off that we're saying to authors, "this crashes in order to make you think harder about what you're doing; if you still think you're doing the right thing, go ahead and add a hack around the crash."
,
Sep 9 2016
Ilya is away so I will reply for him: > In that case, I might send out a CL documenting it right above UMA_HISTOGRAM_ENUMERATION. Sounds good to me. > So is a single-bucket histogram a legitimate thing to do, or not? It's legitimate in some cases, but most of the time I've seen it's being misused. People try to divide the count of one histogram by the count of another, assuming that the first is a strict subset of the other. But there are often edge cases, e.g., things like an accessibility API or extensions, that can bypass part of the typical control flow, and the the subset relationship doesn't strictly hold. It might be wrong to use one count to normalize the other. That's why we encourage people to use enumerated histograms that include their baseline within the same histogram. Obviously, if you have no plans to normalize the value by another then this criticism doesn't apply.
,
Sep 11 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 12 2017
Still legitimate. We either need better documentation or simply handling this under the hood.
,
Sep 12
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by isherman@chromium.org
, Sep 8 2016Status: Available (was: Untriaged)
Summary: UMA: Document that trying to use an enum with just 1 value will crash (was: UMA: Trying to use an enum with just 1 value crashes)