Merge "Implement delete methods in MetricsConfigStore and ResultStore" into sc-v2-dev
diff --git a/car-lib/src/android/car/telemetry/CarTelemetryManager.java b/car-lib/src/android/car/telemetry/CarTelemetryManager.java
index 3c4c187..b0067b1 100644
--- a/car-lib/src/android/car/telemetry/CarTelemetryManager.java
+++ b/car-lib/src/android/car/telemetry/CarTelemetryManager.java
@@ -333,8 +333,9 @@
}
/**
- * Removes a MetricsConfig from {@link com.android.car.telemetry.CarTelemetryService}. If the
- * MetricsConfig does not exist, nothing will be removed.
+ * Removes a MetricsConfig from {@link com.android.car.telemetry.CarTelemetryService}. This
+ * will also remove outputs produced by the MetricsConfig. If the MetricsConfig does not exist,
+ * nothing will be removed.
* The status of this API is sent back asynchronously via {@link CarTelemetryResultsListener}.
*
* @param key the unique key to identify the MetricsConfig. Name and version must be exact.
@@ -351,7 +352,8 @@
}
/**
- * Removes all MetricsConfigs from {@link com.android.car.telemetry.CarTelemetryService}.
+ * Removes all MetricsConfigs from {@link com.android.car.telemetry.CarTelemetryService}. This
+ * will also remove all MetricsConfig outputs.
*
* @hide
*/
diff --git a/car-lib/src/android/car/telemetry/ICarTelemetryService.aidl b/car-lib/src/android/car/telemetry/ICarTelemetryService.aidl
index 49472f4..8343c34 100644
--- a/car-lib/src/android/car/telemetry/ICarTelemetryService.aidl
+++ b/car-lib/src/android/car/telemetry/ICarTelemetryService.aidl
@@ -26,12 +26,13 @@
void addMetricsConfig(in MetricsConfigKey key, in byte[] metricsConfig);
/**
- * Removes a MetricsConfig based on the key.
+ * Removes a MetricsConfig based on the key. This will also remove outputs produced by the
+ * MetricsConfig.
*/
void removeMetricsConfig(in MetricsConfigKey key);
/**
- * Removes all MetricsConfigs.
+ * Removes all MetricsConfigs. This will also remove all MetricsConfig outputs.
*/
void removeAllMetricsConfigs();
diff --git a/service/src/com/android/car/telemetry/CarTelemetryService.java b/service/src/com/android/car/telemetry/CarTelemetryService.java
index 5b78e3b..537ce45 100644
--- a/service/src/com/android/car/telemetry/CarTelemetryService.java
+++ b/service/src/com/android/car/telemetry/CarTelemetryService.java
@@ -15,7 +15,9 @@
*/
package com.android.car.telemetry;
+import static android.car.telemetry.CarTelemetryManager.ERROR_METRICS_CONFIG_NONE;
import static android.car.telemetry.CarTelemetryManager.ERROR_METRICS_CONFIG_PARSE_FAILED;
+import static android.car.telemetry.CarTelemetryManager.ERROR_METRICS_CONFIG_UNKNOWN;
import android.annotation.NonNull;
import android.app.StatsManager;
@@ -161,17 +163,27 @@
public void addMetricsConfig(@NonNull MetricsConfigKey key, @NonNull byte[] config) {
mContext.enforceCallingOrSelfPermission(
Car.PERMISSION_USE_CAR_TELEMETRY_SERVICE, "addMetricsConfig");
- // TODO(b/198797367): if an older version exists, delete its script result/error
mTelemetryHandler.post(() -> {
- TelemetryProto.MetricsConfig metricsConfig;
- int status;
+ Slog.d(CarLog.TAG_TELEMETRY, "Adding metrics config " + key.getName()
+ + " to car telemetry service");
+ // TODO(b/199540952): If config is active, add it to DataBroker
+ TelemetryProto.MetricsConfig metricsConfig = null;
+ int status = ERROR_METRICS_CONFIG_UNKNOWN;
try {
metricsConfig = TelemetryProto.MetricsConfig.parseFrom(config);
- status = mMetricsConfigStore.addMetricsConfig(metricsConfig);
} catch (InvalidProtocolBufferException e) {
Slog.e(CarLog.TAG_TELEMETRY, "Failed to parse MetricsConfig.", e);
status = ERROR_METRICS_CONFIG_PARSE_FAILED;
}
+ // if config can be parsed, add it to persistent storage
+ if (metricsConfig != null) {
+ status = mMetricsConfigStore.addMetricsConfig(metricsConfig);
+ }
+ // If no error (a config is successfully added), script results from an older version
+ // should be deleted
+ if (status == ERROR_METRICS_CONFIG_NONE) {
+ mResultStore.deleteResult(key.getName());
+ }
try {
mListener.onAddMetricsConfigStatus(key, status);
} catch (RemoteException e) {
@@ -181,19 +193,19 @@
}
/**
- * Removes a manifest based on the key.
+ * Removes a metrics config based on the key. This will also remove outputs produced by the
+ * MetricsConfig.
*/
@Override
public void removeMetricsConfig(@NonNull MetricsConfigKey key) {
mContext.enforceCallingOrSelfPermission(
Car.PERMISSION_USE_CAR_TELEMETRY_SERVICE, "removeMetricsConfig");
- // TODO(b/198797367): Delete script result/error associated with this MetricsConfig
mTelemetryHandler.post(() -> {
- if (DEBUG) {
- Slog.d(CarLog.TAG_TELEMETRY, "Removing manifest " + key.getName()
- + " from car telemetry service");
- }
+ Slog.d(CarLog.TAG_TELEMETRY, "Removing metrics config " + key.getName()
+ + " from car telemetry service");
// TODO(b/198792767): Check both config name and config version for deletion
+ // TODO(b/199540952): Stop and remove config from data broker
+ mResultStore.deleteResult(key.getName()); // delete the config's script results
boolean success = mMetricsConfigStore.deleteMetricsConfig(key.getName());
try {
mListener.onRemoveMetricsConfigStatus(key, success);
@@ -204,17 +216,18 @@
}
/**
- * Removes all manifests.
+ * Removes all MetricsConfigs. This will also remove all MetricsConfig outputs.
*/
@Override
public void removeAllMetricsConfigs() {
mContext.enforceCallingOrSelfPermission(
Car.PERMISSION_USE_CAR_TELEMETRY_SERVICE, "removeAllMetricsConfig");
mTelemetryHandler.post(() -> {
- if (DEBUG) {
- Slog.d(CarLog.TAG_TELEMETRY, "Removing all manifest from car telemetry service");
- }
- // TODO(b/184087869): Implement
+ // TODO(b/199540952): Stop and remove all configs from DataBroker
+ Slog.d(CarLog.TAG_TELEMETRY,
+ "Removing all metrics config from car telemetry service");
+ mMetricsConfigStore.deleteAllMetricsConfigs();
+ mResultStore.deleteAllResults();
});
}
@@ -228,7 +241,7 @@
mContext.enforceCallingOrSelfPermission(
Car.PERMISSION_USE_CAR_TELEMETRY_SERVICE, "sendFinishedReports");
if (DEBUG) {
- Slog.d(CarLog.TAG_TELEMETRY, "Flushing reports for a manifest");
+ Slog.d(CarLog.TAG_TELEMETRY, "Flushing reports for a metrics config");
}
}
@@ -249,4 +262,14 @@
Handler getTelemetryHandler() {
return mTelemetryHandler;
}
+
+ @VisibleForTesting
+ ResultStore getResultStore() {
+ return mResultStore;
+ }
+
+ @VisibleForTesting
+ MetricsConfigStore getMetricsConfigStore() {
+ return mMetricsConfigStore;
+ }
}
diff --git a/service/src/com/android/car/telemetry/MetricsConfigStore.java b/service/src/com/android/car/telemetry/MetricsConfigStore.java
index 9802e0e..7a5512a 100644
--- a/service/src/com/android/car/telemetry/MetricsConfigStore.java
+++ b/service/src/com/android/car/telemetry/MetricsConfigStore.java
@@ -89,6 +89,7 @@
} else if (currentVersion == metricsConfig.getVersion()) {
return ERROR_METRICS_CONFIG_ALREADY_EXISTS;
}
+ mActiveConfigs.put(metricsConfig.getName(), metricsConfig);
mNameVersionMap.put(metricsConfig.getName(), metricsConfig.getVersion());
try {
Files.write(
@@ -108,7 +109,10 @@
return new File(mConfigDirectory, metricsConfigName).delete();
}
- void deleteAllMetricsConfig() {
- // TODO(b/198784116): implement
+ void deleteAllMetricsConfigs() {
+ mActiveConfigs.clear();
+ for (File file : mConfigDirectory.listFiles()) {
+ file.delete();
+ }
}
}
diff --git a/service/src/com/android/car/telemetry/ResultStore.java b/service/src/com/android/car/telemetry/ResultStore.java
index 976ee8e..ffa3d09 100644
--- a/service/src/com/android/car/telemetry/ResultStore.java
+++ b/service/src/com/android/car/telemetry/ResultStore.java
@@ -97,27 +97,23 @@
*
* @param metricsConfigName name of the MetricsConfig.
* @param deleteResult if true, the final result will be deleted from disk.
- * @param callback for receiving the metrics output. If result does not exist, it will
- * receive a null value.
*/
- public void getFinalResult(
- String metricsConfigName, boolean deleteResult, FinalResultCallback callback) {
+ public PersistableBundle getFinalResult(String metricsConfigName, boolean deleteResult) {
File file = new File(mFinalResultDirectory, metricsConfigName);
// if no final result exists for this metrics config, return immediately
if (!file.exists()) {
- callback.onFinalResult(metricsConfigName, null);
- return;
+ return null;
}
+ PersistableBundle result = null;
try (FileInputStream fis = new FileInputStream(file)) {
- PersistableBundle bundle = PersistableBundle.readFromStream(fis);
- callback.onFinalResult(metricsConfigName, bundle);
+ result = PersistableBundle.readFromStream(fis);
} catch (IOException e) {
Slog.w(CarLog.TAG_TELEMETRY, "Failed to get final result from disk.", e);
- callback.onFinalResult(metricsConfigName, null);
}
if (deleteResult) {
file.delete();
}
+ return result;
}
/**
@@ -136,6 +132,27 @@
deleteAllStaleData(mInterimResultDirectory, mFinalResultDirectory);
}
+ /**
+ * Deletes script result associated with the given config name. If result does not exist, this
+ * method does not do anything.
+ */
+ public void deleteResult(String metricsConfigName) {
+ mInterimResultCache.remove(metricsConfigName);
+ deleteFileInDirectory(mInterimResultDirectory, metricsConfigName);
+ deleteFileInDirectory(mFinalResultDirectory, metricsConfigName);
+ }
+
+ /** Deletes all interim and final results stored in disk. */
+ public void deleteAllResults() {
+ mInterimResultCache.clear();
+ for (File interimResult : mInterimResultDirectory.listFiles()) {
+ interimResult.delete();
+ }
+ for (File finalResult : mFinalResultDirectory.listFiles()) {
+ finalResult.delete();
+ }
+ }
+
/** Writes dirty interim results to disk. */
private void writeInterimResultsToFile() {
mInterimResultCache.forEach((metricsConfigName, interimResult) -> {
@@ -180,11 +197,6 @@
file.delete();
}
- /** Callback for receiving final metrics output. */
- interface FinalResultCallback {
- void onFinalResult(String metricsConfigName, PersistableBundle result);
- }
-
/** Wrapper around a result and whether the result should be written to disk. */
static final class InterimResult {
private final PersistableBundle mBundle;
diff --git a/tests/carservice_unit_test/src/com/android/car/telemetry/CarTelemetryServiceTest.java b/tests/carservice_unit_test/src/com/android/car/telemetry/CarTelemetryServiceTest.java
index a17e3fa..39389d2 100644
--- a/tests/carservice_unit_test/src/com/android/car/telemetry/CarTelemetryServiceTest.java
+++ b/tests/carservice_unit_test/src/com/android/car/telemetry/CarTelemetryServiceTest.java
@@ -16,6 +16,7 @@
package com.android.car.telemetry;
+import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static org.mockito.ArgumentMatchers.eq;
@@ -27,6 +28,7 @@
import android.car.telemetry.MetricsConfigKey;
import android.content.Context;
import android.os.Handler;
+import android.os.PersistableBundle;
import androidx.test.filters.SmallTest;
@@ -66,6 +68,8 @@
private CarTelemetryService mService;
private File mTempSystemCarDir;
private Handler mTelemetryHandler;
+ private MetricsConfigStore mMetricsConfigStore;
+ private ResultStore mResultStore;
@Rule
public MockitoRule mMockitoRule = MockitoJUnit.rule();
@@ -96,6 +100,9 @@
return true;
});
waitForHandlerThreadToFinish();
+
+ mMetricsConfigStore = mService.getMetricsConfigStore();
+ mResultStore = mService.getResultStore();
}
@Test
@@ -123,7 +130,7 @@
@Test
public void testAddMetricsConfig_invalidMetricsConfig_shouldFail() throws Exception {
- mService.addMetricsConfig(KEY_V1, "bad manifest".getBytes());
+ mService.addMetricsConfig(KEY_V1, "bad config".getBytes());
waitForHandlerThreadToFinish();
verify(mMockListener).onAddMetricsConfigStatus(
@@ -145,34 +152,60 @@
}
@Test
- public void testAddMetricsConfig_newerMetricsConfig_shouldReplace() throws Exception {
+ public void testAddMetricsConfig_newerMetricsConfig_shouldReplaceAndDeleteOldResult()
+ throws Exception {
mService.addMetricsConfig(KEY_V1, METRICS_CONFIG_V1.toByteArray());
+ mResultStore.putInterimResult(KEY_V1.getName(), new PersistableBundle());
mService.addMetricsConfig(KEY_V2, METRICS_CONFIG_V2.toByteArray());
waitForHandlerThreadToFinish();
verify(mMockListener).onAddMetricsConfigStatus(
eq(KEY_V2), eq(CarTelemetryManager.ERROR_METRICS_CONFIG_NONE));
+ assertThat(mMetricsConfigStore.getActiveMetricsConfigs())
+ .containsExactly(METRICS_CONFIG_V2);
+ assertThat(mResultStore.getInterimResult(KEY_V1.getName())).isNull();
}
@Test
- public void testRemoveMetricsConfig_manifestExists_shouldSucceed() throws Exception {
+ public void testRemoveMetricsConfig_configExists_shouldDeleteScriptResult() throws Exception {
mService.addMetricsConfig(KEY_V1, METRICS_CONFIG_V1.toByteArray());
+ mResultStore.putInterimResult(KEY_V1.getName(), new PersistableBundle());
mService.removeMetricsConfig(KEY_V1);
waitForHandlerThreadToFinish();
verify(mMockListener).onRemoveMetricsConfigStatus(eq(KEY_V1), eq(true));
+ assertThat(mMetricsConfigStore.getActiveMetricsConfigs()).isEmpty();
+ assertThat(mResultStore.getInterimResult(KEY_V1.getName())).isNull();
}
@Test
- public void testRemoveMetricsConfig_manifestDoesNotExist_shouldFail() throws Exception {
+ public void testRemoveMetricsConfig_configDoesNotExist_shouldFail() throws Exception {
mService.removeMetricsConfig(KEY_V1);
waitForHandlerThreadToFinish();
verify(mMockListener).onRemoveMetricsConfigStatus(eq(KEY_V1), eq(false));
}
+ @Test
+ public void testRemoveAllMetricsConfigs_shouldRemoveConfigsAndResults() throws Exception {
+ MetricsConfigKey key = new MetricsConfigKey("test config", 2);
+ TelemetryProto.MetricsConfig config =
+ TelemetryProto.MetricsConfig.newBuilder().setName(key.getName()).build();
+ mService.addMetricsConfig(key, config.toByteArray());
+ mService.addMetricsConfig(KEY_V1, METRICS_CONFIG_V1.toByteArray());
+ mResultStore.putInterimResult(KEY_V1.getName(), new PersistableBundle());
+ mResultStore.putFinalResult(key.getName(), new PersistableBundle());
+
+ mService.removeAllMetricsConfigs();
+
+ waitForHandlerThreadToFinish();
+ assertThat(mMetricsConfigStore.getActiveMetricsConfigs()).isEmpty();
+ assertThat(mResultStore.getInterimResult(KEY_V1.getName())).isNull();
+ assertThat(mResultStore.getFinalResult(key.getName(), /* deleteResult = */ false)).isNull();
+ }
+
private void waitForHandlerThreadToFinish() throws Exception {
assertWithMessage("handler not idle in %sms", TIMEOUT_MS)
.that(mIdleHandlerLatch.await(TIMEOUT_MS, TimeUnit.MILLISECONDS)).isTrue();
diff --git a/tests/carservice_unit_test/src/com/android/car/telemetry/MetricsConfigStoreTest.java b/tests/carservice_unit_test/src/com/android/car/telemetry/MetricsConfigStoreTest.java
index 5b326ff..daf83d8 100644
--- a/tests/carservice_unit_test/src/com/android/car/telemetry/MetricsConfigStoreTest.java
+++ b/tests/carservice_unit_test/src/com/android/car/telemetry/MetricsConfigStoreTest.java
@@ -87,6 +87,16 @@
assertThat(new File(mTestMetricsConfigDir, NAME_BAR).exists()).isFalse();
}
+ @Test
+ public void testDeleteAllMetricsConfigs_shouldDeleteAll() throws Exception {
+ writeConfigToDisk(METRICS_CONFIG_FOO);
+ writeConfigToDisk(METRICS_CONFIG_BAR);
+
+ mMetricsConfigStore.deleteAllMetricsConfigs();
+
+ assertThat(mTestMetricsConfigDir.listFiles()).isEmpty();
+ }
+
private void writeConfigToDisk(TelemetryProto.MetricsConfig config) throws Exception {
File file = new File(mTestMetricsConfigDir, config.getName());
Files.write(file.toPath(), config.toByteArray());
diff --git a/tests/carservice_unit_test/src/com/android/car/telemetry/ResultStoreTest.java b/tests/carservice_unit_test/src/com/android/car/telemetry/ResultStoreTest.java
index af0281a..ed59675 100644
--- a/tests/carservice_unit_test/src/com/android/car/telemetry/ResultStoreTest.java
+++ b/tests/carservice_unit_test/src/com/android/car/telemetry/ResultStoreTest.java
@@ -19,17 +19,11 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
-import static org.mockito.Mockito.eq;
-import static org.mockito.Mockito.verify;
-
import android.os.PersistableBundle;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
-import org.mockito.ArgumentCaptor;
-import org.mockito.Captor;
-import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;
import java.io.ByteArrayOutputStream;
@@ -49,12 +43,6 @@
private File mTestFinalResultDir;
private ResultStore mResultStore;
- @Mock
- private ResultStore.FinalResultCallback mMockFinalResultCallback;
- @Captor
- private ArgumentCaptor<PersistableBundle> mBundleCaptor;
-
-
@Before
public void setUp() throws Exception {
TEST_INTERIM_BUNDLE.putString("test key", "interim value");
@@ -131,11 +119,9 @@
public void testGetFinalResult_whenNoData_shouldReceiveNull() throws Exception {
String metricsConfigName = "my_metrics_config";
- mResultStore.getFinalResult(metricsConfigName, true, mMockFinalResultCallback);
+ PersistableBundle bundle = mResultStore.getFinalResult(metricsConfigName, true);
- verify(mMockFinalResultCallback).onFinalResult(eq(metricsConfigName),
- mBundleCaptor.capture());
- assertThat(mBundleCaptor.getValue()).isNull();
+ assertThat(bundle).isNull();
}
@Test
@@ -144,11 +130,9 @@
Files.write(new File(mTestFinalResultDir, metricsConfigName).toPath(),
"not a bundle".getBytes(StandardCharsets.UTF_8));
- mResultStore.getFinalResult(metricsConfigName, true, mMockFinalResultCallback);
+ PersistableBundle bundle = mResultStore.getFinalResult(metricsConfigName, true);
- verify(mMockFinalResultCallback).onFinalResult(eq(metricsConfigName),
- mBundleCaptor.capture());
- assertThat(mBundleCaptor.getValue()).isNull();
+ assertThat(bundle).isNull();
}
@Test
@@ -156,12 +140,10 @@
String testFinalFileName = "my_metrics_config";
writeBundleToFile(mTestFinalResultDir, testFinalFileName, TEST_FINAL_BUNDLE);
- mResultStore.getFinalResult(testFinalFileName, true, mMockFinalResultCallback);
+ PersistableBundle bundle = mResultStore.getFinalResult(testFinalFileName, true);
- verify(mMockFinalResultCallback).onFinalResult(eq(testFinalFileName),
- mBundleCaptor.capture());
// should compare value instead of reference
- assertThat(mBundleCaptor.getValue().toString()).isEqualTo(TEST_FINAL_BUNDLE.toString());
+ assertThat(bundle.toString()).isEqualTo(TEST_FINAL_BUNDLE.toString());
assertThat(new File(mTestFinalResultDir, testFinalFileName).exists()).isFalse();
}
@@ -170,12 +152,10 @@
String testFinalFileName = "my_metrics_config";
writeBundleToFile(mTestFinalResultDir, testFinalFileName, TEST_FINAL_BUNDLE);
- mResultStore.getFinalResult(testFinalFileName, false, mMockFinalResultCallback);
+ PersistableBundle bundle = mResultStore.getFinalResult(testFinalFileName, false);
- verify(mMockFinalResultCallback).onFinalResult(eq(testFinalFileName),
- mBundleCaptor.capture());
// should compare value instead of reference
- assertThat(mBundleCaptor.getValue().toString()).isEqualTo(TEST_FINAL_BUNDLE.toString());
+ assertThat(bundle.toString()).isEqualTo(TEST_FINAL_BUNDLE.toString());
assertThat(new File(mTestFinalResultDir, testFinalFileName).exists()).isTrue();
}
@@ -236,6 +216,26 @@
assertThat(new File(mTestFinalResultDir, metricsConfigName).exists()).isTrue();
}
+ @Test
+ public void testDeleteResult_whenInterimResult_shouldDelete() throws Exception {
+ String metricsConfigName = "my_metrics_config";
+ writeBundleToFile(mTestInterimResultDir, metricsConfigName, TEST_INTERIM_BUNDLE);
+
+ mResultStore.deleteResult(metricsConfigName);
+
+ assertThat(new File(mTestInterimResultDir, metricsConfigName).exists()).isFalse();
+ }
+
+ @Test
+ public void testDeleteResult_whenFinalResult_shouldDelete() throws Exception {
+ String metricsConfigName = "my_metrics_config";
+ writeBundleToFile(mTestFinalResultDir, metricsConfigName, TEST_FINAL_BUNDLE);
+
+ mResultStore.deleteResult(metricsConfigName);
+
+ assertThat(new File(mTestFinalResultDir, metricsConfigName).exists()).isFalse();
+ }
+
private void writeBundleToFile(
File dir, String fileName, PersistableBundle persistableBundle) throws Exception {
writeBundleToFile(new File(dir, fileName), persistableBundle);