1438335 - Accumulating histogram values >INT_MAX overflows

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Chris H-C :chutten]

Here's a fun one:

Open a console on about:telemetry and:

let a = Telemetry.getHistogramById("TELEMETRY_TEST_LINEAR")
a.snapshot().ranges
a.snapshot().counts
a.add(2149594584)
a.snapshot().counts

For some reason 2149594584 accumulates to the 0 bucket.

In fact, near as I can tell, values between 2^31 and 2^32 all accumulate to the 0 bucket.

2^31 - 1 accumulates to the high bucket
2^32 + 1 accumulates to the 1 bucket.

There's something odd going on here. It might be JS::ToUint32 misbehaving, or it might be something in our handling. Time to investigate.

Comment 1

[Chris H-C :chutten]

I'm 80% sure this is a type coercion going through a signed 32-bit representation and having a Bad Time, but that theory'll have to wait until tomorrow.

Comment 2

[Chris H-C :chutten]

I was right.

Accumulate and JSHistogram's add both treat intputs as uint32_t. We don't support negative values, and 32-bit widths seemed like a good idea at the time. So far so good.

The base::Histogram class that actually takes the values and stores the counts, however, takes int[1]. Telemetry's internal_HistogramAdd takes an int32_t[2].

And so we coerce silently from unsigned to signed (because C++, that's why) which falls into the "low" bucket because it is less than 1.

*sigh* I wish this was written in Rust.

[1]: https://searchfox.org/mozilla-central/rev/4234703a532006c5ef9ce09b4c487d88124526a0/ipc/chromium/src/base/histogram.cc#107
[2]: https://searchfox.org/mozilla-central/rev/4234703a532006c5ef9ce09b4c487d88124526a0/toolkit/components/telemetry/TelemetryHistogram.cpp#598

Comment 3

[Chris H-C :chutten]

I plan on changing our behaviour so that values > 2^31 received from both the JS and C++ accumulation APIs are clamped to exactly 2^31 before being stored.

(( This is instead of JS taking the value mod 2^32 and then (along with C++ values) allowing it to end up being negative if it lies between 2^31 and 2^32. ))

I also plan to document and test this 2^31 limit.

Sound like a valid plan, :Dexter, :gfritzsche?

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten from comment #2)
> The base::Histogram class that actually takes the values and stores the
> counts, however, takes int[1]. Telemetry's internal_HistogramAdd takes an
> int32_t[2].

I assume this means we were never able to record or transmit a value > 2^31?
With that, this sounds valid, thanks!

I would add that we should visibly log errors.
Should we track this error class in Telemetry?

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Chris H-C :chutten from comment #3)
> Sound like a valid plan, :Dexter, :gfritzsche?

+1 on this + the extended instrumentation that was discussed on IRC!

Comment 6

[(Away)]

(In reply to Chris H-C :chutten from comment #2)
> And so we coerce silently from unsigned to signed (because C++, that's why)
> which falls into the "low" bucket because it is less than 1.
> 
> *sigh* I wish this was written in Rust.

Wow, this happens without even a warning? I can't find any complaints about it in any of our compilers' build logs. But don't we get heaps of signed/unsigned warnings for other things (that we habitually ignore)? Or am I thinking about a different type of use pattern?

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to David Major [:dmajor] from comment #6)
> (In reply to Chris H-C :chutten from comment #2)
> > And so we coerce silently from unsigned to signed (because C++, that's why)
> > which falls into the "low" bucket because it is less than 1.
> > 
> > *sigh* I wish this was written in Rust.
> 
> Wow, this happens without even a warning? I can't find any complaints about
> it in any of our compilers' build logs. But don't we get heaps of
> signed/unsigned warnings for other things (that we habitually ignore)? Or am
> I thinking about a different type of use pattern?

I think there's a signed/unsigned implicit coercion flag somewhere, but it's *super* noisy.  (MSVC enables it, I think?  It's very spammy.)  The warning you're probably thinking of is signed/unsigned *comparison*, which is also dangerous!

Comment 8

[Chris H-C :chutten]

Attachment: bug 1438335 - Clamp the samples accumulated to histograms.

Internally Histograms use int, which is signed.

Our API uses uint32_t, which is not.

Our JS API uses ToUint32, which takes provided values modulo UINT32_MAX.

This patch consolidates behaviour so that all values greater than INT_MAX will
be correctly recorded in the highest bucket of a Histogram... instead of
having values between INT_MAX and UINT32_MAX end up as 0s, and values above
UINT32_MAX being taken modulo UINT32_MAX, but only from the JS API.

Review commit: https://reviewboard.mozilla.org/r/220604/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220604/

Comment 9

[Chris H-C :chutten]

Attachment: bug 1438335 - Document Histogram accumulation clamping behaviour

Review commit: https://reviewboard.mozilla.org/r/220606/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220606/

Comment 10

[Chris H-C :chutten]

Attachment: bug 1438335 - Test edge-of-representation accumulations to Histograms.

Review commit: https://reviewboard.mozilla.org/r/220608/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220608/

Comment 11

[Chris H-C :chutten]

Attachment: bug 1438335 - Record when we have to clamp large Telemetry accumulations.

We need to clamp accumulations to fit in our data representation (int). This
patch records the number of times, and for which probes, we had to do so.

Review commit: https://reviewboard.mozilla.org/r/220722/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220722/

Comment 12

[Chris H-C :chutten]

Comment on attachment 8951356 [details]
bug 1438335 - Clamp the samples accumulated to histograms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220604/diff/1-2/

Comment 13

[Chris H-C :chutten]

Comment on attachment 8951357 [details]
bug 1438335 - Document Histogram accumulation clamping behaviour

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220606/diff/1-2/

Comment 14

[Chris H-C :chutten]

Comment on attachment 8951358 [details]
bug 1438335 - Test edge-of-representation accumulations to Histograms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220608/diff/1-2/

Comment 15

[Chris H-C :chutten]

DATA COLLECTION REVIEW

    What questions will you answer with this data?

How much data were we previously recording as 0s, even though they were quite large values?

    Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? 

We have a bug in our data collection. We need to figure out how big of a deal this is, and whether analyses may have been affected.

    What alternative methods did you consider to answer these questions? Why were they not sufficient?

There are no alternatives. The data was lost at the client side.

    Can current instrumentation answer these questions?

Nope.

    List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories on the found on the Mozilla wiki.

telemetry.accumulate_clamped_values | Category 1 | bug 1438335
A count of the number of accumulations, keyed by the histogram that was trying to accumulate them, that had to be clamped.


    How long will this data be collected? Choose one of the following:
Forever. I (chutten) wish to monitor this data forever to determine when Telemetry users have a problem.

    What populations will you measure?
pre-release ought to be fine.

    Which release channels?
All pre-release

    Which countries?
All

    Which locales?
All

    Any other filters? Please describe in detail below.
None.

    If this data collection is default on, what is the opt-out mechanism for users?
Data Upload preferences in about:preferences.

    Please provide a general description of how you will analyze this data.
I will eyeball the number of accumulations and inform probe owners if they have a problem.

    Where do you intend to share the results of your analysis?
Bugzilla and email. Though simple analysis will be available to all on telemetry.mozilla.org if you look at the aggregated values.

Comment 16

[Chris H-C :chutten]

Comment on attachment 8951427 [details]
bug 1438335 - Record when we have to clamp large Telemetry accumulations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220722/diff/1-2/

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8951356 [details]
bug 1438335 - Clamp the samples accumulated to histograms.

https://reviewboard.mozilla.org/r/220604/#review226720

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1191
(Diff revision 2)
>        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Not a number"));
>        return true;
>      }
>  
> -    if (!JS::ToUint32(cx, args[0], &value)) {
> +    if (args[0].isNumber() && args[0].toNumber() > UINT32_MAX) {
> +      // Clamp large numerical arguments.

Do you think it's worth expanding this to mention why are we clamping to `UINT32_MAX` here?

Comment 18

[Alessio Placitelli [:Dexter]]

Comment on attachment 8951357 [details]
bug 1438335 - Document Histogram accumulation clamping behaviour

https://reviewboard.mozilla.org/r/220606/#review226722

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8951358 [details]
bug 1438335 - Test edge-of-representation accumulations to Histograms.

https://reviewboard.mozilla.org/r/220608/#review226724

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp:348
(Diff revision 2)
>    uint32_t uCountLast = 0;
>    JS::ToUint32(cx.GetJSContext(), countFirst, &uCountFirst);
>    JS::ToUint32(cx.GetJSContext(), countLast, &uCountLast);
>  
>    const uint32_t kExpectedCountFirst = 2;
> -  const uint32_t kExpectedCountLast = 1;
> +  const uint32_t kExpectedCountLast = 3;

nit: would you mind adding a comment mentioning why we expect 3? We know that now, may be worth adding it in case we stumble on this test in a year or so

Comment 20

[Alessio Placitelli [:Dexter]]

Comment on attachment 8951427 [details]
bug 1438335 - Record when we have to clamp large Telemetry accumulations.

https://reviewboard.mozilla.org/r/220722/#review226726

Comment 21

[Chris H-C :chutten]

Comment on attachment 8951356 [details]
bug 1438335 - Clamp the samples accumulated to histograms.

https://reviewboard.mozilla.org/r/220604/#review226720

> Do you think it's worth expanding this to mention why are we clamping to `UINT32_MAX` here?

`value` is uint32_t, so I figured it was implied. I could rewrite as "Clamp large numerical arguments to value's acceptable values"?

Comment 22

[Georg Fritzsche [:gfritzsche]]

When we clamp, we should log an error (to the browser console?).

Comment 23

[Chris H-C :chutten]

Comment on attachment 8951356 [details]
bug 1438335 - Clamp the samples accumulated to histograms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220604/diff/2-3/

Comment 24

[Chris H-C :chutten]

Comment on attachment 8951357 [details]
bug 1438335 - Document Histogram accumulation clamping behaviour

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220606/diff/2-3/

Comment 25

[Chris H-C :chutten]

Comment on attachment 8951358 [details]
bug 1438335 - Test edge-of-representation accumulations to Histograms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220608/diff/2-3/

Comment 26

[Chris H-C :chutten]

Comment on attachment 8951427 [details]
bug 1438335 - Record when we have to clamp large Telemetry accumulations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220722/diff/2-3/

Comment 27

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8951356 [details]
bug 1438335 - Clamp the samples accumulated to histograms.

https://reviewboard.mozilla.org/r/220604/#review227460

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1194
(Diff revision 3)
>  
> -    if (!JS::ToUint32(cx, args[0], &value)) {
> +    if (args[0].isNumber() && args[0].toNumber() > UINT32_MAX) {
> +      // Clamp large numerical arguments to value's acceptable values.
> +      // JS::ToUint32 will take arg[0] modulo 2^32 before returning it, which
> +      // may result in a smaller final value.
> +      value = UINT32_MAX;

Are we logging this error anywhere?
We should log it here or in a follow-up bug, to make it possible for devs or QA to spot when this happens.

Comment 28

[Chris H-C :chutten]

Sorry, the patches I submitted are one revision behind. They were hanging around since Comment 21 :S

I'll get the one with the logging up for review soonish.

Comment 29

[Chris H-C :chutten]

Comment on attachment 8951356 [details]
bug 1438335 - Clamp the samples accumulated to histograms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220604/diff/3-4/

Comment 30

[Chris H-C :chutten]

Comment on attachment 8951357 [details]
bug 1438335 - Document Histogram accumulation clamping behaviour

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220606/diff/3-4/

Comment 31

[Chris H-C :chutten]

Comment on attachment 8951358 [details]
bug 1438335 - Test edge-of-representation accumulations to Histograms.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220608/diff/3-4/

Comment 32

[Chris H-C :chutten]

Comment on attachment 8951427 [details]
bug 1438335 - Record when we have to clamp large Telemetry accumulations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220722/diff/3-4/

Comment 33

[Chris H-C :chutten]

Comment on attachment 8951427 [details]
bug 1438335 - Record when we have to clamp large Telemetry accumulations.

Redirecting to :francois for data review.

Comment 34

[François Marier [:francois]]

Comment on attachment 8951427 [details]
bug 1438335 - Record when we have to clamp large Telemetry accumulations.

https://reviewboard.mozilla.org/r/220722/#review227658

Scalars.yaml looks fine (with change below).

::: toolkit/components/telemetry/Scalars.yaml:1017
(Diff revision 4)
> +      saved-session pings.
> +    expires: never
> +    kind: uint
> +    keyed: true
> +    notification_emails:
> +      - [e-mail]

Can you please add your own email address here (or someone else's)?

We now ask that every probe be tied to a person (in addition to optionally be CCed to a list).

Comment 35

[François Marier [:francois]]

datareview+

(In reply to Chris H-C :chutten from comment #15)
> DATA COLLECTION REVIEW

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in Scalars.yaml.

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, telemetry setting.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, Chris.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 1.

5) Is the data collection request for default-on or default-off?

Default-on in pre-release channels only.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, permanent.

Comment 36

[Chris H-C :chutten]

Comment on attachment 8951427 [details]
bug 1438335 - Record when we have to clamp large Telemetry accumulations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220722/diff/4-5/

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01f045f12c5c
Clamp the samples accumulated to histograms. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/a06841d0b44a
Document Histogram accumulation clamping behaviour r=Dexter
https://hg.mozilla.org/integration/autoland/rev/afef6b06366a
Test edge-of-representation accumulations to Histograms. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/4aa002ab5d40
Record when we have to clamp large Telemetry accumulations. r=Dexter,francois

Comment 38

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/01f045f12c5c
https://hg.mozilla.org/mozilla-central/rev/a06841d0b44a
https://hg.mozilla.org/mozilla-central/rev/afef6b06366a
https://hg.mozilla.org/mozilla-central/rev/4aa002ab5d40

Comment 39

[Georg Fritzsche [:gfritzsche]]

Should we uplift this?

Comment 40

[Chris H-C :chutten]

Comment on attachment 8951356 [details]
bug 1438335 - Clamp the samples accumulated to histograms.

Approval Request Comment
[Feature/Bug causing the regression]: unknown, this appears to always have been the case
[User impact if declined]: Telemetry from a list of histograms will erroneously report 0s when instead they should report very large numbers.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes. You can see the data here, if you'd like: https://mzl.la/2CmmpmK
[Needs manual test from QE? If yes, steps to reproduce]: No, I don't think so.
[List of other uplifts needed for the feature/fix]: Just the patches in this bug
[Is the change risky?]: I wouldn't say so, no
[Why is the change risky/not risky?]: It only changes the values reported when they are above INT_MAX in value. This rarely happens.
[String changes made/needed]: None.

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8951356 [details]
bug 1438335 - Clamp the samples accumulated to histograms.

Verified in nightly, let's uplift this for beta 14.

Comment 42

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/00e20572ed81
https://hg.mozilla.org/releases/mozilla-beta/rev/8505f530d9f9
https://hg.mozilla.org/releases/mozilla-beta/rev/dc5d87eb8bfc
https://hg.mozilla.org/releases/mozilla-beta/rev/72a0030b41dc

Comment 43

[Chris H-C :chutten]

One quick clarification, the maximum value permitted to be stored in histograms is INT_MAX - 1[1]. So though we  clamp values coming in from JS to UINT32_MAX, and then clamp values heading out to the histogram storage at INT_MAX, the histogram storage itself performs a _third_ clamp down to INT_MAX - 1.

Huzzah, off-by-one.

[1]: https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/ipc/chromium/src/base/histogram.cc#107

Comment 44

[Andrei Vaida [:avaida]]

(In reply to Chris H-C :chutten from comment #40)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes. You can see the data here, if
> you'd like: https://mzl.la/2CmmpmK
> [Needs manual test from QE? If yes, steps to reproduce]: No, I don't think
> so.

Setting qe-verify- based on Chris's assessment on manual testing needs and the fact that this fix has automated coverage.