1476609 - Display labels for categorical keyed histograms in about:telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Mathieu Leplatre [:leplatrem]]

Attachment: Screenshot from 2018-07-18 15-27-53.png

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

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.

Comment 2

[Mathieu Leplatre [:leplatrem]]

I'm OK with both C++ and JS, if you give me some pointers ;)

Thanks!

Comment 3

[Georg Fritzsche [:gfritzsche]]

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.

Comment 4

[Arash Fotouhi]

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?

Comment 5

[Chris H-C :chutten]

Yes, please go ahead.

Comment 6

[Arash Fotouhi]

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?

Comment 7

[Jan-Erik Rediger [:janerik]]

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.

Comment 8

[Arash Fotouhi]

Attachment: Bug 1476609 - Add display labelsfor categorical keyed histograms - r=janerik,chutten,gfritzsche

Comment 9

[Arash Fotouhi]

Attachment: Bug 1476609 - Add display labels for categorical keyed histograms - r=janerik,chutten

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1381c5099e1
Add display labels for categorical keyed histograms - r=chutten

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a1381c5099e1