Closed Bug 1476609 Opened 3 years ago Closed 11 months ago

Display labels for categorical keyed histograms in about:telemetry

Categories

(Toolkit :: Telemetry, enhancement, P3)

61 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: leplatrem, Assigned: arash9k, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good next bug][lang=c++])

Attachments

(2 files, 1 obsolete file)

We have a categorical keyed histogram UPTAKE_REMOTE_CONTENT_RESULT_1 [0]

We use it for Normandy, RemoteSettings etc [1]. The labels are shown correctly on telemetry.m.o [2]

But if I go to about:telemetry, in Keyed Histogram, the histograms values are integer values instead of the labels. See attached screenshot :)

If it's a small fix and you're willing to guide me through it, I'd be glad to prepare a patch :) 

Thanks!


Note: in order to trigger a RemoteSettings synchronization manually, you can do this:

   const { RemoteSettings } = ChromeUtils.import("resource://services-settings/remote-settings.js", {});
   RemoteSettings.pollChanges();


[0] https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/toolkit/components/telemetry/Histograms.json#5631-5641

[1] https://firefox-source-docs.mozilla.org/main/latest/services/common/services/RemoteSettings.html#uptake-telemetry

[2] https://mzl.la/2L1MI6q
See Also: → 1476624
This will require:
- a bit of C++ to expose the label strings to JavaScript, to use them in about:telemetry
- a bit of JavaScript in about:telemetry to display the labels

Mathieu, are you interested to look into only the JS or also the C++?
Either way, me and Jan-Erik can help through this and it is not a big change.
Priority: -- → P3
Flags: needinfo?(mathieu)
Mentor: jrediger, gfritzsche
I'm OK with both C++ and JS, if you give me some pointers ;)

Thanks!
Flags: needinfo?(mathieu)
Ok, nice.
So, we're storing the label strings for the categorical histograms in C++.
We'll need to add a function on nsITelemetry to get them out from C++ to JS, then from JS display them in about:telemetry.

### Getting the labels in C++

For C++, the main code goes into TelemetryHistogram.cpp. The histogram descriptions are stored in gHistogramInfos (read-only data generated at build time). From the new function, you can:
- Run through gHistogramInfos:
  https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/TelemetryHistogram.cpp#844
- Continue if `.histogramType` is not categorical.
- Look up the labels through the tables.
- Add them to a collection.
- Serialize the collection to JS objects.

The labels are stored in gHistogramStringTable. This contains histogram strings with no duplicates, so you need to use the offsets from gHistogramLabelTable, which are stored in the histogram descriptions:
https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/TelemetryHistogram.cpp#524

Here is an example of a C++ function that is called from JS and returns an object to JS (you don't need the locking etc. because you're only accessing read-only Telemetry storage):
https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/TelemetryScalar.cpp#2950

TelemetryEvents.cpp also has some clean examples of serializing arrays to JS:
https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/TelemetryEvent.cpp#601

I assume we'll return an object of the format:
{
  "histogram id": ["label 1", "label 2", ...],
  "histogram id 2": ...,
  ...
}

### Exposing this to JS

- Add a new method to nsITelemetry, similar to this one:
  https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/nsITelemetry.idl#72
- This gets plumbed through Telemetry.cpp:
  https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/Telemetry.cpp#630
- Then calls the implementing function in TelemetryHistogram.cpp:
  https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/TelemetryHistogram.cpp#2437

You can add a basic test for this:
- Add the test code around here:
  https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js#242
- You can use our test histograms to test for the right labels being serialized:
  https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/components/telemetry/Histograms.json#7256

### about:telemetry

- Histograms are rendered here:
  https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/toolkit/content/aboutTelemetry.js#1041
- You can now call `Telemetry.getCategoricalHistogramLabels()` or so to get the label strings.
- ... let's check into that part later.
Assignee: nobody → mathieu
Assignee: mathieu → nobody
Mentor: gfritzsche
Whiteboard: [good next bug][lang=c++]

Hi - I would like to work on this bug. I have experience with C++ but no experience with JS. Should I go ahead and follow instructions in comment 3?

Yes, please go ahead.

So I have some very basic, extremely preliminary code (nowhere near complete), but want to know if I'm on the right track. What is the best way to get some feedback? Submit a patch even though it's not complete?

Yes, please do submit it as a patch. Preliminary code is fine and we're happy to guide you through to the finish line from there.

Assignee: nobody → arash9k
Status: NEW → ASSIGNED
Attachment #9143395 - Attachment is obsolete: true
Attachment #9144233 - Attachment description: Bug 1476609 - Add display labelsfor categorical keyed histograms - r=janerik,chutten → Bug 1476609 - Add display labels for categorical keyed histograms - r=janerik,chutten
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1381c5099e1
Add display labels for categorical keyed histograms - r=chutten
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79
Blocks: 1645836
You need to log in before you can comment on or make changes to this bug.