-
Notifications
You must be signed in to change notification settings - Fork 1.3k
For #11118: Add missing telemetry #11446
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -590,7 +590,53 @@ class GleanMetricsService(private val context: Context) : MetricsService { | |
} | ||
|
||
internal fun setStartupMetrics() { | ||
setPreferenceMetrics() | ||
Metrics.apply { | ||
defaultBrowser.set(BrowsersCache.all(context).isDefaultBrowser) | ||
MozillaProductDetector.getMozillaBrowserDefault(context)?.also { | ||
defaultMozBrowser.set(it) | ||
} | ||
mozillaProducts.set(MozillaProductDetector.getInstalledMozillaProducts(context)) | ||
|
||
adjustCampaign.set(context.settings().adjustCampaignId) | ||
adjustAdGroup.set(context.settings().adjustAdGroup) | ||
adjustCreative.set(context.settings().adjustCreative) | ||
adjustNetwork.set(context.settings().adjustNetwork) | ||
|
||
searchWidgetInstalled.set(context.settings().searchWidgetInstalled) | ||
|
||
val topSitesSize = context.settings().topSitesSize | ||
hasTopSites.set(topSitesSize > 0) | ||
if (topSitesSize > 0) { | ||
topSitesCount.add(topSitesSize) | ||
} | ||
|
||
toolbarPosition.set( | ||
if (context.settings().shouldUseBottomToolbar) { | ||
Event.ToolbarPositionChanged.Position.BOTTOM.name | ||
} else { | ||
Event.ToolbarPositionChanged.Position.TOP.name | ||
} | ||
) | ||
} | ||
|
||
SearchDefaultEngine.apply { | ||
val defaultEngine = context | ||
.components | ||
.search | ||
.searchEngineManager | ||
.defaultSearchEngine ?: return@apply | ||
|
||
code.set(defaultEngine.identifier) | ||
name.set(defaultEngine.name) | ||
submissionUrl.set(defaultEngine.buildSearchUrl("")) | ||
} | ||
|
||
activationPing.checkAndSend() | ||
installationPing.checkAndSend() | ||
} | ||
|
||
private fun setPreferenceMetrics() { | ||
// We purposefully make all of our preferences the string_list format to make data analysis | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see why this isn't a boolean now. |
||
// simpler. While it makes things like booleans a bit more complicated, it means all our | ||
// preferences can be analyzed with the same dashboard and compared. | ||
|
@@ -603,6 +649,8 @@ class GleanMetricsService(private val context: Context) : MetricsService { | |
showSearchShortcuts.set(context.settings().shouldShowSearchShortcuts.toStringList()) | ||
openLinksInAPrivateTab.set(context.settings().openLinksInAPrivateTab.toStringList()) | ||
searchSuggestionsPrivate.set(context.settings().shouldShowSearchSuggestionsInPrivate.toStringList()) | ||
showVoiceSearch.set(context.settings().shouldShowVoiceSearch.toStringList()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these new additions to the pref-toggle-event or prefs-metrics ping? If so please document. Or was it one of the missing pings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope! Part of the initial telemetry I forgot. |
||
openLinksInApp.set(context.settings().openLinksInExternalApp.toStringList()) | ||
|
||
val isLoggedIn = | ||
context.components.backgroundServices.accountManager.accountProfile() != null | ||
|
@@ -614,6 +662,17 @@ class GleanMetricsService(private val context: Context) : MetricsService { | |
|
||
syncItems.set(syncedItems) | ||
|
||
val toolbarPositionSelection = | ||
if (context.settings().shouldUseFixedTopToolbar) { | ||
"fixed_top" | ||
} else if (context.settings().shouldUseBottomToolbar) { | ||
"bottom" | ||
} else { | ||
"top" | ||
} | ||
|
||
toolbarPosition.set(listOf(toolbarPositionSelection)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a new addition? If so, please document There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope! Part of the initial telemetry I forgot. |
||
|
||
val etpSelection = | ||
if (!context.settings().shouldUseTrackingProtection) { | ||
"" | ||
|
@@ -638,51 +697,22 @@ class GleanMetricsService(private val context: Context) : MetricsService { | |
} | ||
|
||
accessibilityServices.set(accessibilitySelection.toList()) | ||
} | ||
|
||
Metrics.apply { | ||
defaultBrowser.set(BrowsersCache.all(context).isDefaultBrowser) | ||
MozillaProductDetector.getMozillaBrowserDefault(context)?.also { | ||
defaultMozBrowser.set(it) | ||
} | ||
mozillaProducts.set(MozillaProductDetector.getInstalledMozillaProducts(context)) | ||
|
||
adjustCampaign.set(context.settings().adjustCampaignId) | ||
adjustAdGroup.set(context.settings().adjustAdGroup) | ||
adjustCreative.set(context.settings().adjustCreative) | ||
adjustNetwork.set(context.settings().adjustNetwork) | ||
|
||
searchWidgetInstalled.set(context.settings().searchWidgetInstalled) | ||
|
||
val topSitesSize = context.settings().topSitesSize | ||
hasTopSites.set(topSitesSize > 0) | ||
if (topSitesSize > 0) { | ||
topSitesCount.add(topSitesSize) | ||
} | ||
|
||
toolbarPosition.set( | ||
if (context.settings().shouldUseBottomToolbar) { | ||
Event.ToolbarPositionChanged.Position.BOTTOM.name | ||
val themeSelection = | ||
if (context.settings().shouldUseLightTheme) { | ||
"light" | ||
} else if (context.settings().shouldUseDarkTheme) { | ||
"dark" | ||
} else if (context.settings().shouldFollowDeviceTheme) { | ||
"system" | ||
} else if (context.settings().shouldUseAutoBatteryTheme) { | ||
"battery" | ||
} else { | ||
Event.ToolbarPositionChanged.Position.TOP.name | ||
"" | ||
} | ||
) | ||
} | ||
|
||
SearchDefaultEngine.apply { | ||
val defaultEngine = context | ||
.components | ||
.search | ||
.searchEngineManager | ||
.defaultSearchEngine ?: return@apply | ||
|
||
code.set(defaultEngine.identifier) | ||
name.set(defaultEngine.name) | ||
submissionUrl.set(defaultEngine.buildSearchUrl("")) | ||
theme.set(listOf(themeSelection)) | ||
} | ||
|
||
activationPing.checkAndSend() | ||
installationPing.checkAndSend() | ||
} | ||
|
||
override fun stop() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not a boolean? If there are other options, please list all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see why now - can you list both true/false as the possible values, that will make it a little more clear. (Optionally explain why this needs to be string - it's in-code, but not here. OTOH if someone is analyzing this data, they might not care about why boolean vs string).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay pt 3, I see we don't do this for other booleans either. I think this should be pretty clear, so just ignore me 😂