1347216 - Fix categorical keyed histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Mathieu Leplatre [:leplatrem]]

I defined the following histogram:

    "UPDATE_REMOTE_SETTINGS_RESULT_1": {
      "expires_in_version": "never",
      "kind": "categorical",
      "keyed": true,
      "labels": ["up_to_date", "success", "backoff", "unknown_error", "network_error", "sync_error", "sync_conflict", "sign_error", "sign_retry_error", "application_error"],
      "releaseChannelCollection": "opt-out",
      "alert_emails": ["[e-mail]"],
      "bug_numbers": [1254099],
      "description": "Result for update of remote settings."
    },

When I try to use it, like this:

    Components.utils.import("resource://gre/modules/Services.jsm");
    
    const ID = "UPDATE_REMOTE_SETTINGS_RESULT_1";
    const KEY = "polling-changes";
    const VALUE = "success";
    const h = Services.telemetry.getKeyedHistogramById(ID);
    h.add(KEY, VALUE);

I have the following error: "Not a number" (and the snapshot remains empty).

Submitting the value with the label index (like I would do to introspect snapshots) works though:

    h.add(KEY, 0);

And the snapshot is well incremented in ``h.snapshot(KEY).counts[0]``.

How should we use the categorical keyed histogram? With the label value or its index in the enumerated labels list? Also is there a way to have the labels as keys in snapshots? 

Thanks for your insights!


Notes: In https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITelemetry#getKeyedHistogramById() it clearly specifies: ``add(key, [optional] value) - Adds an integer value to the appropriate bucket.``

Comment 1

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1347216 - Allow label values on keyed categorical histograms

Review commit: https://reviewboard.mozilla.org/r/120530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120530/

Comment 2

[Mathieu Leplatre [:leplatrem]]

With the attached patch in keyed histograms, submitting with ``add(key, label)`` now works and our tests pass :)

Don't hesitate to drop it and rewrite it your way if you prefer!

Comment 3

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8847553 [details]
Bug 1347216 - Allow label values on keyed categorical histograms

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120530/diff/1-2/

Comment 4

[Mathieu Leplatre [:leplatrem]]

Folks, do you have feedback on the bug/patch? 
Thanks!

Comment 5

[Georg Fritzsche [:gfritzsche]]

Sorry for the lag, we are still catching up on our backlog.
Alessio will look into this today.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8847553 [details]
Bug 1347216 - Allow label values on keyed categorical histograms

https://reviewboard.mozilla.org/r/120530/#review125468

This looks good overall, thank you for the patch and sorry for the delay!

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1726
(Diff revision 2)
>        LogToBrowserConsole(nsIScriptError::errorFlag,
>                            NS_LITERAL_STRING("Expected two arguments for this histogram type"));
>        return true;
>      }
>  
> +    if (args[1].isString() &&

Should this also be checking for *type == base::LinearHistogram::LINEAR_HISTOGRAM* as done for the non-keyed part?

::: toolkit/components/telemetry/TelemetryHistogram.cpp:1730
(Diff revision 2)
> +      nsAutoJSString label;
> +      if (!label.init(cx, args[1])) {
> +        LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid string parameter"));
> +        return true;
> +      }
> +      nsresult rv = gHistograms[id].label_id(NS_ConvertUTF16toUTF8(label).get(), &value);
> +      if (NS_FAILED(rv)) {
> +        LogToBrowserConsole(nsIScriptError::errorFlag,
> +                            NS_LITERAL_STRING("Unknown label for categorical histogram"));
> +        return true;
> +      }

nit: since this is basically the same code of the non-keyed version, I wonder if we could share the logic

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:607
(Diff revision 2)
> +    // Via label or index.
> +    for (let v of ["CommonLabel", "Label2", "Label3", "Label3", 0, 0, 1]) {
> +      h.add(k, v);
> +    }
> +  }
> +  // Categorical histograms default to 50 linear buckets.

Could you please also check that the API does not throw with unexpected/unknown labels? See test_categorical_histogram for how to do that.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js:613
(Diff revision 2)
> +  let expectedRanges = [];
> +  for (let i = 0; i < 51; ++i) {
> +    expectedRanges.push(i);
> +  }
> +
> +  for(let k of KEYS) {

Please also check that the snapshot of the whole histogram *only* contains the keys you expect (the ones in KEYS).

Comment 7

[Georg Fritzsche [:gfritzsche]]

This is probably more efficiently solved by us, we'll look at who later.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix categorical keyed histograms

This also fixes an existing bug where we checked for the wrong type in JSKeyedHistogram_Add().
KeyedHistogram::GetHistogramType() returns one of the histogram types from nsITelemetry.idl, not one of the chromium types (base::*).

I extended the test coverage accordingly, as it seemed to be lacking.

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8855792 [details] [diff] [review]
Fix categorical keyed histograms

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

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1729,5 @@
>  
>    // If we don't have an argument for the count histogram, assume an increment of 1.
>    // Otherwise, make sure to run some sanity checks on the argument.
> +  uint32_t value = 1;
> +  if ((type != nsITelemetry::HISTOGRAM_COUNT) || (args.length() == 2)) {

Great catch!

Comment 10

[Georg Fritzsche [:gfritzsche]]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc95735e66bf
Fix categorical keyed histograms. r=Dexter

Comment 12

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

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