Closed Bug 1908108 Opened 1 year ago Closed 1 year ago

Remove TelemetryUtils.isTelemetryEnabled

Categories

(Toolkit :: Telemetry, task)

Product:
Toolkit
Component:
Telemetry
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
130 Branch
Tracking Flags:
Tracking Status
firefox130 --- fixed

People

(Reporter: saschanaz, Assigned: saschanaz)

References

Details

Attachments

(1 file)

Bug 1908108 - Replace TelemetryUtils.isTelemetryEnabled with inline pref reading r=chutten
48 bytes, text/x-phabricator-request
Details | Review

Per Jan-Erik, this is an old leftover and should be removed. This checks a locked pref toolkit.telemetry.enabled which does not reflect telemetry upload setting at runtime, the right pref to check for that purpose is datareporting.healthreport.uploadEnabled.

Note that there are some valid uses of toolkit.telemetry.enabled (through TelemetryUtils.isTelemetryEnabled).
Some parts are disabled in configurations where the whole telemetry system is disabled (unofficial builds).
See also https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/internals/preferences.html

So we might wanna rename isTelemetryEnabled for the moment to avoid confusion.

Assignee: nobody → krosylight

Hi chutten, I was to go ahead and replace 1 2 3 with datareporting pref, but was suggested to wait for your opinion first (which is a right thing I think 😄). Do you think those can be replaced easily, and do you have a good idea for potential rename as comment #1 suggested?

Flags: needinfo?(chutten)

For background: It's only locked on Desktop. On Mobile it's, er, mobile. It shouldn't have any effect except where Legacy Telemetry isn't working in unified mode (the less you know about that, the better), but it has caused trouble not that long ago (e.g. bug 1850847).

As such and due to other scars I have from best-of-intentions changes, I'm very hesitant to change anything Legacy-Telemetry-related without a clear positive impact. I'm not sure that renaming a module-internal API like TelemetryUtils.isTelemetryEnabled meets that threshold, but I'm ready to be convinced.

Flags: needinfo?(chutten) → needinfo?(krosylight)

(In reply to Chris H-C :chutten from comment #3)

For background: It's only locked on Desktop. On Mobile it's, er, mobile. It shouldn't have any effect except where Legacy Telemetry isn't working in unified mode (the less you know about that, the better), but it has caused trouble not that long ago (e.g. bug 1850847).

Oh oops, that one, yeah.

As such and due to other scars I have from best-of-intentions changes, I'm very hesitant to change anything Legacy-Telemetry-related without a clear positive impact. I'm not sure that renaming a module-internal API like TelemetryUtils.isTelemetryEnabled meets that threshold, but I'm ready to be convinced.

The issue is that it's not exactly perceived as module-internal, we have 3 calls outside toolkit directory: https://searchfox.org/mozilla-central/search?q=TelemetryUtils.isTelemetryEnabled&path=&case=false&regexp=false, two new-ish uses added by https://phabricator.services.mozilla.com/D163818 in 2022 (reviewed by you, so maybe valid?) and https://phabricator.services.mozilla.com/D212223 in May 2024. Not sure there's a way to prevent importing internal-purpose module this way, but maybe the rename could be isTelemetryEnabledInternal or some such in that case. What do you think?

Flags: needinfo?(krosylight) → needinfo?(chutten)

At the risk of expanding scope I think rather than simply removing/renaming isTelemetryEnabled we need to figure out an API that tells other features what they need to know without them having to know the preferences and build config settings to rely on since that is fragile and likely to lead to mistakes and potential future breakage as those assumptions change. As a strawman I think this looks something like:

isDataReportingSupported() {
  return AppConstants.MOZ_TELEMETRY_REPORTING
}

isDataReportingEnabled() {
  return isDataReportingSupported() && Services.prefs.getBoolPref("datareporting.healthreport.uploadEnabled", false)
}

We'll also want a straightforward way to listen for changes to those and all this available to both C++ and JavaScript.

If we need to retain isTelemetryEnabled for legacy telemetry then I suggest renaming to _isTelemetryEnabled.

Thoughts?

(In reply to Dave Townsend [:mossop] from comment #5)

At the risk of expanding scope I think rather than simply removing/renaming isTelemetryEnabled we need to figure out an API that tells other features what they need to know without them having to know the preferences and build config settings to rely on since that is fragile and likely to lead to mistakes and potential future breakage as those assumptions change. As a strawman I think this looks something like:

isDataReportingSupported() {
  return AppConstants.MOZ_TELEMETRY_REPORTING
}

isDataReportingEnabled() {
  return isDataReportingSupported() && Services.prefs.getBoolPref("datareporting.healthreport.uploadEnabled", false)
}

We'll also want a straightforward way to listen for changes to those and all this available to both C++ and JavaScript.

This matches what I think we need / this should look like, yes. I'm not sure how many consumers we really have for isDataReportingSupported on its own.

One other thing that is a variation of something different that you suggested, is locking the datareporting.healthreport.uploadEnabled pref to false in the !AppConstants.MOZ_TELEMETRY_REPORTING case (at compile time). A more explicit API like the one you propose here would still be a good idea, but this would simplify some of the (currently very convoluted) code in the privacy section of the Firefox settings quite a bit, and makes future mistakes less likely.

If we need to retain isTelemetryEnabled for legacy telemetry then I suggest renaming to _isTelemetryEnabled.

Yes - I'd go further as the comments from Jan-Erik and :chutten imply to me that the only valid uses are internal to telemetry, so I might even suggest some variation of _isTelemetryInternalDisabledNotForUseOutsideTelemetry (somewhat exaggerated, but only kind of).

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #4)

The issue is that it's not exactly perceived as module-internal, we have 3 calls outside toolkit directory: https://searchfox.org/mozilla-central/search?q=TelemetryUtils.isTelemetryEnabled&path=&case=false&regexp=false, two new-ish uses added by https://phabricator.services.mozilla.com/D163818 in 2022 (reviewed by you, so maybe valid?) and https://phabricator.services.mozilla.com/D212223 in May 2024. Not sure there's a way to prevent importing internal-purpose module this way, but maybe the rename could be isTelemetryEnabledInternal or some such in that case. What do you think?

Yeah, that review from 2022 is incorrect and is on me. It shouldn't have been using TelemetryUtils at all. The name on the tin gives it away: it's utilities for Telemetry, not for everyone.

There are a lot of modules and features interested in data consent across JS, Rust, and C++. I don't think TelemetryUtils is a sufficiently portable place for such a proposed API. It may even be that the polyglot nature of the problem is why querying the raw pref is the recommended option today.

I'm resistant to prefixing the API with _ because that denotes file-internal APIs. I've not encountered syntax conventions for denoting Module-internal APIs, though, so maybe it's overloaded to mean both? Maybe the best angle is indeed to remove the API altogether and inline its implementation into the places where it's actually needed.

Flags: needinfo?(chutten)

Inlining pref reading works for my interest, at least for now. Probably we still need a helper to properly decide to read the correct pref though, because it's still too easy to use toolkit.telemetry.enabled. I assume any modules outside telemetry should use datareporting pref instead, am I correct?

Flags: needinfo?(chutten)

Yes. As far as I know, Firefox Desktop's user data consent preference is stored in datareporting.healthreport.uploadEnabled so any modules interested in data consent should indeed use it.

Flags: needinfo?(chutten)

(In reply to Chris H-C :chutten from comment #9)

Yes. As far as I know, Firefox Desktop's user data consent preference is stored in datareporting.healthreport.uploadEnabled so any modules interested in data consent should indeed use it.

If other apps use something else then that is an even stronger argument for a sane API.

Strawperson proposal: put the API (per what Mossop is suggesting) on Services.appinfo (JS) and an appropriate XPCOM interface (maybe nsIXULAppInfo), implement in C++ in nsAppRunner.cpp . Whether the crash reporter is enabled already lives there. We also would probably need an observer notification or other mechanism for callers to observe changes to the state.

Two questions:

  1. any objection to that location + calling it isDataReportingEnabled or similar, Jan-Erik / :chutten?
  2. in terms of implementation, do we need any additional and/or different checks (vs. the MOZ_TELEMETRY_REPORTING define and the pref) for Fenix? (per comment #9 / 10)
Flags: needinfo?(jrediger)
Flags: needinfo?(chutten)

I have no objection to the location or naming, except to note that it'll also be needed in Rust. Observer notification's a fine mechanism for our uses in Rust, and is what we're using today, so that's another vote for that route.

As for MOZ_TELEMETRY_REPORTING that's... a weird one and is probably best left out of this. On its own it doesn't do too much (aside from being a part of Telemetry::isOfficialTelemetry, which is Legacy-Telemetry-specific (other folks who look at the pref don't look at the define at all)) and the argument could be made that its behaviour ought to have been rolled into the pref ages ago (why make it so hard to put the "data reporting is locked to off" message up when we could signal it simply by locking the pref to false?).

To put it another way: when designing and implementing FOG back in 2021-2022, we asked around and the only thing we learned from Product, Privacy, and Platform that we needed to check for "can this generic data collection system upload data?" was whether the pref datareporting.healthreport.uploadEnabled was a boolean true. In all other cases (that aren't clearly a developer doing testing, see MOZILLA_OFFICIAL and telemetry.fog.test.localhost_port rules): don't upload.

Flags: needinfo?(chutten)

IMO, any in-product telemetry (legacy, glean, etc) should be gated at minimum on the same thing that the checkbox in the UI is gated on, which is currently Services.prefs.getBoolPref(PREF_UPLOAD_ENABLED) && AppConstants.MOZ_TELEMETRY_REPORTING: https://searchfox.org/mozilla-central/rev/b220e40ff2ee3d10ce68e07d8a8a577d5558e2a2/browser/components/preferences/privacy.js#3481. I realize Glean's local development checks may be covering the same conditions but we should make it as easy as possible to reason about this. Maybe the eventual fix is removing the constant and locking the pref instead, but the benefit of standing up a shared getter is that we can migrate existing consumers to the existing about:preferences behavior and then change it in one place when necessary.

(Unassigning me until we get consensus)

Assignee: krosylight → nobody

I think, since conversations are ongoing about the feature request side of this, that we scope this bug to be what it says on the tin: Remove TelemetryUtils.isTelemetryEnabled. Do you feel you have what you need to move forward with that task, Kagami?

Flags: needinfo?(krosylight)

As bug 1908312 went forward with inlining the pref I guess I can do the same for the remaining one in BrowserGlue. But what do you want for the telemetry module internal uses? Use TelemetryControllerBase.isTelemetryEnabled directly as the Utils function is just calling that?

Flags: needinfo?(krosylight) → needinfo?(chutten)

Either that or inline the pref if TelemetryControllerBase isn't already imported, yes please.

Flags: needinfo?(chutten)
Assignee: nobody → krosylight
Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bd96edb77459 Replace TelemetryUtils.isTelemetryEnabled with inline pref reading r=chutten,firefox-desktop-core-reviewers ,mak

https://hg.mozilla.org/mozilla-central/rev/bd96edb77459

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox130: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 130 Branch
Flags: needinfo?(jrediger)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: