1158317 - Make sure Fx39 doesn't enable Telemetry when toolkit.telemetry.enabled is off

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Filing per bug 1139460, comment 44 for tracking and discussion.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Release management prefers to have the unified feature off when we go to beta.
I don't think we get much out of the beta data with the current state on 39.

Benjamin, you definitely wanted this to go on beta?

Comment 2

[Georg Fritzsche [:gfritzsche]]

Turning the feature off is possible after bug 1148500, via the pref "toolkit.telemetry.unified".
If we don't turn it off we definitely need bug 1140037 on 39 before it goes to beta.

Comment 3

[Benjamin Smedberg]

I really don't understand the phrase "the unified feature". The telemetry code needs to work  on beta because we use it to drive dashboards. At that point, disabling a different set of things seems like extra risk, not reward. The only issue in my mind is really the midnight throttling issue.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Ok, both bug 1148500 & bug 1140037 landed and i will request approval on their 39 versions soon.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> I really don't understand the phrase "the unified feature".

I mean:
* not initializing Telemetry when only FHR is enabled
* not generating the new ping types ("main")
* disabling the ping archiving

We definitely want to flip that pref before we go to release.
I think flipping it early in beta would be good to iron out any potential remaining issues.

Comment 5

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

Tracking for 39.

Comment 6

[Benjamin Smedberg]

I don't think we should do this. I think we should keep sending all the ping types for users who have telemetry enabled, and the only thing we should do here is turn the entire feature off for users who have telemetry disabled.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Ok, that makes sense.
We have the switch from comment 4 anyway (for e.g. Fennec/Thunderbird/B2G), but we don't need to uplift that to 39.
So that would mean less uplifting to Beta and the new behavior would only affect users who have opted into Telemetry.

Lawrence, does that plus bug 1140037 sound ok?

Comment 8

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

My understanding is that unified Telemetry will not ship in 39. I think comment 6 is suggesting that we continue to send the old and new Telemetry ping for all users. What's the value in sending duplicate information? Why wouldn't we disable unified Telemetry in 39 if the feature isn't ready (requires phase 3 work still) and we don't intend to use the data? Is the intention still to disable this feature before 39 hits release?

Comment 9

[Benjamin Smedberg]

I think the confusion here may just be about terminology:

Unified telemetry is two things:

A. a set of incremental changes to how telemetry behaves
B. a policy change that we should upload telemetry for users who have FHR on (but telemetry off)

I believe that we shouldn't do anything special for A for 39. Testing the changes on the beta population shouldn't hurt and might help.

We need to make sure that B is turned off for 39. That's what I think this bug should track.

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> We need to make sure that B is turned off for 39. That's what I think this
> bug should track.

This is fine on 39 currently.

Given that we are running this without impact on the current Telemetry data now, i guess there is no big risk then (given that i verify B & we land bug 1140037 on 39).

Lawrence, is this ok with you? Do you need more info/details/actions on this?

Comment 11

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

If I have understood comment 9 correctly, Unified Telemetry is simply the existing Telemetry system with change. ie. There are not separate Telemetry and Unified Telemetry pings. There is also not a separate set of probes for Unified Telemetry. If that's the case, our goal is to ensure that Telemetry is not broken in 39 and that we do not make a policy change (as Benjamin noted in B). Sounds like what we need to get into the proper state is bug 1140037.

Have I summarized this correctly?

Comment 12

[Georg Fritzsche [:gfritzsche]]

* we introduced new ping types, but they build on the same (extended) system
* there is no separate set of probes
* bug 1140037 is required to not break beta

Comment 13

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

Your plan wfm. Looks like there is good progress in bug 1140037. Can you confirm that you expect to land that fix this week before 39 merges to Beta?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Yes, we are currently waiting for the data to trickle in and confirm it.
I will flag for approval as soon as that happened.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> > We need to make sure that B is turned off for 39. That's what I think this
> > bug should track.
> 
> This is fine on 39 currently.
> 
> Given that we are running this without impact on the current Telemetry data
> now, i guess there is no big risk then (given that i verify B & we land bug
> 1140037 on 39).

I verified B and basic Telemetry behavior with bug 1140037.
Per bug 1140037, comment 43 that bug is pending 39 approval, see the details there.

After that landed we should be good.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Bug 1140037 landed on 39, so we are good here.