If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add keyed histogram types

RESOLVED FIXED in Firefox 34

Status

()

Toolkit
Telemetry
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

(Blocks: 1 bug, {addon-compat})

unspecified
mozilla36
addon-compat
Points:
5
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 wontfix)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Comment hidden (empty)
Depends on: 1069880
No longer depends on: 1069880

Comment 1

3 years ago
Do you want to limit this to a keyed *counter*, or make it possible for any histogram type to be keyed? If it's not too much extra work, the latter sounds a lot more flexible later on, since it can be used for things like addon performance measurements.
I was planning on keyed counters specifically at this point, but a more general keyed histogram sounds useful.
Vladan, what do you think about this?
Flags: needinfo?(vdjeric)

Updated

3 years ago
Blocks: 1071880
This would be an awesome feature!
(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> I was planning on keyed counters specifically at this point, but a more
> general keyed histogram sounds useful.
> Vladan, what do you think about this?

I think there's a real need for counter histograms. Some people are already using boolean histograms that way, e.g. NEWTAB_PAGE_SHOWN histogram for the number of times a page was shown.
However boolean histograms are inadequate. The counter histograms should behave like flag histograms: a counter histogram should be submitted even if the counter was never incremented (i.e. count=0).

I do believe there's a need for a key-value Telemetry type. We already use JS objects in this fashion for storing "simple measurements" and system information. We use a similar, but slightly more complex format for storing add-on performance details and slowSQL. You can see this data in your about:telemetry page.

So I think that we should we implement both a histogram counter type and also a means for storing key/value Telemetry data. I don't think we should implement the key-value histogram type on top of the existing Chromium histogram.cc code, and we also should not store the keyed-histograms in the existing Histograms section of the Telemetry ping.

Note that Irving already sort of tried doing something similar in Bug 846921:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#1838
Flags: needinfo?(vdjeric)
(In reply to Vladan Djeric (:vladan) from comment #4)
> I think there's a real need for counter histograms. Some people are already
> using boolean histograms that way, e.g. NEWTAB_PAGE_SHOWN histogram for the
> number of times a page was shown.
> However boolean histograms are inadequate. The counter histograms should
> behave like flag histograms: a counter histogram should be submitted even if
> the counter was never incremented (i.e. count=0).

Counter histograms landed in bug 1069873. I initially always submitted them, but dropped it per Nathans review in bug 1069873, comment 11.
Are we missing any important data here?

> I do believe there's a need for a key-value Telemetry type. We already use
> JS objects in this fashion for storing "simple measurements" and system
> information. We use a similar, but slightly more complex format for storing
> add-on performance details and slowSQL. You can see this data in your
> about:telemetry page.
> 
> So I think that we should we implement both a histogram counter type and
> also a means for storing key/value Telemetry data. I don't think we should
> implement the key-value histogram type on top of the existing Chromium
> histogram.cc code, and we also should not store the keyed-histograms in the
> existing Histograms section of the Telemetry ping.

We definitely need "keyed counter" & "keyed exponential" right now, for which we could directly re-use the histograms.
Do you think we need a more general & free-form key-value store here?
Either way, i think we should have a bounded set of value-types (say KEYED_COUNT, KEYED_EXPONENTIAL, ..., maybe KEYED_STRING?) so we wouldn't need custom display/evaluation/... handling for each.
Flags: needinfo?(vdjeric)

Updated

3 years ago
Blocks: 1070755
After clarifying some things on IRC, I'm on board with this. Georg, needinfo me again when you come up with the API + JSON representation of the histogram data.

I like the representation you described on IRC:

14:51 < gfritzsche> vladan: or just re-use histograms as in { "Bing": ...histogramData
14:52 < gfritzsche> vladan: which would mean we could just do (ID,key)->histogram mappings
14:53 <@vladan> gfritzsche: in that alternate implementation you just described, you're basically proposing sub-histograms ("BING_SEARCHES", "GOOGLE_SEARCHES") nested within a parent histogram ("SEARCH_USES")?
14:53 < gfritzsche> vladan: exactly
14:53 < irving> really it's a dynamically named counter, like we support inside simple measurements
Flags: needinfo?(vdjeric)
So, this is the proposal sketched out a little more clearly. Vladan, is that ok with you?

I propose to add the following to nsITelemetry:

> jsval newKeyedHistogram(in ACString name,
>                         in ACString expiration,
>                         in unsigned long histogram_type,
>                         [optional] in uint32_t min,
>                         [optional] in uint32_t max,
>                         [optional] in uint32_t bucket_count);
> 
> jsval getKeyedHistogramById(in ACString id);

Those return JS objects with the following functions:
* add(string key, [optional] int) - Add an int value to the histogram for that key. If no histogram for that key exists yet, it is created.
* snapshot([optional] string key) - If key is provided, returns a snapshot for the histogram with that key or null. If key is not provided, returns the snapshots of all the registered keys in the form {key1: snapshot1, key2: snapshot2, ...}.
* keys() - Returns an array with the string keys of the currently registered histograms
* clear() - Clears the registered histograms from this.

By keeping things clearly separate from the existing histograms, this will hopefully cause less confusion.
If we find we need to clone keyed histograms too or there is a need to expose this for addons as well, we can still add it then.


For the JSON representation, i think we should an additional section "keyedHistograms" to the payload.
This would contain an entry for each keyed histogram, with a key-to-histogram entry for every registered key, e.g.:

> "keyedHistograms": {
>   "SEARCH_COUNTS": {
>     "bing": {
>       // ... usual histogram data - range, bucket_count, histogram_type, ...
>     },
>     "google": {
>       // ... histogram data
>     },
>   },
>   "NPPNEWSTREAM_DURATION": {
>     "flash": {
>       // ...
Flags: needinfo?(vdjeric)
This looks fine.

Btw, don't forget to update the Telemetry probe wiki page with your new counter & keyed-histogram types:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
Flags: needinfo?(vdjeric)
Blocks: 1089670
Assignee: nobody → georg.fritzsche
Summary: Add histogram type: keyed counter → Add keyed histogram types
Created attachment 8513831 [details] [diff] [review]
WIP, missing the ping payload integration

WIP: I didn't get the payload/persistence integration done yet and testing is not complete.
I would however appreciate feedback at this point to integrate it tomorrow.
Please ignore the dump()s & printf()s.
Attachment #8513831 - Flags: feedback?(nfroyd)
Potential open points:

* We didn't talk about the Histogram.json integration before - i just went with adding a "keyed" attribute

* The way keyed histograms are integrated Telemetry.cpp requires slightly awkward checks on some "normal histograms" paths. The alternative though would be more some duplicated code (or a rather large refactoring?).
Comment on attachment 8513831 [details] [diff] [review]
WIP, missing the ping payload integration

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

This all looks pretty reasonable, modulo cleaning up printfs and whatnot.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1033,5 @@
>    case nsITelemetry::HISTOGRAM_COUNT:
>      *result = CountHistogram::FactoryGet(name, Histogram::kUmaTargetedHistogramFlag);
>      break;
>    default:
> +    printf("*** HistogramGet() - invalid type\n");

This is actually a good place for the non-fatal NS_ASSERTION.
Attachment #8513831 - Flags: feedback?(nfroyd) → feedback+
Created attachment 8514404 [details] [diff] [review]
Add keyed histograms

Missing:
* about:telemetry integration, up for a separate patch or, if acceptable, a follow-up bug
* maybe some test coverage paths?
Attachment #8513831 - Attachment is obsolete: true
Attachment #8514404 - Flags: review?(nfroyd)
Comment on attachment 8514404 [details] [diff] [review]
Add keyed histograms

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +2851,5 @@
>    return NS_OK;
>  }
>  
> +bool
> +GetRegisteredHistogramIds(bool keyed, JSContext* cx, JS::MutableHandleValue ret)

Where does the |keyed| parameter get used here?

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +435,5 @@
>      return ret;
>    },
>  
> +  getKeyedHistograms: function() {
> +    let snapshots = Telemetry.keyedHistogramSnapshots;

This is a little odd, that you're not using registeredKeyedHistograms...is the plan for people to be making up keyed histograms on their own, rather than using pre-defined ones from Histograms.json?

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +135,5 @@
>     * Returns an array whose values are the names of histograms defined
>     * in Histograms.json.
>     */
> +  [implicit_jscontext]
> +  jsval registeredHistograms();

Ugh, we really want to move to less jsval usage in this API, not more. :(

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +511,5 @@
> +  threw = false;
> +  try {
> +    Telemetry.getKeyedHistogramById("test::unknown histogram", "never", Telemetry.HISTOGRAM_BOOLEAN);
> +  } catch (e) {
> +    // This should throw as it an unknown ID

Nit: "as it is an unknown ID"
Attachment #8514404 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #13)
> ::: toolkit/components/telemetry/Telemetry.cpp
> @@ +2851,5 @@
> >    return NS_OK;
> >  }
> >  
> > +bool
> > +GetRegisteredHistogramIds(bool keyed, JSContext* cx, JS::MutableHandleValue ret)
> 
> Where does the |keyed| parameter get used here?

Oops, that is supposed to be a filtering criteria.

> ::: toolkit/components/telemetry/TelemetryPing.jsm
> @@ +435,5 @@
> >      return ret;
> >    },
> >  
> > +  getKeyedHistograms: function() {
> > +    let snapshots = Telemetry.keyedHistogramSnapshots;
> 
> This is a little odd, that you're not using registeredKeyedHistograms...is
> the plan for people to be making up keyed histograms on their own, rather
> than using pre-defined ones from Histograms.json?

Ah, should we limit the payload to what is defined in Histograms.json? I missed that that was the intention.

> ::: toolkit/components/telemetry/nsITelemetry.idl
> @@ +135,5 @@
> >     * Returns an array whose values are the names of histograms defined
> >     * in Histograms.json.
> >     */
> > +  [implicit_jscontext]
> > +  jsval registeredHistograms();
> 
> Ugh, we really want to move to less jsval usage in this API, not more. :(

Ok, i'm happy to improve this with suggestions. We should not keep the "char***" out" approach though, because with how things changed apparently we'd now have to
* run through gHistogram, collecting keyed/non-keyed into a temp buffer
* now that we know the size, allocate a buffer
* clone things into that buffer
(In reply to Georg Fritzsche [:gfritzsche] from comment #14)
> > ::: toolkit/components/telemetry/TelemetryPing.jsm
> > @@ +435,5 @@
> > >      return ret;
> > >    },
> > >  
> > > +  getKeyedHistograms: function() {
> > > +    let snapshots = Telemetry.keyedHistogramSnapshots;
> > 
> > This is a little odd, that you're not using registeredKeyedHistograms...is
> > the plan for people to be making up keyed histograms on their own, rather
> > than using pre-defined ones from Histograms.json?
> 
> Ah, should we limit the payload to what is defined in Histograms.json? I
> missed that that was the intention.

Well, that's what we do for normal histograms, I was assuming that we'd do the same for keyed histograms as well.

> > ::: toolkit/components/telemetry/nsITelemetry.idl
> > @@ +135,5 @@
> > >     * Returns an array whose values are the names of histograms defined
> > >     * in Histograms.json.
> > >     */
> > > +  [implicit_jscontext]
> > > +  jsval registeredHistograms();
> > 
> > Ugh, we really want to move to less jsval usage in this API, not more. :(
> 
> Ok, i'm happy to improve this with suggestions. We should not keep the
> "char***" out" approach though, because with how things changed apparently
> we'd now have to
> * run through gHistogram, collecting keyed/non-keyed into a temp buffer
> * now that we know the size, allocate a buffer
> * clone things into that buffer

The |char*** out| approach is dictated by XPConnect, so I don't think we're going to improve on that much.  And the current RegisteredHistograms implementation wastes space on expired histograms already, so we could just allocate |ArrayLength(gHistograms)| for both finding both keyed and non-keyed histograms.  (I *think* that memory is short-lived anyway and XPConnect will free it once it's converted the array into JS objects.)

I think you could just do a more space-efficient approach with a single loop, though:

  nsTArray<char*> collection;
  for (size_t i = 0; i < ArrayLength(gHistograms); ++i) {
    if (gHistograms[i] matches condition) {
      const char* id = gHistograms[i].id();
      size_t len = strlen(id);
      collection.AppendElement(nsMemory::clone(id, len+1));
    }
  }

  size_t bytes = collection.Length() * sizeof(char*);
  char** histograms = static_cast<char**>(nsMemory::Alloc(bytes));
  memcpy(histograms, collection.Elements(), bytes);
  *aCount = collection.Length();
  *aHistograms = histograms;
Thanks, updated per the above comments and pushed to try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f0ed0355608f
https://tbpl.mozilla.org/?tree=Try&rev=f0ed0355608f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dd2bbd2847d
Blocks: 1092176
Depends on: 1092219
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7dd2bbd2847d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8514404 [details] [diff] [review]
Add keyed histograms

Approval Request Comment
[Feature/regressing bug #]: Search telemetry.
[User impact if declined]:
[Describe test coverage new/current, TBPL]: Automated test-coverage.
[Risks and why]: Low to medium - good test coverage of existing code that this touches, so i'm optimistic.
[String/UUID change made/needed]: None.
Attachment #8514404 - Flags: approval-mozilla-beta?
Attachment #8514404 - Flags: approval-mozilla-aurora?
Created attachment 8515293 [details] [diff] [review]
Rebased for Aurora - Add keyed histograms.
Attachment #8515293 - Flags: review+
Created attachment 8515326 [details] [diff] [review]
Rebased for Beta - Add keyed histograms.
Attachment #8515326 - Flags: review+
Created attachment 8515645 [details] [diff] [review]
Rebased for Beta - Add keyed histograms.

Unbreak tests from Histogram.json changes in 34.
Attachment #8515326 - Attachment is obsolete: true
Attachment #8515645 - Flags: review+
Comment on attachment 8514404 [details] [diff] [review]
Add keyed histograms

Moving approval requests to rebased patches for Beta and Aurora.
Attachment #8514404 - Flags: approval-mozilla-beta?
Attachment #8514404 - Flags: approval-mozilla-aurora?
Comment on attachment 8515293 [details] [diff] [review]
Rebased for Aurora - Add keyed histograms.

Note that there is an UUID change in here but this is fine for Aurora.
Attachment #8515293 - Flags: approval-mozilla-aurora+
Comment on attachment 8515645 [details] [diff] [review]
Rebased for Beta - Add keyed histograms.

Is there a way to add keyed histograms without an UUID change? If not, how do you expect this UUID change to impact add-on?
Flags: needinfo?(georg.fritzsche)
Attachment #8515645 - Flags: approval-mozilla-beta?
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → fixed
status-firefox-esr31: --- → wontfix
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #25)
> Is there a way to add keyed histograms without an UUID change? If not, how
> do you expect this UUID change to impact add-on?

I don't see an easy way to change this, see bug 1069953, comment 20 for details.
Flags: needinfo?(georg.fritzsche)
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f7b1b50cbd7
Comment on attachment 8515645 [details] [diff] [review]
Rebased for Beta - Add keyed histograms.

Approving for Beta as per bug 1069953 comment 20 this UUID bump is not expected to have any impact on add-ons.

Jorge - Adding ni on you for awareness in case you want to communicate the Telemetry related changes to add-on authors.
Flags: needinfo?(jorge)
Attachment #8515645 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/3fe1e43c97b8
status-firefox34: affected → fixed
status-firefox35: affected → fixed
Flags: needinfo?(jorge)
Keywords: addon-compat
Depends on: 1094035
Blocks: 1096785
Blocks: 1124073
Blocks: 1205898
You need to log in before you can comment on or make changes to this bug.