Add keyed histogram types

RESOLVED FIXED in Firefox 34

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

({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)
(Assignee)

Updated

5 years ago
Depends on: 1069880
(Assignee)

Updated

5 years ago
No longer depends on: 1069880

Comment 1

5 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.
(Assignee)

Comment 2

5 years ago
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

5 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)
(Assignee)

Comment 5

5 years ago
(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

5 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)
(Assignee)

Comment 7

4 years ago
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)
(Assignee)

Updated

4 years ago
Blocks: 1089670
(Assignee)

Updated

4 years ago
Assignee: nobody → georg.fritzsche
(Assignee)

Updated

4 years ago
Summary: Add histogram type: keyed counter → Add keyed histogram types
(Assignee)

Comment 9

4 years ago
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)
(Assignee)

Comment 10

4 years ago
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+
(Assignee)

Comment 12

4 years ago
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+
(Assignee)

Comment 14

4 years ago
(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;
(Assignee)

Updated

4 years ago
Blocks: 1092176
(Assignee)

Updated

4 years ago
Depends on: 1092219
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7dd2bbd2847d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Comment 19

4 years ago
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?
(Assignee)

Comment 20

4 years ago
Created attachment 8515293 [details] [diff] [review]
Rebased for Aurora - Add keyed histograms.
Attachment #8515293 - Flags: review+
(Assignee)

Comment 21

4 years ago
Created attachment 8515326 [details] [diff] [review]
Rebased for Beta - Add keyed histograms.
Attachment #8515326 - Flags: review+
(Assignee)

Comment 22

4 years ago
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
(Assignee)

Comment 26

4 years ago
(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)
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+
(Assignee)

Comment 29

4 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/3fe1e43c97b8
status-firefox34: affected → fixed
status-firefox35: affected → fixed
Flags: needinfo?(jorge)
Keywords: addon-compat
(Assignee)

Updated

4 years ago
Depends on: 1094035
(Assignee)

Updated

4 years ago
Blocks: 1096785
You need to log in before you can comment on or make changes to this bug.