Skip to content

Commit

Permalink
AW: remove Variations.AppSeedRequestState histogram
Browse files Browse the repository at this point in the history 
No change to behavior. This removes the Variations.AppSeedRequestState
histogram and the associated tests.

Fixed: 1130651
Test: run_webview_instrumentation_test_apk -f *Variations*
Change-Id: If7102fe04758c864e00cadd671fc4875af98322a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2491184
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Commit-Queue: Mark Pearson <mpearson@chromium.org>
Auto-Submit: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Reviewed-by: Changwan Ryu <changwan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819951}
  • Loading branch information
@ntfschr-chromium
ntfschr-chromium authored and Commit Bot committed Oct 22, 2020
1 parent e1919c1 commit 9fb203e
Show file tree
Hide file tree
Showing 4 changed files with 6 additions and 115 deletions.
35 changes: 0 additions & 35 deletions android_webview/java/src/org/chromium/android_webview/variations/VariationsSeedLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import android.os.RemoteException;
import android.os.SystemClock;

import androidx.annotation.IntDef;
import androidx.annotation.VisibleForTesting;

import org.chromium.android_webview.AwBrowserProcess;
Expand All @@ -35,8 +34,6 @@
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.util.Date;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;
Expand Down Expand Up @@ -92,9 +89,6 @@ public class VariationsSeedLoader {
@VisibleForTesting
public static final String APP_SEED_FRESHNESS_HISTOGRAM_NAME = "Variations.AppSeedFreshness";
@VisibleForTesting
public static final String APP_SEED_REQUEST_STATE_HISTOGRAM_NAME =
"Variations.AppSeedRequestState";
@VisibleForTesting
public static final String DOWNLOAD_JOB_FETCH_RESULT_HISTOGRAM_NAME =
"Variations.WebViewDownloadJobFetchResult";
@VisibleForTesting
Expand All @@ -114,23 +108,6 @@ public class VariationsSeedLoader {
private SeedLoadAndUpdateRunnable mRunnable;
private SeedServerCallback mSeedServerCallback = new SeedServerCallback();

// UMA histogram values for the result of checking if the app needs a new variations seed.
// Keep in sync with AppSeedRequestState enum in enums.xml.
//
// These values are persisted to logs. Entries should not be renumbered and
// numeric values should never be reused.
@IntDef({AppSeedRequestState.UNKNOWN, AppSeedRequestState.SEED_FRESH,
AppSeedRequestState.SEED_REQUESTED, AppSeedRequestState.SEED_REQUEST_THROTTLED})
@Retention(RetentionPolicy.SOURCE)
@VisibleForTesting
public @interface AppSeedRequestState {
int UNKNOWN = 0;
int SEED_FRESH = 1;
int SEED_REQUESTED = 2;
int SEED_REQUEST_THROTTLED = 3;
int NUM_ENTRIES = 4;
}

private static void recordLoadSeedResult(@LoadSeedResult int result) {
RecordHistogram.recordEnumeratedHistogram(
SEED_LOAD_RESULT_HISTOGRAM_NAME, result, LoadSeedResult.ENUM_SIZE);
Expand All @@ -140,11 +117,6 @@ private static void recordSeedLoadBlockingTime(long timeMs) {
RecordHistogram.recordTimesHistogram(SEED_LOAD_BLOCKING_TIME_HISTOGRAM_NAME, timeMs);
}

private static void recordSeedRequestState(@AppSeedRequestState int state) {
RecordHistogram.recordEnumeratedHistogram(
APP_SEED_REQUEST_STATE_HISTOGRAM_NAME, state, AppSeedRequestState.NUM_ENTRIES);
}

private static void recordAppSeedFreshness(long freshnessMinutes) {
// Bucket parameters should match Variations.SeedFreshness.
// See variations::RecordSeedFreshness.
Expand Down Expand Up @@ -185,12 +157,10 @@ private class SeedLoadAndUpdateRunnable implements Runnable {
// epoch, or Long.MIN_VALUE if we have no seed. This value originates from the server.
// - mSeedFileTime: The time, in milliseconds since the UNIX epoch, our local copy of the
// seed was last written to disk as measured by the device's clock.
// - mSeedRequestState: The result of checking if a new seed is required.
private boolean mFoundNewSeed;
private boolean mNeedNewSeed;
private long mCurrentSeedDate = Long.MIN_VALUE;
private long mSeedFileTime;
private int mSeedRequestState = AppSeedRequestState.UNKNOWN;

private boolean parseSeedFile(File seedFile) {
if (!VariationsSeedLoaderJni.get().parseAndSaveSeedProto(seedFile.getPath())) {
Expand Down Expand Up @@ -222,15 +192,11 @@ private boolean parseSeedFile(File seedFile) {
// avoid delaying FutureTask.get().)
if (!loadedSeed || isSeedExpired(mSeedFileTime)) {
mNeedNewSeed = true;
mSeedRequestState = AppSeedRequestState.SEED_REQUESTED;

// Rate-limit the requests.
if (shouldThrottleRequests(getCurrentTimeMillis())) {
mNeedNewSeed = false;
mSeedRequestState = AppSeedRequestState.SEED_REQUEST_THROTTLED;
}
} else {
mSeedRequestState = AppSeedRequestState.SEED_FRESH;
}

// Save the date field of whatever seed was loaded, if any.
Expand Down Expand Up @@ -266,7 +232,6 @@ public void run() {
public boolean get(long timeout, TimeUnit unit)
throws InterruptedException, ExecutionException, TimeoutException {
boolean success = mLoadTask.get(timeout, unit);
recordSeedRequestState(mSeedRequestState);
if (mSeedFileTime != 0) {
long freshnessMinutes =
TimeUnit.MILLISECONDS.toMinutes(getCurrentTimeMillis() - mSeedFileTime);
Expand Down
80 changes: 0 additions & 80 deletions ...oid_webview/javatests/src/org/chromium/android_webview/test/VariationsSeedLoaderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,18 +164,6 @@ private void assertSingleRecordInHistogram(String histogramName, int expectedVal
RecordHistogram.getHistogramValueCountForTesting(histogramName, expectedValue * 2));
}

private void assertNoAppSeedRequestStateValues() {
Assert.assertEquals(0,
RecordHistogram.getHistogramTotalCountForTesting(
VariationsSeedLoader.APP_SEED_REQUEST_STATE_HISTOGRAM_NAME));
}

private void assertSingleAppSeedRequestStateValue(
@VariationsSeedLoader.AppSeedRequestState int state) {
assertSingleRecordInHistogram(
VariationsSeedLoader.APP_SEED_REQUEST_STATE_HISTOGRAM_NAME, state);
}

// Test the case that:
// VariationsUtils.getSeedFile() - doesn't exist
// VariationsUtils.getNewSeedFile() - doesn't exist
Expand Down Expand Up @@ -364,74 +352,6 @@ public void testDoubleLoad() throws Exception {
}
}

// Test we record the Variations.AppSeedRequestState metric when the seed is fresh.
@Test
@MediumTest
public void testRecordSeedFresh() throws Exception {
File oldFile = VariationsUtils.getSeedFile();
Assert.assertTrue("Expected seed file to not already exist", oldFile.createNewFile());
VariationsTestUtils.writeMockSeed(oldFile);
oldFile.setLastModified(CURRENT_TIME_MILLIS);
assertNoAppSeedRequestStateValues();

runTestLoaderBlocking();

assertSingleAppSeedRequestStateValue(VariationsSeedLoader.AppSeedRequestState.SEED_FRESH);
}

// Test we record the Variations.AppSeedRequestState metric when a new seed is requested.
@Test
@MediumTest
public void testRecordSeedRequested() throws Exception {
File oldFile = VariationsUtils.getSeedFile();
Assert.assertTrue("Expected seed file to not already exist", oldFile.createNewFile());
VariationsTestUtils.writeMockSeed(oldFile);
oldFile.setLastModified(EXPIRED_TIMESTAMP);
assertNoAppSeedRequestStateValues();

runTestLoaderBlocking();

assertSingleAppSeedRequestStateValue(
VariationsSeedLoader.AppSeedRequestState.SEED_REQUESTED);
}

// Test we record the Variations.AppSeedRequestState metric when a seed request is throttled.
@Test
@MediumTest
public void testRecordSeedRequestThrottled() throws Exception {
File oldFile = VariationsUtils.getSeedFile();
Assert.assertTrue("Expected seed file to not already exist", oldFile.createNewFile());
VariationsTestUtils.writeMockSeed(oldFile);
oldFile.setLastModified(EXPIRED_TIMESTAMP);
// Update the last modified time of the stamp file to simulate having just requested a
// new seed from the service.
VariationsUtils.updateStampTime();
assertNoAppSeedRequestStateValues();

runTestLoaderBlocking();

assertSingleAppSeedRequestStateValue(
VariationsSeedLoader.AppSeedRequestState.SEED_REQUEST_THROTTLED);
}

// Test we record the Variations.AppSeedFreshness metric with loading a seed.
@Test
@MediumTest
public void testRecordAppSeedFreshness() throws Exception {
long seedAgeHours = 2;
File oldFile = VariationsUtils.getSeedFile();
Assert.assertTrue("Expected seed file to not already exist", oldFile.createNewFile());
VariationsTestUtils.writeMockSeed(oldFile);
oldFile.setLastModified(CURRENT_TIME_MILLIS - TimeUnit.HOURS.toMillis(seedAgeHours));

runTestLoaderBlocking();

Assert.assertEquals(1,
RecordHistogram.getHistogramValueCountForTesting(
VariationsSeedLoader.APP_SEED_FRESHNESS_HISTOGRAM_NAME,
(int) TimeUnit.HOURS.toMinutes(seedAgeHours)));
}

// Tests that the finch-seed-expiration-age flag works.
@Test
@MediumTest
Expand Down
3 changes: 3 additions & 0 deletions tools/metrics/histograms/enums.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3101,6 +3101,9 @@ Unknown properties are collapsed to zero. -->
</enum>

<enum name="AppSeedRequestState">
<obsolete>
Removed from code October 2020.
</obsolete>
<int value="0" label="Unknown seed state"/>
<int value="1" label="Current seed fresh"/>
<int value="2" label="New seed requested from the service"/>
Expand Down
3 changes: 3 additions & 0 deletions tools/metrics/histograms/histograms_xml/variations/histograms.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.

<histogram name="Variations.AppSeedRequestState" enum="AppSeedRequestState"
expires_after="2020-11-01">
<obsolete>
Removed from code October 2020.
</obsolete>
<owner>rmcelrath@chromium.org</owner>
<owner>ntfschr@chromium.org</owner>
<owner>src/android_webview/OWNERS</owner>
Expand Down

0 comments on commit 9fb203e

Please sign in to comment.