[M123]Fix when and how some readaloud histograms are emitted
- Eligibility is now logged when profile is available
- Readability was previously biased towards logging ineligible pages (from the client side optimizations). Now we first check if feature is enabled at all and all required objects initialized before we log readability, and to remove bias, we also log it for cached results.
- New histogram for server side readability rate is added
(cherry picked from commit c8aec78d412fac262ff3eec3a26da0121ed99a28)
Bug: 328264691
Bug: 328264863
Change-Id: Ia3aaf2f762d3c01683fced9ee08cfc7f5057b04f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5346496
Reviewed-by: Ian Wells <iwells@chromium.org>
Reviewed-by: Andrea Gomez <andreaxg@google.com>
Commit-Queue: Basia Zimirska <basiaz@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1268694}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5352705
Cr-Commit-Position: refs/branch-heads/6312@{#517}
Cr-Branched-From: 6711dcdae48edaf98cbc6964f90fac85b7d9986e-refs/heads/main@{#1262506}
diff --git a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudController.java b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudController.java
index ff5f6d0..e52c3cb 100644
--- a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudController.java
+++ b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudController.java
@@ -271,6 +271,7 @@
public void onSuccess(String url, boolean isReadable, boolean timepointsSupported) {
Log.d(TAG, "onSuccess called for %s", url);
ReadAloudMetrics.recordIsPageReadable(isReadable);
+ ReadAloudMetrics.recordServerReadabilityResult(isReadable);
ReadAloudMetrics.recordIsPageReadabilitySuccessful(true);
// Register _KnownReadable trial before checking more playback conditions
@@ -337,13 +338,21 @@
public ObservableSupplier<String> getReadabilitySupplier() {
return mReadabilitySupplier;
}
- private void onProfileAvailable(Profile profile) {
+
+ @VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE)
+ public void onProfileAvailable(Profile profile) {
mProfile = profile;
mReadabilityHooks =
sReadabilityHooksForTesting != null
? sReadabilityHooksForTesting
: new ReadAloudReadabilityHooksImpl(mActivity, profile);
if (mReadabilityHooks.isEnabled()) {
+ boolean isAllowed = ReadAloudFeatures.isAllowed(mProfileSupplier.get());
+ ReadAloudMetrics.recordIsUserEligible(isAllowed);
+ if (!isAllowed) {
+ ReadAloudMetrics.recordIneligibilityReason(
+ ReadAloudFeatures.getIneligibilityReason());
+ }
mHighlightingEnabled.addObserver(
ReadAloudController.this::onHighlightingEnabledChanged);
mHighlightingEnabled.set(ReadAloudPrefs.isHighlightingEnabled(getPrefService()));
@@ -356,12 +365,6 @@
maybeCheckReadability(url);
maybeHandleTabReload(tab, url);
maybeStopPlayback(tab);
- boolean isAllowed = ReadAloudFeatures.isAllowed(mProfileSupplier.get());
- ReadAloudMetrics.recordIsUserEligible(isAllowed);
- if (!isAllowed) {
- ReadAloudMetrics.recordIneligibilityReason(
- ReadAloudFeatures.getIneligibilityReason());
- }
}
@Override
@@ -453,22 +456,6 @@
@VisibleForTesting(otherwise = VisibleForTesting.PACKAGE_PRIVATE)
public void maybeCheckReadability(GURL url) {
- if (!isURLReadAloudSupported(url)) {
- ReadAloudMetrics.recordIsPageReadable(false);
- return;
- }
-
- if (mProfile == null || !mProfile.isNativeInitialized()) {
- return;
- }
-
- String urlSpec = stripUserData(url).getSpec();
- // TODO: 2 different URLs can have the same sanitized URL
- mSanitizedToFullUrlMap.put(url.getSpec(), urlSpec);
- if (mReadabilityMap.containsKey(urlSpec) || mPendingRequests.contains(urlSpec)) {
- return;
- }
-
if (!isAvailable()) {
return;
}
@@ -477,6 +464,28 @@
return;
}
+ if (mProfile == null || !mProfile.isNativeInitialized()) {
+ return;
+ }
+
+ if (!isURLReadAloudSupported(url)) {
+ ReadAloudMetrics.recordIsPageReadable(false);
+ return;
+ }
+
+ String urlSpec = stripUserData(url).getSpec();
+ // TODO: 2 different URLs can have the same sanitized URL
+ mSanitizedToFullUrlMap.put(url.getSpec(), urlSpec);
+ if (mReadabilityMap.containsKey(urlSpec)) {
+ ReadAloudMetrics.recordIsPageReadable(mReadabilityMap.get(urlSpec));
+ return;
+ }
+
+ if (mPendingRequests.contains(urlSpec)) {
+ return;
+ }
+
+
mPendingRequests.add(urlSpec);
mReadabilityHooks.isPageReadable(urlSpec, mReadabilityCallback);
}
diff --git a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudControllerUnitTest.java b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudControllerUnitTest.java
index 3a7d16f..392bbe5 100644
--- a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudControllerUnitTest.java
+++ b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudControllerUnitTest.java
@@ -1499,6 +1499,41 @@
}
@Test
+ public void testMetricRecorded_serverReadability() {
+ final String histogramName = ReadAloudMetrics.READABILITY_SERVER_SIDE;
+
+ var histogram = HistogramWatcher.newSingleRecordWatcher(histogramName, true);
+ mController.maybeCheckReadability(sTestGURL);
+ verify(mHooksImpl).isPageReadable(eq(sTestGURL.getSpec()), mCallbackCaptor.capture());
+ mCallbackCaptor
+ .getValue()
+ .onSuccess(
+ sTestGURL.getSpec(),
+ /* isReadable= */ true,
+ /* timepointsSupported= */ false);
+ histogram.assertExpected();
+
+ histogram = HistogramWatcher.newSingleRecordWatcher(histogramName, false);
+ mCallbackCaptor
+ .getValue()
+ .onSuccess(
+ sTestGURL.getSpec(),
+ /* isReadable= */ false,
+ /* timepointsSupported= */ false);
+ histogram.assertExpected();
+
+ // nothing should be emitted on error
+ histogram = HistogramWatcher.newBuilder().expectNoRecords(histogramName).build();
+ mController.maybeCheckReadability(sTestGURL);
+ verify(mHooksImpl, times(1))
+ .isPageReadable(eq(sTestGURL.getSpec()), mCallbackCaptor.capture());
+ mCallbackCaptor
+ .getValue()
+ .onFailure(sTestGURL.getSpec(), new Throwable("Something went wrong"));
+ histogram.assertExpected();
+ }
+
+ @Test
@DisableFeatures(ChromeFeatureList.READALOUD_PLAYBACK)
public void testReadAloudPlaybackFlagCheckedAfterReadability() {
mController.maybeCheckReadability(sTestGURL);
@@ -1608,12 +1643,12 @@
final String histogramName = ReadAloudMetrics.IS_USER_ELIGIBLE;
var histogram = HistogramWatcher.newSingleRecordWatcher(histogramName, true);
- mController.getTabModelTabObserverforTests().onPageLoadStarted(mTab, mTab.getUrl());
+ mController.onProfileAvailable(mMockProfile);
histogram.assertExpected();
histogram = HistogramWatcher.newSingleRecordWatcher(histogramName, false);
when(mPrefService.getBoolean("readaloud.listen_to_this_page_enabled")).thenReturn(false);
- mController.getTabModelTabObserverforTests().onPageLoadStarted(mTab, mTab.getUrl());
+ mController.onProfileAvailable(mMockProfile);
histogram.assertExpected();
}
@@ -1625,7 +1660,7 @@
HistogramWatcher.newSingleRecordWatcher(
histogramName, IneligibilityReason.POLICY_DISABLED);
when(mPrefService.getBoolean("readaloud.listen_to_this_page_enabled")).thenReturn(false);
- mController.getTabModelTabObserverforTests().onPageLoadStarted(mTab, mTab.getUrl());
+ mController.onProfileAvailable(mMockProfile);
histogram.assertExpected();
when(mPrefService.getBoolean("readaloud.listen_to_this_page_enabled")).thenReturn(true);
@@ -1635,7 +1670,7 @@
doReturn(SearchEngineType.SEARCH_ENGINE_OTHER)
.when(mTemplateUrlService)
.getSearchEngineTypeFromTemplateUrl(anyString());
- mController.getTabModelTabObserverforTests().onPageLoadStarted(mTab, mTab.getUrl());
+ mController.onProfileAvailable(mMockProfile);
histogram.assertExpected();
}
diff --git a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudMetrics.java b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudMetrics.java
index a4da241..cfd5d90 100644
--- a/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudMetrics.java
+++ b/chrome/browser/readaloud/android/java/src/org/chromium/chrome/browser/readaloud/ReadAloudMetrics.java
@@ -26,6 +26,7 @@
public static String TIME_SPENT_LISTENING_LOCKED_SCREEN =
"ReadAloud.DurationListened.LockedScreen";
public static String HAS_DATE_MODIFIED = "ReadAloud.HasDateModified";
+ public static String READABILITY_SERVER_SIDE = "ReadAloud.ServerReadabilityResult";
/**
* The reason why we clear the prepared message.
@@ -106,6 +107,10 @@
RecordHistogram.recordBooleanHistogram(IS_READABLE, successful);
}
+ public static void recordServerReadabilityResult(boolean successful) {
+ RecordHistogram.recordBooleanHistogram(READABILITY_SERVER_SIDE, successful);
+ }
+
public static void recordIsPageReadabilitySuccessful(boolean successful) {
RecordHistogram.recordBooleanHistogram(READABILITY_SUCCESS, successful);
}
diff --git a/tools/metrics/histograms/metadata/readaloud/histograms.xml b/tools/metrics/histograms/metadata/readaloud/histograms.xml
index b0cd1c6..bb1e9da 100644
--- a/tools/metrics/histograms/metadata/readaloud/histograms.xml
+++ b/tools/metrics/histograms/metadata/readaloud/histograms.xml
@@ -58,9 +58,10 @@
<owner>basiaz@google.com</owner>
<owner>iwells@chromium.org</owner>
<summary>
- Histogram for recording why a user is not eligible for the ReadAloud
- feature. Emitted when the user profile is available and the user is
- ineligible. Conditions are checked in ReadAloudFeatures.
+ Android only. Histogram for recording why a user is not eligible for the
+ ReadAloud feature. Emitted when the user profile is available and the user
+ is ineligible. Conditions are checked in ReadAloudFeatures. Don't use data
+ from before M123.
</summary>
</histogram>
@@ -70,8 +71,8 @@
<owner>basiaz@google.com</owner>
<owner>iwells@chromium.org</owner>
<summary>
- Histogram for recording if a user is eligible for ReadAloud. Emitted when
- the user profile is available.
+ Android only. Histogram for recording if a user is eligible for ReadAloud.
+ Emitted when the user profile is available. Don't use data from before M123.
</summary>
</histogram>
@@ -149,9 +150,15 @@
<owner>basiaz@google.com</owner>
<owner>iwells@chromium.org</owner>
<summary>
- Histogram for recording if a page readability check comes back isReadable or
- not. The readability check is run on each page load for Android clients who
- are eligible for ReadAloud.
+ Android only. Histogram for recording if a page is readable or not. Will be
+ recorded only if ReadAloud is enabled and initialization of all
+ ReadAloud-required objects was successful. Emitted when readability check is
+ requested (on page load, on redirect, on selecting a tab), and cached
+ readability results may be retured. This histogram contain both server side
+ readability responses as well as client side optimizations (client will fail
+ fast readability check for some URL types). For server side only readability
+ ratio, check ReadAloud.ServerReadabilityResult. Do not use data from before
+ M123.
</summary>
</histogram>
@@ -207,6 +214,21 @@
<token key="Phase" variants="Phase"/>
</histogram>
+<histogram name="ReadAloud.ServerReadabilityResult" enum="BooleanEligible"
+ expires_after="2024-08-04">
+ <owner>andreaxg@google.com</owner>
+ <owner>basiaz@google.com</owner>
+ <owner>iwells@chromium.org</owner>
+ <summary>
+ Android only. Histogram for recording server side responses if a page is
+ readable or not. Will be recorded only if ReadAloud is enabled,
+ initialization of all ReadAloud-required objects was successful and no
+ previously cached readability results are available for the requested URL.
+ Emitted from a server response callback when requesting readability
+ information.
+ </summary>
+</histogram>
+
<histogram name="ReadAloud.SpeedChange" enum="ReadAloudSpeed"
expires_after="2024-08-11">
<owner>andreaxg@google.com</owner>