Nathan Froyd [:froydnj] Assignee
	Description • 13 years ago

+++ This bug was initially created as a clone of Bug #723802 +++

Is it necessary to call accumulate with a false value for boolean telemetry when we just want to know how often something happens?  From what I can tell it should be enough to just report the interesting (true) case and then compare that to the number of telemetry reports for the time period.  This is what Telemetry::A11Y_INSTANTIATED currently does but it leads to useless bar graphs showing 100% of reporters with it on without showing the total number of reports that didn't specify a value.  It seems like boolean telemetry probes should have a default value.  The advantage of only reporting in the true case is that it has no perf. impact.

	(dormant account)
	Comment 1 • 13 years ago

I think we should do this on the serverside instead.

	Justin Lebar (not reading bugmail)
	Comment 2 • 13 years ago

> The advantage of only reporting in the true case is that it has no perf. impact.

Have you measured a perf impact of accumulating into a histogram?

	Matthew N. [:MattN]
	Comment 3 • 13 years ago

(In reply to Taras Glek (:taras) from comment #1)
> I think we should do this on the serverside instead.

That's possible and why I didn't file a bug for it in the first place.  I wanted to know if that's something that will get fixed on the server.  The problem is that the server side currently doesn't know for sure if a boolean will be used just for a true or false value or whether it's reporting both.  I guess they can just check if 100% of the reports are for one side and then assume it should be compared to the total reports?  This only works if you are looking only at data for a browser that reports that telemetry probe, otherwise it'll skew the data.

(In reply to Justin Lebar [:jlebar] from comment #2)
> > The advantage of only reporting in the true case is that it has no perf. impact.
> 
> Have you measured a perf impact of accumulating into a histogram?

That's a quote from me btw.

Note that I wasn't saying there was a perf impact of accumulating, I was just saying that not accumulating has zero perf impact compared to not using telemetry.

	Nathan Froyd [:froydnj] Assignee
	Comment 4 • 13 years ago

(In reply to Matthew N. [:MattN] from comment #3)
> (In reply to Taras Glek (:taras) from comment #1)
> > I think we should do this on the serverside instead.
> 
> That's possible and why I didn't file a bug for it in the first place.  I
> wanted to know if that's something that will get fixed on the server.

File a bug for the server side or the client side, doesn't matter much.  If it's not a good idea, somebody will say so.

> The problem is that the server side currently doesn't know for sure if a boolean
> will be used just for a true or false value or whether it's reporting both.

This is a problem, yes.  We could probably just use the heuristic you proposed for the boolean histograms we have now, though.

	Justin Lebar (not reading bugmail)
	Comment 5 • 13 years ago

> The problem is that the server side currently doesn't know for sure if a boolean will be used just 
> for a true or false value or whether it's reporting both.  I guess they can just check if 100% of 
> the reports are for one side and then assume it should be compared to the total reports?

I don't think you should rely on this.  Take a jaunt through the Telemetry dashboard and you'll see a bunch of bogus reporter names.  I don't think it's unlikely that we have bogus values as well.  Just one bogus value out of potentially millions of pings could throw your report off.

	Nathan Froyd [:froydnj] Assignee
	Comment 7 • 13 years ago

Here's a proposed solution: let's have a boolean-esque histogram that actually starts with a 0 value and only lets people accumulate a single positive value, kind of like a toggleable flag.  Hence the name FlagHistogram.

I realize that it might be more conceptually elegant to have a separate entity for this sort of thing, a TelemetryFlag or somesuch.  But this is expedient from a code point of view, an existing client's point of view, and from a metrics point of view (histogram dashboard already built, etc.)

	Nathan Froyd [:froydnj] Assignee
	Comment 8 • 13 years ago

Happily, little code is needed on the telemetry side of things.

	Nathan Froyd [:froydnj] Assignee
	Comment 9 • 13 years ago

...and tests to make sure everything works correctly.

	(dormant account)
	Comment 10 • 13 years ago

Comment on attachment 602356 [details] [diff] [review]
part 1 - add FlagHistogram to ipc/

that's a neat hack. This better get documented well

	(dormant account)
	Comment 11 • 13 years ago

Comment on attachment 602356 [details] [diff] [review]
part 1 - add FlagHistogram to ipc/

+  Histogram::Accumulate(1, 1, zero_index);
I'm confused why you don't use 0 as a default value

	(dormant account)
	Comment 12 • 13 years ago

Comment on attachment 602357 [details] [diff] [review]
part 2 - add FlagHistogram support to telemetry

 * HISTOGRAM_FLAG - For storing single 0/1 values

That's confusing. You should say that this histogram can only contain a single value, ie count == 1.

Also this is kind of weird. This histogram gets instantiated the first time you assign or read from it...so then why have the default value?

I thought the point of these was to exist in every ping, which will allow you to look at stuff like marketshare of l10n.

	(dormant account)
	Comment 13 • 13 years ago

Comment on attachment 602359 [details] [diff] [review]
part 3 - tests for FlagHistogram

+  // Should only switch counts once.
+  h.add(3);

Should this not throw an error?

	(dormant account)
	Comment 14 • 13 years ago

Comment on attachment 602357 [details] [diff] [review]
part 2 - add FlagHistogram support to telemetry

I'm pretty sure in order to work as intended you have to request all  HISTOGRAM_FLAG histograms to be created the first time histogramSnapshots is called.

	Nathan Froyd [:froydnj] Assignee
	Comment 15 • 13 years ago

(In reply to Taras Glek (:taras) from comment #11)
> +  Histogram::Accumulate(1, 1, zero_index);
> I'm confused why you don't use 0 as a default value

I am using 0 as a default value; I compute the index/bucket 0 goes in--that's zero_index--and stick a 1 there.

The inverse might be useful: default to 1 and get flipped to 0.  But I suppose you can have a FOO_DISABLED histogram to capture that semantic.

(In reply to Taras Glek (:taras) from comment #13)
> +  // Should only switch counts once.
> +  h.add(3);
> 
> Should this not throw an error?

I am ambivalent about this.  Doing that would require modifying all the Histogram subclasses to have their Add methods (and Accumulate methods, and SampleSet::Accumulate) return a status code.  I can see the value in doing this and am happy to do it if need be.

(In reply to Taras Glek (:taras) from comment #14)
> I'm pretty sure in order to work as intended you have to request all 
> HISTOGRAM_FLAG histograms to be created the first time histogramSnapshots is
> called.

Ooo, good point.  I'll do that and add a test.

I wonder how badly that would screw with the metrics side of things though; before you could assume that the histogram data was increasing only.  Now we might have these crazy histograms with one set of values one ping and a totally different (non-superset) set next ping.

	(dormant account)
	Comment 16 • 13 years ago

(In reply to Nathan Froyd (:froydnj) from comment #15)

> I wonder how badly that would screw with the metrics side of things though;
> before you could assume that the histogram data was increasing only.  Now we
> might have these crazy histograms with one set of values one ping and a
> totally different (non-superset) set next ping.

I'm not sure what you mean.

	Nathan Froyd [:froydnj] Assignee
	Comment 17 • 13 years ago

Updated patch, creating flag histograms automagically from addons and "main" telemetry, and fixing a couple of thinkos.

	Nathan Froyd [:froydnj] Assignee
	Comment 18 • 13 years ago

Updated tests for addons.

	(dormant account)
	Comment 19 • 13 years ago

Comment on attachment 602932 [details] [diff] [review]
part 2 - add FlagHistogram support to telemetry

+  printf("Getting histogram %s with type %u\n", name, histogramType);

^ I doubt you meant to put that in.

+  printf("Got histogram %s with type %u!\n", name, histogramType);
   return NS_OK;

+      nsresult rv = GetHistogramByEnumId(Telemetry::ID(i), &h);
+      if (NS_FAILED(rv)) {
+        return rv;
+      }

This is a bit scary. Having one ill-specified flag histogram will bust our entire telemetry reporting. An assert would work better.

I'll trust you on addon histogram stuff(that never fails, right?).

	Nathan Froyd [:froydnj] Assignee
	Comment 20 • 13 years ago

(In reply to Taras Glek (:taras) from comment #19)
> +  printf("Getting histogram %s with type %u\n", name, histogramType);
> 
> ^ I doubt you meant to put that in.

Oops, indeed.

> +      nsresult rv = GetHistogramByEnumId(Telemetry::ID(i), &h);
> +      if (NS_FAILED(rv)) {
> +        return rv;
> +      }
> 
> This is a bit scary. Having one ill-specified flag histogram will bust our
> entire telemetry reporting. An assert would work better.

GetHistogramByEnumId doesn't do any real checking of flag histogram parameters; we can make the static asserts for boolean histograms do the same checks for flag histograms, though.

So maybe there's no point in checking the return value here.

> I'll trust you on addon histogram stuff(that never fails, right?).

The addon histogram stuff at least has tests.  ^_^

	Nathan Froyd [:froydnj] Assignee
	Comment 21 • 13 years ago

Debugging code removed.  We now only complain about finding flag histograms in debug mode with an assert.

	(dormant account)
	Comment 22 • 13 years ago

Comment on attachment 602933 [details] [diff] [review]
part 3 - tests for FlagHistogram

You need a test in test_TelemetryPing to see that a FLAG histogram is being registered with a default 0 value

	Nathan Froyd [:froydnj] Assignee
	Comment 23 • 13 years ago

There was a bug in FlagHistogram; the bucket ranges are not initialized in the constructor, but rather by explicit calls in FactoryGet.  So the initial bucketing of 0 was totally bogus.  I am unsure of how the nsITelemetry tests didn't catch this...

	Nathan Froyd [:froydnj] Assignee
	Comment 24 • 13 years ago

Add HISTOGRAM_FLAG macro and check for HISTOGRAM_FLAG in the static checking bits.

	Nathan Froyd [:froydnj] Assignee
	Comment 25 • 13 years ago

Add TelemetryPing test to make sure we automagically instantiate flag histograms.  Also add a little extra checking for sane sums to the nsITelemetry tests.

	Mozilla RelEng Bot
	Comment 26 • 13 years ago

Autoland Patchset:
	Patches: 603454, 603455, 603456
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=edb3d7d29932
Try run started, revision edb3d7d29932. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=edb3d7d29932

	Nathan Froyd [:froydnj] Assignee
	Comment 27 • 13 years ago

Try showed that Windows and Android builds fail on histogram.cc because MOZ_ASSERT is not defined there (possibly Mac too; those builds aren't done yet).  Curious...

Using the moral equivalent, DCHECK_EQ instead.

	Mozilla RelEng Bot
	Comment 28 • 13 years ago

Try run for edb3d7d29932 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=edb3d7d29932
Results (out of 63 total builds):
    success: 47
    warnings: 7
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-edb3d7d29932
 Timed out after 06 hours without completing.

	Mozilla RelEng Bot
	Comment 29 • 13 years ago

Autoland Patchset:
	Patches: 603530, 603455, 603456
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=ac8c0c20a522
Try run started, revision ac8c0c20a522. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=ac8c0c20a522

	Nathan Froyd [:froydnj] Assignee
	Comment 30 • 13 years ago

Try run looks good, flagging for checkin.

	Mozilla RelEng Bot
	Comment 31 • 13 years ago

Try run for ac8c0c20a522 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ac8c0c20a522
Results (out of 217 total builds):
    success: 172
    warnings: 42
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-ac8c0c20a522

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae9d28fa8ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/1af7ce2bc6ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa0c9febd3d

	Ed Morley [:emorley]
	Comment 33 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/3ae9d28fa8ca
https://hg.mozilla.org/mozilla-central/rev/1af7ce2bc6ea
https://hg.mozilla.org/mozilla-central/rev/4fa0c9febd3d