Closed Bug 1500791 Opened 3 years ago Closed 3 years ago

PrioEncoder.encode() should accept arbitrary sequence of bools

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: rhelmer, Assigned: rhelmer)

Details

Attachments

(2 files)

PrioEncoder currently expects a dictionary (JS options object) with 3 hardcoded booleans:

https://searchfox.org/mozilla-central/rev/c56977420df7a1b692ce0f7e499ddb364d9fd7b2/dom/chrome-webidl/PrioEncoder.webidl#13

Instead, this should allow an arbitrary set of key/value pairs. Per bug 1421501 comment 136, we will keep the Telemetry and PrioEncoder code in-sync so we'll know which version of Firefox is sending which keys.

I think the right way to do this in Web IDL is to make the PrioParams dictionary optional, have `encode()` throw if there are no members, and then modify https://searchfox.org/mozilla-central/rev/c56977420df7a1b692ce0f7e499ddb364d9fd7b2/dom/prio/PrioEncoder.cpp#108 to iterate over the dictionary instead of having a hard-coded list of members.

`PrioEncoder.encode()` should throw also if any of the dictionary entries are anything but key/value pairs, where each value is a boolean.

We shouldn't need to touch the Telemetry client code for this change, and the Telemetry code will then be responsible for deciding which histograms to encode and include in the main ping.
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
(In reply to Robert Helmer [:rhelmer] from comment #0)
> `PrioEncoder.encode()` should throw also if any of the dictionary entries
> are anything but key/value pairs, where each value is a boolean.

While working on this, I realized that PrioEncoder does not actually need the keys at all, a sequence of booleans is the only thing actually used. As long as they are in the order that Telemetry code passes them in that's all we need.

So actually this could be much simpler, maybe keep PrioParams dict required argument to encode() in the Web IDL and have the dictionary be:

dictionary PrioParams {
  sequence<boolean> booleans;
};

This leaves room to add other types besides bool in the future, without breaking the existing API, and we shouldn't need to do any explicit check+throw in the code to ensure that it's valid.
Summary: PrioEncoder.encode() should accept arbitrary key/value pairs → PrioEncoder.encode() should accept arbitrary sequence of bools
Marking as P1 as it is currently being worked on. (so it shows up in our dashboards)
Priority: -- → P1
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce8127ea1c4b
remove hardcoded restriction on bools that may be prio-encoded r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/f3cd65f62168
Telemetry client now decides which histograms should be prio-encoded r=janerik
https://hg.mozilla.org/mozilla-central/rev/ce8127ea1c4b
https://hg.mozilla.org/mozilla-central/rev/f3cd65f62168
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.