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

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: adbugger, Assigned: adbugger, Mentored)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
See Also: → 1434004
(Assignee)

Comment 1

a year ago
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
Attachment #8946488 - Flags: review?(chutten)
(Assignee)

Comment 2

a year ago
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?
Flags: needinfo?(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.
Attachment #8946488 - Flags: review?(chutten) → review+
Flags: needinfo?(chutten)
(Assignee)

Comment 4

a year ago
Marking this patch for checkin because of no bustages on try and r+
Keywords: checkin-needed

Comment 5

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c33556a7467
Allow C++ to accumulate multiple samples into a keyed categorical histogram. r=chutten
Keywords: checkin-needed
Mentor: chutten
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/3c33556a7467
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Assignee)

Updated

a year ago
See Also: → 1428893
You need to log in before you can comment on or make changes to this bug.