don't send no-data histograms

RESOLVED FIXED in mozilla15

Status

()

Toolkit
Telemetry
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

5.71 KB, patch
(dormant account)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 621045 [details] [diff] [review]
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.
Attachment #621045 - Flags: review?(taras.mozilla)

Comment 2

6 years ago
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
Attachment #621045 - Flags: review?(taras.mozilla) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 622080 [details] [diff] [review]
patch

Ended up being simpler on the C++ side.  Efficiency?  What's that?
Attachment #621045 - Attachment is obsolete: true
Attachment #622080 - Flags: review?(taras.mozilla)

Comment 4

6 years ago
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
Attachment #622080 - Flags: review?(taras.mozilla) → review-
(Assignee)

Comment 5

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

Comment 6

6 years ago
Created attachment 622387 [details] [diff] [review]
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.
Attachment #622080 - Attachment is obsolete: true
Attachment #622387 - Flags: review?(taras.mozilla)

Comment 7

6 years ago
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
Attachment #622387 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 8

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f0d4464f447
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla15

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/5f0d4464f447
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.