don't initialize Telemetry if we can't send any data in

RESOLVED FIXED in mozilla20

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
Created attachment 692454 [details] [diff] [review]
don't initialize Telemetry if we can't send any data in

I noticed this when doing some research for Lawrence on B2G.  We don't
need to hook up all the observers and whatnot if we're never going to
send the results in.  Tests still pass with this, so we don't have to
worry about that.
Attachment #692454 - Flags: review?(vdjeric)
Comment on attachment 692454 [details] [diff] [review]
don't initialize Telemetry if we can't send any data in

Looks good, but please test that "about:telemetry" doesn't throw any exceptions in the JS console and doesn't display garbage when Telemetry isn't initialized.
Attachment #692454 - Flags: review?(vdjeric) → review+
(In reply to Vladan Djeric (:vladan) from comment #2)
> Looks good, but please test that "about:telemetry" doesn't throw any
> exceptions in the JS console and doesn't display garbage when Telemetry
> isn't initialized.

Is firing up dist/bin/firefox sufficient?  about:telemetry appears to display OK and there's even data in the histograms (?) despite it saying that telemetry is disabled...

Oh, that's probably because this is a non-official build, so Telemetry.canSend is false.  So everything checks out here, yes?
Flags: needinfo?(vdjeric)
So with this change, we won't be recording any Telemetry data locally in developer builds? I can see this being problematic, e.g. I add a new histogram and I want to test it before landing the patch, or I want to use local Telemetry numbers to confirm the impact of a code change. I would have to comment out your patch or add a build option to send in Telemetry. 
Would it make sense to add a new config or a build flag (e.g "dont-gather-telemetry") to prevent initialization of the Telemetry service?
Flags: needinfo?(vdjeric)
No, we *do* record telemetry data; about:telemetry displays it.

We effectively already have a build option to disable sending of telemetry data.  I don't think we need another one.
(In reply to Nathan Froyd (:froydnj) from comment #5)
> No, we *do* record telemetry data; about:telemetry displays it.

Hmm, maybe we're misunderstanding each other? I don't see how we could be recording Telemetry data if you set Telemetry.canRecord to false in your patch. I applied your patch to my build and I don't see any histograms, slow SQL or chrome hangs being recorded (because of the canRecord check). The 20 or so histograms that I do see are recorded on startup because of a subtlety in TelemetryImpl::CanRecord behavior:

bool
TelemetryImpl::CanRecord() {
  return !sTelemetry || sTelemetry->mCanRecord;
}

We unconditionally allow recording of histograms on startup if we haven't yet initialized the Telemetry service.

> We effectively already have a build option to disable sending of telemetry
> data.  I don't think we need another one.

I wasn't suggesting that. My point is that I don't think we should hobble Telemetry *recording* in developer builds where it is very useful.
Ah, I see what you're saying.  In that case, perhaps a:

if (!Telemetry.canSend) {
  return;
}

check is all that's needed, so that we don't set up any of the observers, but we still permit recording?
I should add: if you agree, I'll just make that change and check in the patch.
Created attachment 697087 [details] [diff] [review]
don't initialize Telemetry if we can't send any data in

Thinking about this a little more, the original aim was to reduce overhead for builds where we don't care about telemetry by:

1) Not hooking up observers where we don't need them;
2) Indicating that we don't have to record a bunch of performance statistics we'll never touch.

However, the concern from comment 6 is that the performance statistics *are* useful for developers, even if we're never going to send them in.

So let's try to change the recording flag to indicate that more directly.  We still have the loophole where statistics get recorded until Telemetry gets initialized.  I don't think there's a good way around that without redoing a lot of stuff.
Attachment #697087 - Flags: review?(vdjeric)
Created attachment 697096 [details] [diff] [review]
don't initialize Telemetry if we can't send any data in

Now without syntax errors.
Attachment #692454 - Attachment is obsolete: true
Attachment #697087 - Attachment is obsolete: true
Attachment #697087 - Flags: review?(vdjeric)
Attachment #697096 - Flags: review?(vdjeric)
Comment on attachment 697096 [details] [diff] [review]
don't initialize Telemetry if we can't send any data in

>   setup: function setup() {
>+    if (!Telemetry.canSend) {
>+      // We can't send data; no point in initializing observers etc.
>+      return;
>+    }

This will cause developer builds to skip Telemetry initialization. That means:

1) We'll ignore the developer's Telemetry opt-out (a few lines further in the file) 
2) If Telemetry is enabled in the developer build, we won't register for important events. If we don't register for these events, some of the gathered Telemetry data will be missing or incorrect: startup bytes read/written, startup histograms, late writes, shutdown time, etc.

The larger observation is that Telemetry recording should behave the same on developer and release builds so that developers can test their changes. I think we should only skip Telemetry setup in official builds without the TELEMETRY_REPORTING define.
Attachment #697096 - Flags: review?(vdjeric) → review-
Created attachment 697151 [details] [diff] [review]
don't initialize Telemetry if we can't send any data in

OK, maybe this is more of what you're thinking of, then?
Attachment #697096 - Attachment is obsolete: true
Attachment #697151 - Flags: review?(vdjeric)
Attachment #697151 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/84d3b622b3da
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 697151 [details] [diff] [review]
don't initialize Telemetry if we can't send any data in

[Approval Request Comment]
This is a low-risk patch to disable telemetry data collection for builds that are official builds, but that we haven't enabled telemetry reporting for.  The canonical example is B2G.  The net result is less code executed (no data collection, no extra observers due to telemetry, etc.).

This change does not affect any future enhancements wrt telemetry and B2G: user prefs about data collection would continue to be taken into account if and when we turn on telemetry reporting for B2G.

User impact if declined: Very small; possibly slightly slower performance due to collecting data that we'll never use.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #697151 - Flags: approval-mozilla-b2g18?
Attachment #697151 - Flags: approval-mozilla-aurora?
Comment on attachment 697151 [details] [diff] [review]
don't initialize Telemetry if we can't send any data in

We're at the point in the project where speculative changes, even if low risk, are not necessary on branches.
Attachment #697151 - Flags: approval-mozilla-b2g18?
Attachment #697151 - Flags: approval-mozilla-b2g18-
Attachment #697151 - Flags: approval-mozilla-aurora?
Attachment #697151 - Flags: approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.