826998 - Avoid hardcoding the result of TelemetryImpl::GetCanSend

Status: RESOLVED INVALID

(Toolkit :: Telemetry, defect)

Description

[Mike Hommey [:glandium]]

Such hardcoding means xulrunner applications can't choose whether they want telemetry to be able to send or not.

Comment 1

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #0)
> Such hardcoding means xulrunner applications can't choose whether they want
> telemetry to be able to send or not.

... while they can choose a server where to send to.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Avoid hardcoding the result of TelemetryImpl::GetCanSend

Comment 3

[Mike Hommey [:glandium]]

(I might need to adjust unit tests)

Comment 4

[Vladan Djeric (:vladan)]

1. Do any XULRunner app authors actually want to collect Telemetry?
2. I think we still want GetCanSend guarded by #ifdefs to avoid a situation where someone runs their own developer build against a profile created by release Firefox and ends up submitting highly skewed Telemetry data
3. We should also blank the default value of toolkit.telemetry.server in non-official builds so we don't accidentally get sent XULRunner Telemetry submissions
4. If we go with this approach, let's read the pref once and cache the result. We don't handle the pref being changed at runtime gracefully (see comments in bug 821879)

Comment 5

[Mike Hommey [:glandium]]

(In reply to Vladan Djeric (:vladan) from comment #4)
> 1. Do any XULRunner app authors actually want to collect Telemetry?

This is mostly a theoretical situation, but it's also a concern for Firefox built as a xulrunner application.

> 2. I think we still want GetCanSend guarded by #ifdefs to avoid a situation
> where someone runs their own developer build against a profile created by
> release Firefox and ends up submitting highly skewed Telemetry data

Prefs are not stored in the profile unless they are changed. So either a profile created by release Firefox would have it false, or not at all.

> 3. We should also blank the default value of toolkit.telemetry.server in
> non-official builds so we don't accidentally get sent XULRunner Telemetry
> submissions

I think it's actually better that it doesn't show up at all in about:config in applications that don't set it already.

> 4. If we go with this approach, let's read the pref once and cache the
> result. We don't handle the pref being changed at runtime gracefully (see
> comments in bug 821879)

It telemetry is not initialized, GetCanSend is not going to be called, is it? Why care?

Comment 6

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #3)
> (I might need to adjust unit tests)

It looks like I don't.

Comment 7

[Vladan Djeric (:vladan)]

(In reply to Mike Hommey [:glandium] from comment #5)
> > 3. We should also blank the default value of toolkit.telemetry.server in
> > non-official builds so we don't accidentally get sent XULRunner Telemetry
> > submissions
> I think it's actually better that it doesn't show up at all in about:config
> in applications that don't set it already.

I noticed the greprefs.js file in the xulrunner-sdk has toolkit.telemetry.server set to our official server. Is this something we have to worry about?

Also, it looks like XULRunner is built with the MOZILLA_OFFICIAL flag, so we won't be initializing Telemetry or gathering any local XULRunner Telemetry when canSend = false. I think that's OK. 

> > 4. If we go with this approach, let's read the pref once and cache the
> > result. We don't handle the pref being changed at runtime gracefully (see
> > comments in bug 821879)
> 
> It telemetry is not initialized, GetCanSend is not going to be called, is
> it? Why care?

I was thinking of the inconsistent behavior that would arise from changing the canSend pref at runtime from true->false vs false->true. One would cause the current session's behavior to change and the other would not have an effect until restart. But that is a very minor concern.

-- 

Btw, do you know why MOZ_TELEMETRY_REPORTING=1 in some B2G mozconfigs?

http://mxr.mozilla.org/mozilla-central/source/b2g/config/mozconfigs/win32_gecko/nightly

We want GetCanSend to return false on B2G for the time-being (bug 821879)

Comment 8

[Vladan Djeric (:vladan)]

Comment on attachment 698262 [details] [diff] [review]
Avoid hardcoding the result of TelemetryImpl::GetCanSend

Taras or Lawrence, do you have an opinion on this?

Comment 9

[Mike Hommey [:glandium]]

(In reply to Vladan Djeric (:vladan) from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #5)
> > > 3. We should also blank the default value of toolkit.telemetry.server in
> > > non-official builds so we don't accidentally get sent XULRunner Telemetry
> > > submissions
> > I think it's actually better that it doesn't show up at all in about:config
> > in applications that don't set it already.
> 
> I noticed the greprefs.js file in the xulrunner-sdk has
> toolkit.telemetry.server set to our official server. Is this something we
> have to worry about?

It depends whether we have faith in the people enabling toolkit.telemetry.canSend.
We could move toolkit.telemetry.server next to toolkit.telemetry.canSend.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

I think that it is preferable to limit Telemetry data collection and submission to builds that we care about (Firefox channels). However, any data submitted by other builds should be filtered out on the server side (or we can update the server to filter it out). Vlad and Taras are in a better position to comment on the mechanism that we use to enable Telemetry.

Comment 11

[(dormant account)]

I have no strong opinion on this. I fearful of the additional footgun created by allowing people to explicitly enable canSend in dev builds.

I also have doubts about people wanting to setup infrastructure for serverside telemetry. Seems like a very unlikely scenario. We certainly are not in a position to provide satisfying telemetry serverside for every xulrunner app.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Taras Glek (:taras) from comment #11)
> I also have doubts about people wanting to setup infrastructure for
> serverside telemetry. Seems like a very unlikely scenario. We certainly are
> not in a position to provide satisfying telemetry serverside for every
> xulrunner app.

And we shouldn't. So I guess it would make sense to move toolkit.telemetry.server next to toolkit.telemetry.canSend.

Comment 13

[Chris H-C :chutten]

This has been iterated over a few times since then. We now have a few ways to ensure, at compile and run-time, that Telemetry is or is not enabled.