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?

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

(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.

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

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": { > // ...

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

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.

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.

Missing: * about:telemetry integration, up for a separate patch or, if acceptable, a follow-up bug * maybe some test coverage paths?

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"

(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

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.

Unbreak tests from Histogram.json changes in 34.

Comment on attachment 8514404 [details] [diff] [review] Add keyed histograms Moving approval requests to rebased patches for Beta and 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.

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?

(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.

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.