1433998 - Allow C++ to accumulate multiple samples into a keyed categorical histogram in one call

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Aditya Bharti [:adbugger]]

As per bug 1364043, the C++ Telemetry::Accumulate function was extended to support adding multiple samples to a histogram in one call. Bug 1428885 extended the same for keyed histograms and bug 1428888 extended the same for categorical histograms. This bug is to extend the same support for keyed categorical histograms. See comment #27 on bug 1364043 for more information.

Comment 1

[Aditya Bharti [:adbugger]]

Attachment: Telmetry::Accumulate() now supports Accumulate(string key, enumValues array)

Support for C++ to add multiple samples to Keyed Categorical histograms.
Also added another test case.

Extended the Telemetry::Accumulate() API to accept a string key and a nsTArray of Telemetry::LABELS_* enums. The typesafety of the class templates ensures there are no label mismatches within the array.

Since there is no singular version for accumulating a single string label into a keyed categorical histogram, support for the plural version with multiple string labels has not been included.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28a3156ecc038b5e87b1458e038848daefd3f9df

Comment 2

[Aditya Bharti [:adbugger]]

Should I support accumulating multiple string labels into a keyed categorical histogram? Since there is no API to do that even for the single case, am I correct in assuming that would be superfluous?

In which case, I guess modifying the JSAPI is the next series of steps. Would you mind pointing me in the right direction?

Comment 3

[Chris H-C :chutten]

Comment on attachment 8946488 [details] [diff] [review]
Telmetry::Accumulate() now supports Accumulate(string key, enumValues array)

Review of attachment 8946488 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

As for adding a string-based one, I think you're right that it need not be done.

For the JS portions, things get a little different. In JS, consumers can ask for Histogram objects and then call things like "add" on them. These JS methods call into the JS{Keyed}Histogram_* C++ functions in TelemetryHistogram.cpp. For Scalars, we put a few functions (like scalarAdd) on the Telemetry object directly. These API endpoints are defined in nsITelemetry.idl (the idl generates the bindings at build time, instead of us having to bind things at runtime like we do for histograms) and are dispatched in Telemetry.cpp in the TelemetryImpl::* namespace (which is where you'll find TelemetryImple::ScalarAdd, for instance)

Histograms might be the easier ones to tackle at first, as we already inspect and coerce the types of arguments provided. We might be able to introspect whether the provided argument is an array and then handle it specially inside JSHistogram_Add.

Scalars might involve adding a new idl method... or it might not. I'm not sure if JSHandleValue has to point to a simple value or whether it, too, can mean an array. This will be some investigation on your part.

Testing is different as well. You'll be writing xpcshell tests which are located in the toolkit/components/telemetry/tests/unit directory. They will very much resemble the gtests you've been writing, at least in content, but with far less "I'm writing C++ to access JS values" boilerplate.

For ad-hoc testing and debugging as you go, you can call Telemetry methods from a built Firefox (./mach build && ./mach run) by first navigating to about:telemetry opening the developer tools (right-click > Inspect Element, or, Ctrl+Shift+K) and then typing things like Telemetry.scalarAdd in the Console.

...sheez, that's a lot of text I just wrote. Please let me know if you have questions about any of it.

Comment 4

[Aditya Bharti [:adbugger]]

Marking this patch for checkin because of no bustages on try and r+

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c33556a7467
Allow C++ to accumulate multiple samples into a keyed categorical histogram. r=chutten

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3c33556a7467