748907 - don't send no-data histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

Due to the way our persistent telemetry implementation works, we can have histograms instantiated that don't have any data in them.  We then send those histograms in (and possibly persist them, so we're doomed forevermore!).  This wastes space and time in many places.

We should fix this to only reflect/send histograms with actual data in them.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

Simple patch to do this.  Doing it at the JS level is easier than doing it at the C++ level and naturally handles normal pings and persisted pings.

I don't have good tests for this.  The added bits in test_TelemetryPing.js are half-hearted (and associated changes for sending addon histograms), as addon histograms don't get created until they have data anyway.  At least all the histograms that the tests are checking for *do* get sent, so we have some degree of confidence this won't totally bork our telemetry data.

Comment 2

[(dormant account)]

Comment on attachment 621045 [details] [diff] [review]
patch

I think the right way to do this is on the C++ side. That would work automatically for about:telemetry, etc

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: patch

Ended up being simpler on the C++ side.  Efficiency?  What's that?

Comment 4

[(dormant account)]

Comment on attachment 622080 [details] [diff] [review]
patch



HasInterestingData -> IsEmpty() 


+  for (size_t n = h->bucket_count(), i = 0; i < n; ++i) {
+    if (ss.counts(i) != 0) {
+      return true;
+    }
+  }

Pretty sure you can do .sum() || ss.counts(0) != 0 to skip the loop


+    // We don't check HasInterestingData here.  We discard no-data
+    // histograms on read-in, instead.  It's easier to write out the
+    // number of histograms required that way.  (The pickle interface
+    // doesn't make it easy to go back and overwrite previous data.)
+ <-- should put a todo here or just remember to remove this comment when we serialize things properly

This needs a test, doing a gethistogrambyid to initiate a histogram and then .histogramSnapshots to check that it's not present should be sufficient

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Taras Glek (:taras) from comment #4)
> HasInterestingData -> IsEmpty() 

*shrug*

> +  for (size_t n = h->bucket_count(), i = 0; i < n; ++i) {
> +    if (ss.counts(i) != 0) {
> +      return true;
> +    }
> +  }
> 
> Pretty sure you can do .sum() || ss.counts(0) != 0 to skip the loop

That's a good point.  See?  Hard to tell about efficiency concerns.

> +    // We don't check HasInterestingData here.  We discard no-data
> +    // histograms on read-in, instead.  It's easier to write out the
> +    // number of histograms required that way.  (The pickle interface
> +    // doesn't make it easy to go back and overwrite previous data.)
> + <-- should put a todo here or just remember to remove this comment when we
> serialize things properly

When we serialize things properly (wherever the persistent-data-in-JS bug is), all this code is going away.

> This needs a test, doing a gethistogrambyid to initiate a histogram and then
> .histogramSnapshots to check that it's not present should be sufficient

Ah, yes.  Tests.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: patch

Updated, with tests.  Removed the comment about addon histograms, because we can hit that check even if the addon's histogram has been created, of course.

Comment 7

[(dormant account)]

Comment on attachment 622387 [details] [diff] [review]
patch

> 
> > +  for (size_t n = h->bucket_count(), i = 0; i < n; ++i) {
> > +    if (ss.counts(i) != 0) {
> > +      return true;
> > +    }
> > +  }
> > 
> > Pretty sure you can do .sum() || ss.counts(0) != 0 to skip the loop
> 
> That's a good point.  See?  Hard to tell about efficiency concerns.

I think it's a pretty easy call when you end up with less code that does less

+    // We don't check HasInterestingData here.  We discard no-data
                               ^--- fixme

Comment 8

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f0d4464f447

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5f0d4464f447