Closed Bug 1347216 Opened 3 years ago Closed 2 years ago

Fix categorical keyed histograms

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: leplatrem, Assigned: gfritzsche)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 1 obsolete file)

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": ["storage-team@mozilla.com"],
      "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.``
Blocks: 1254099
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!
Folks, do you have feedback on the bug/patch? 
Thanks!
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
Sorry for the lag, we are still catching up on our backlog.
Alessio will look into this today.
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
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).
Attachment #8847553 - Flags: review?(alessio.placitelli)
Assignee: nobody → mathieu
Priority: -- → P1
Whiteboard: [measurement:client]
This is probably more efficiently solved by us, we'll look at who later.
Assignee: mathieu → nobody
Priority: P1 → P2
Priority: P2 → P1
Points: --- → 2
Summary: Usage of categorical keyed histograms → Fix categorical keyed histograms
Assignee: nobody → gfritzsche
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.
Attachment #8855792 - Flags: review?(alessio.placitelli)
Attachment #8847553 - Attachment is obsolete: true
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!
Attachment #8855792 - Flags: review?(alessio.placitelli) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc95735e66bf
Fix categorical keyed histograms. r=Dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cc95735e66bf
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.