This should be basically done, but i'm running into |uneval(payload.histograms[TELEMETRY_TEST_COUNT])| being |(void 0)| and haven't quite tracked it down yet: > test_TelemetryPing.js:249 | "(void 0)" == "({range:[1, 2], bucket_count:3, histogram_type:4, values:{0:0, 1:0}, sum:0, sum_squares_lo:0, sum_squares_hi:0})" Also, weirdly this fails in test_TelemetryPing.js locally for me (but doesn't seem related to my changes): |do_check_true(payload.info.revision.startsWith("http"));|

(In reply to Georg Fritzsche [:gfritzsche] from comment #1) > This should be basically done, but i'm running into > |uneval(payload.histograms[TELEMETRY_TEST_COUNT])| being |(void 0)| and > haven't quite tracked it down yet: > > test_TelemetryPing.js:249 | "(void 0)" == "({range:[1, 2], bucket_count:3, histogram_type:4, values:{0:0, 1:0}, sum:0, sum_squares_lo:0, sum_squares_hi:0})" This may just mean that i always have to accumulate a value into CountHistograms initially... not sure.

(In reply to Georg Fritzsche [:gfritzsche] from comment #1) > Created attachment 8494654 [details] [diff] [review] > WIP > > This should be basically done, but i'm running into > |uneval(payload.histograms[TELEMETRY_TEST_COUNT])| being |(void 0)| and > haven't quite tracked it down yet: > > test_TelemetryPing.js:249 | "(void 0)" == "({range:[1, 2], bucket_count:3, histogram_type:4, values:{0:0, 1:0}, sum:0, sum_squares_lo:0, sum_squares_hi:0})" > > Also, weirdly this fails in test_TelemetryPing.js locally for me (but > doesn't seem related to my changes): > |do_check_true(payload.info.revision.startsWith("http"));| Are you using git? The bits to get the current revision assume that your srcdir is checked out with hg.

(In reply to Nathan Froyd (:froydnj) from comment #3) > > Also, weirdly this fails in test_TelemetryPing.js locally for me (but > > doesn't seem related to my changes): > > |do_check_true(payload.info.revision.startsWith("http"));| > > Are you using git? The bits to get the current revision assume that your > srcdir is checked out with hg. No git, all hg for me. Maybe i'm just missing something after updating Histogram.json & the python scripts & build?

Ok, traced down the remaining things. I still see the revision.startsWith("http") locally, but for now i assume that's not related to my patch. I'm not happy that the current histogram.cc code forces us to a) use multiple buckets and b) dummy initialize one bucket and count in the second one ... but rewriting that doesn't seem worth the effort/risk here. https://tbpl.mozilla.org/?tree=Try&rev=5932b7baeb3b

Another patch in that queue triggered failures: https://tbpl.mozilla.org/?tree=Try&rev=f7f9fce4f0d9

Comment on attachment 8495411 [details] [diff] [review] Add count histogram type Review of attachment 8495411 [details] [diff] [review]: ----------------------------------------------------------------- WDYT about giving counter histograms (and only counter histograms) an increment() method in JS, so we're not overloading add()? Just an idea, not something that need to be implemented. I'm a little unclear on the accumulation logic in histogram.cc, and the ergonomics of .add() for counting histograms, so just f+. ::: ipc/chromium/src/base/histogram.cc @@ +1062,5 @@ > + CountHistogram *fh = new CountHistogram(name); > + fh->InitializeBucketRange(); > + fh->SetFlags(flags); > + size_t zero_index = fh->BucketIndex(0); > + fh->LinearHistogram::Accumulate(0, 1, zero_index); Why are you starting with a count in the zero'th bucket? @@ +1082,5 @@ > + > +void > +CountHistogram::Accumulate(Sample value, Count count, size_t index) > +{ > + size_t one_index = BucketIndex(1); This seems at odds with the code in FactoryGet and AddSampleSet. @@ +1091,5 @@ > +CountHistogram::AddSampleSet(const SampleSet& sample) { > + DCHECK_EQ(bucket_count(), sample.size()); > + // We can't be sure the SampleSet provided came from another CountHistogram, > + // so we at least check if the zero bucket count matches and take no action > + // otherwise. This comment doesn't seem to match up with the code below. I expect, given the comment, for the code to look something like: if (zero bucket count matches) { // do something } // otherwise, take no action. @@ +1099,5 @@ > + } > + > + const size_t one_index = BucketIndex(1); > + if (sample.counts(one_index) != 0) { > + Accumulate(1, sample.counts(one_index), zero_index); So we're taking all the things from the first bucket of the other histogram and putting them in our zero'th bucket? Are we accumulating all things into our zero'th bucket, or our one'th (sic) bucket? ::: toolkit/components/telemetry/Telemetry.cpp @@ +935,4 @@ > } > + > + if (TelemetryImpl::CanRecord()) { > + h->Add((type != base::CountHistogram::COUNT_HISTOGRAM) ? value : 1); It seems better to me to initialize |value| to 1 above, then you don't need to duplicate COUNT_HISTOGRAM logic here. Though, really, since the logic in histogram.cc ignores whatever value you pass in here, I'm not sure why you can't just let: counting_histogram.add(1) be the appropriate way of updating things, and then you don't need to change anything in here. Or are you worried that people are going to do: counting_histogram.add($VALUE_LARGER_THAN_ONE); and then wonder why things are only going up by 1 in their histogram? ::: toolkit/components/telemetry/nsITelemetry.idl @@ +145,5 @@ > * @param expiration Expiration version > * @param min - Minimal bucket size > * @param max - Maximum bucket size > * @param bucket_count - number of buckets in the histogram. > + * @param type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR ISTOGRAM_BOOLEAN or HISTOGRAM_COUNT Nit: "...LINEAR, HISTOGRAM_BOOLEAN..."

(In reply to Nathan Froyd (:froydnj) from comment #7) > ::: ipc/chromium/src/base/histogram.cc > @@ +1062,5 @@ > > + CountHistogram *fh = new CountHistogram(name); > > + fh->InitializeBucketRange(); > > + fh->SetFlags(flags); > > + size_t zero_index = fh->BucketIndex(0); > > + fh->LinearHistogram::Accumulate(0, 1, zero_index); > > Why are you starting with a count in the zero'th bucket? So, i obviously missed expanding on the "why". Without this they are not showing up in the payload until something is .add()ed, this is similar to flag histograms. As we need this and also have >1 bucket anyway already, i just decided to accumulate the actual counts in the one-bucket. > @@ +1082,5 @@ > > + > > +void > > +CountHistogram::Accumulate(Sample value, Count count, size_t index) > > +{ > > + size_t one_index = BucketIndex(1); > > This seems at odds with the code in FactoryGet and AddSampleSet. Per above, this is intentional. > @@ +1091,5 @@ > > +CountHistogram::AddSampleSet(const SampleSet& sample) { > > + DCHECK_EQ(bucket_count(), sample.size()); > > + // We can't be sure the SampleSet provided came from another CountHistogram, > > + // so we at least check if the zero bucket count matches and take no action > > + // otherwise. > > This comment doesn't seem to match up with the code below. See above, this is right but i'm expanding the comment. > @@ +1099,5 @@ > > + } > > + > > + const size_t one_index = BucketIndex(1); > > + if (sample.counts(one_index) != 0) { > > + Accumulate(1, sample.counts(one_index), zero_index); > > So we're taking all the things from the first bucket of the other histogram > and putting them in our zero'th bucket? Are we accumulating all things into > our zero'th bucket, or our one'th (sic) bucket? Oops, fixed. > ::: toolkit/components/telemetry/Telemetry.cpp > @@ +935,4 @@ > > } > > + > > + if (TelemetryImpl::CanRecord()) { > > + h->Add((type != base::CountHistogram::COUNT_HISTOGRAM) ? value : 1); > > It seems better to me to initialize |value| to 1 above, then you don't need > to duplicate COUNT_HISTOGRAM logic here. > > Though, really, since the logic in histogram.cc ignores whatever value you > pass in here, I'm not sure why you can't just let: > > counting_histogram.add(1) > > be the appropriate way of updating things, and then you don't need to change > anything in here. Or are you worried that people are going to do: > > counting_histogram.add($VALUE_LARGER_THAN_ONE); > > and then wonder why things are only going up by 1 in their histogram? I don't like the idea of a) the always-dummy-argument of 1 b) allowing non-monotonic increments ... and just allowing .add() seemed like a convenient way out. Am i maybe overly worried here?

(In reply to Nathan Froyd (:froydnj) from comment #7) > WDYT about giving counter histograms (and only counter histograms) an > increment() method in JS, so we're not overloading add()? Just an idea, not > something that need to be implemented. Hm, this is probably better. I'm not sure if it's nice to look up / remember additional functions, but that probably stands out better/clearer than .add(1) or .add().

(In reply to Georg Fritzsche [:gfritzsche] from comment #4) > (In reply to Nathan Froyd (:froydnj) from comment #3) > > > Also, weirdly this fails in test_TelemetryPing.js locally for me (but > > > doesn't seem related to my changes): > > > |do_check_true(payload.info.revision.startsWith("http"));| > > > > Are you using git? The bits to get the current revision assume that your > > srcdir is checked out with hg. > > No git, all hg for me. Maybe i'm just missing something after updating > Histogram.json & the python scripts & build? Turns out i just missed the "official" parts here and filed bug 1073536 & bug 1073537 on this.

(In reply to Georg Fritzsche [:gfritzsche] from comment #8) > (In reply to Nathan Froyd (:froydnj) from comment #7) > > ::: ipc/chromium/src/base/histogram.cc > > @@ +1062,5 @@ > > > + CountHistogram *fh = new CountHistogram(name); > > > + fh->InitializeBucketRange(); > > > + fh->SetFlags(flags); > > > + size_t zero_index = fh->BucketIndex(0); > > > + fh->LinearHistogram::Accumulate(0, 1, zero_index); > > > > Why are you starting with a count in the zero'th bucket? > > So, i obviously missed expanding on the "why". > Without this they are not showing up in the payload until something is > .add()ed, this is similar to flag histograms. > As we need this and also have >1 bucket anyway already, i just decided to > accumulate the actual counts in the one-bucket. Ah, I understand what you're trying to do now. A histogram with no data in it should never be sent; that just wastes storage, bandwidth, processing on the server, etc. etc. Flag histograms are special, though: the flag not being set (i.e. nothing add()'dd to the histogram) *is* important data, so we always set something in the bucket. Not having add()'d anything to a counting histogram, though, is unimportant. That's just a count of zero, so we don't need to know about it. (I imagine sending that count of zero would also require special casing the histogram on the server side somehow for processing purposes...?) Please take this bit of code out and adjust the tests as necessary.

Removed dummy bucket init and fixed the other comments.

Comment on attachment 8496020 [details] [diff] [review] Add count histogram type, v2 Review of attachment 8496020 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js @@ +240,5 @@ > }; > let flag = payload.histograms[TELEMETRY_TEST_FLAG]; > do_check_eq(uneval(flag), uneval(expected_flag)); > > + // Count histograms should automagically spring to life. This comment is inaccurate. Please remove it.

Comment on attachment 8496020 [details] [diff] [review] Add count histogram type, v2 Approval Request Comment [Feature/regressing bug #]: Search telemetry. [User impact if declined]: [Describe test coverage new/current, TBPL]: Automated test-coverage etc. [Risks and why]: Low-risk, this was fine on Nightly and Aurora. [String/UUID change made/needed]: None.

Unbreak tests from Histogram.json changes in 34.

Comment on attachment 8496020 [details] [diff] [review] Add count histogram type, v2 I take it that approval is needed on the beta rebase patch and not this patch.

(In reply to Georg Fritzsche [:gfritzsche] from comment #16) > [String/UUID change made/needed]: None. This is incorrect. There is an UUID change in the patch. Is there a way to add the counter type without an UUID change? If not, do you know how this change may impact add-ons?

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #20) > This is incorrect. There is an UUID change in the patch. Is there a way to > add the counter type without an UUID change? If not, do you know how this > change may impact add-ons? Sorry about missing this (i was only thinking of strings...). I don't see an easy way to change this, see bug 1069953, comment 20 for details.

Comment on attachment 8515644 [details] [diff] [review] Beta rebase - Add count histogram type 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.