media/ presubmit check on UMA_HISTOGRAM_ENUMERATION is outdated |
|||||
Issue description
Today we have a check in media/PRESUBMIT.py called _CheckForHistogramOffByOne() that demands the following rule [1] :
UMA_HISTOGRAM_ENUMERATION reports in src/media/ are expected to adhere to the following guidelines:
- The max value (3rd argument) should be an enum value equal to the last valid value, e.g. FOO_MAX = LAST_VALID_FOO.
- 1 must be added to that max value.
However, now with enum class in C++11, FOO_MAX + 1 simply won't work.
Also, this is not consistent with the current UMA_HISTOGRAM_ENUMERATION documentation, which recommends to use "COUNT" instead of "MAX" [2]:
// enum class MyEnum {
// FIRST_VALUE = 0,
// SECOND_VALUE = 1,
// ...
// FINAL_VALUE = N,
// COUNT
// };
// UMA_HISTOGRAM_ENUMERATION("My.Enumeration",
// MyEnum::SOME_VALUE, MyEnum::COUNT);
With that, I suggest we stick with the general recommendation of using COUNT, and update our PRESUBMIT.py to reflect that.
[1] https://chromium.googlesource.com/chromium/src/+/c8c0ca6bf44b5c456fb020c9f00b3b419f548154%5E%21/#F6
[2] https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?rcl=0999f90c07a462fc7a8b5bfd905eba968f943d4a&l=48
,
Oct 6 2017
Actually media/ seems to be the only one checking this, I wonder whether we should fix this to check for the COUNT, and make it more generically available, or should we just drop it? +isherman for comments as metrics/ OWNERS.
,
Oct 9 2017
I think some teams prefer using FOO_MAX = LAST_VALID_FOO so that there's no need to handle "COUNT" in switch stmts. I do agree that with the need to typecast for "MAX + 1" with enum classes makes this reasoning less compelling – either way, you're having to fight the compiler =/ The best-practices recommendations are non-mandatory, and I think it's fine for different teams to prefer MAX vs. COUNT, so I wouldn't want to enforce any global presubmit validation that forces one or the other. OTOH, if the presubmit script can help catch errors without mandating a particular style, then I do think that it would be helpful to generalize it to cover more of the code base.
,
Oct 9 2017
Thanks for the insights! Given we are using enum class more and more, MAX + 1 won't really work without casting, so even though COUNT has its own problems I feel it's a better option. Or, does it make sense to update UMA_HISTOGRAM_ENUMERATION to take MAX instead of COUNT? It can always do a cast and +1 internally. But that would require a lot of change and people are already used to the +1 syntax I guess.
,
Oct 9 2017
Yeah, I think it would be tough to migrate at this point, as it would confuse people. Plus, many existing clients would need to pass COUNT - 1, which is a bit confusing. (OTOH, there's not too much harm from passing COUNT instead of COUNT - 1 – it just allocates a single extra bucket. That's much better than being off-by-one in the other direction.) We could also introduce a new macro that expects MAX, but I think it would be confusing to have two similar macros.
,
Oct 10
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
,
Today
(19 hours ago)
xhwang@, I'm assuming it's safe to mark this as "Wont Fix" given the age. WDYT? If not, perhaps we can assign it to someone to review pri. and set it to available? Thanks.
,
Today
(92 minutes ago)
dalecurtis: This is related to issue 773993 . Given now it's possible to use the two-parameter version of UMA_HISTOGRAM_ENUMERATION, and even for the three-parameter version it's recommended to use COUNT instead of MAX+1, I suggest we simply drop this presubmit check. WDYT? |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by dalecur...@chromium.org
, Oct 6 2017