1121785 - JSHistogram_Add should fail if an argument is given for a "count" histogram

Status: RESOLVED WORKSFORME

(Toolkit :: Telemetry, defect)

Description

[Matthew N. [:MattN]]

I thought that the argument to a non-keyed "count" histogram allow me to increment/set the count that gets reported but instead the argument gets ignored and we always increment by 1 for this kind of histogram. This seems like a footgun as-is especially since most documentation shows an argument to .add() but it's actually not required for some probes types.

Comment 1

[Georg Fritzsche [:gfritzsche]]

In bug 1069873 i think we didn't really want to block the .add(arg) to stay with the common, documented syntax.
I'd like us to end up with the solution that leads to the least usage issues though, whatever that is exactly.

Comment 2

[Chris H-C :chutten|PTO (back March 17)]

Usage has solidified around being able to add to count histograms by any step value the user desires, so we won't be closing it off.

We also prefer uint scalars these days instead of count histograms, so we're not likely to make changes to their behaviour.

Comment 3

[Matthew N. [:MattN]]

Are you saying that comment 0 is no longer correct meaning we don't ignore the argument? If so, this should be WORKSFORME

Comment 4

[Chris H-C :chutten|PTO (back March 17)]

Good catch, I misread the bug.

According to my read of JS_Histogram_Add, we coerce the arg to a uint32_t and then accumulate that way.

And a quick little JS in about:telemetry confirms:

Telemetry.getHistogramById("TELEMETRY_TEST_COUNT").add(10); Telemetry.getHistogramById("TELEMETRY_TEST_COUNT").snapshot().sum
<- 10

Comment 5

[Matthew N. [:MattN]]

Thanks for clarifying