[RESTRICT AUTOMERGE] Send metrics config removal request to DataBroker
Currently config removal is only propagated to ResultStore and
MetricsConfigStore. This CL will remove config from DataBroker.
This CL also deletes the async return value of the removeMetricsConfig
API to make remove and removeAll APIs consistent.
Bug: 199540952
Test: atest CarServiceUnitTest:CarTelemetryServiceTest
Test: atest CarServiceUnitTest:DataBrokerTest
Test: atest CarServiceUnitTest:MetricsConfigStoreTest
Test: atest CarServiceUnitTest:ResultStoreTest
Test: atest CarSecurityPermissionTest:CarTelemetryManagerPermissionTest
Change-Id: I949d9571588a44455c04c19c5b1b2524a92b6663
diff --git a/service/src/com/android/car/telemetry/CarTelemetryService.java b/service/src/com/android/car/telemetry/CarTelemetryService.java
index f48d8d8..6f5ca1a 100644
--- a/service/src/com/android/car/telemetry/CarTelemetryService.java
+++ b/service/src/com/android/car/telemetry/CarTelemetryService.java
@@ -182,7 +182,7 @@
// 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());
+ mResultStore.removeResult(key.getName());
}
try {
mListener.onAddMetricsConfigStatus(key, status);
@@ -194,29 +194,21 @@
/**
* Removes a metrics config based on the key. This will also remove outputs produced by the
- * MetricsConfig. This method assumes {@link #setListener(ICarTelemetryServiceListener)} is
- * called. Otherwise it does nothing.
+ * MetricsConfig.
*
* @param key the unique identifier of a MetricsConfig.
*/
@Override
public void removeMetricsConfig(@NonNull MetricsConfigKey key) {
+ mContext.enforceCallingOrSelfPermission(
+ Car.PERMISSION_USE_CAR_TELEMETRY_SERVICE, "removeMetricsConfig");
mTelemetryHandler.post(() -> {
- if (mListener == null) {
- Slog.w(CarLog.TAG_TELEMETRY, "ICarTelemetryServiceListener is not set");
- return;
- }
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);
- } catch (RemoteException e) {
- Slog.w(CarLog.TAG_TELEMETRY, "error with ICarTelemetryServiceListener", e);
- }
+ // TODO(b/198792767): Check both config name and config version for removal
+ mDataBroker.removeMetricsConfiguration(key.getName());
+ mResultStore.removeResult(key.getName());
+ mMetricsConfigStore.removeMetricsConfig(key.getName());
});
}
@@ -226,13 +218,13 @@
@Override
public void removeAllMetricsConfigs() {
mContext.enforceCallingOrSelfPermission(
- Car.PERMISSION_USE_CAR_TELEMETRY_SERVICE, "removeAllMetricsConfig");
+ Car.PERMISSION_USE_CAR_TELEMETRY_SERVICE, "removeAllMetricsConfigs");
mTelemetryHandler.post(() -> {
- // 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();
+ mDataBroker.removeAllMetricsConfigurations();
+ mMetricsConfigStore.removeAllMetricsConfigs();
+ mResultStore.removeAllResults();
});
}
diff --git a/service/src/com/android/car/telemetry/MetricsConfigStore.java b/service/src/com/android/car/telemetry/MetricsConfigStore.java
index ff17d0e..2d259d4 100644
--- a/service/src/com/android/car/telemetry/MetricsConfigStore.java
+++ b/service/src/com/android/car/telemetry/MetricsConfigStore.java
@@ -104,17 +104,21 @@
}
/** Deletes the MetricsConfig from disk. Returns the success status. */
- public boolean deleteMetricsConfig(String metricsConfigName) {
+ public void removeMetricsConfig(String metricsConfigName) {
mActiveConfigs.remove(metricsConfigName);
mNameVersionMap.remove(metricsConfigName);
- return new File(mConfigDirectory, metricsConfigName).delete();
+ if (!new File(mConfigDirectory, metricsConfigName).delete()) {
+ Slog.w(CarLog.TAG_TELEMETRY, "Failed to remove MetricsConfig: " + metricsConfigName);
+ }
}
/** Deletes all MetricsConfigs from disk. */
- public void deleteAllMetricsConfigs() {
+ public void removeAllMetricsConfigs() {
mActiveConfigs.clear();
for (File file : mConfigDirectory.listFiles()) {
- file.delete();
+ if (!file.delete()) {
+ Slog.w(CarLog.TAG_TELEMETRY, "Failed to remove MetricsConfig: " + file.getName());
+ }
}
}
}
diff --git a/service/src/com/android/car/telemetry/ResultStore.java b/service/src/com/android/car/telemetry/ResultStore.java
index 9c8b2fe..85aa23e 100644
--- a/service/src/com/android/car/telemetry/ResultStore.java
+++ b/service/src/com/android/car/telemetry/ResultStore.java
@@ -179,14 +179,14 @@
* 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) {
+ public void removeResult(String metricsConfigName) {
mInterimResultCache.remove(metricsConfigName);
deleteFileInDirectory(mInterimResultDirectory, metricsConfigName);
deleteFileInDirectory(mFinalResultDirectory, metricsConfigName);
}
/** Deletes all interim and final results stored in disk. */
- public void deleteAllResults() {
+ public void removeAllResults() {
mInterimResultCache.clear();
for (File interimResult : mInterimResultDirectory.listFiles()) {
interimResult.delete();
diff --git a/service/src/com/android/car/telemetry/databroker/DataBroker.java b/service/src/com/android/car/telemetry/databroker/DataBroker.java
index d7cc2e6..2b7fa01 100644
--- a/service/src/com/android/car/telemetry/databroker/DataBroker.java
+++ b/service/src/com/android/car/telemetry/databroker/DataBroker.java
@@ -36,8 +36,7 @@
/**
* Adds an active {@link com.android.car.telemetry.TelemetryProto.MetricsConfig} that is pending
* execution. When updating the MetricsConfig to a newer version, the caller must call
- * {@link #removeMetricsConfiguration(TelemetryProto.MetricsConfig)} first to clear the old
- * MetricsConfig.
+ * {@link #removeMetricsConfiguration(String)} first to clear the old MetricsConfig.
* TODO(b/191378559): Define behavior when metricsConfig contains invalid config
*
* @param metricsConfig to be added and queued for execution.
@@ -48,9 +47,15 @@
* Removes a {@link com.android.car.telemetry.TelemetryProto.MetricsConfig} and all its
* relevant subscriptions.
*
- * @param metricsConfig to be removed from DataBroker.
+ * @param metricsConfigName name of the MetricsConfig to be removed.
*/
- void removeMetricsConfiguration(TelemetryProto.MetricsConfig metricsConfig);
+ void removeMetricsConfiguration(String metricsConfigName);
+
+ /**
+ * Removes all {@link com.android.car.telemetry.TelemetryProto.MetricsConfig}s and
+ * subscriptions.
+ */
+ void removeAllMetricsConfigurations();
/**
* Adds a {@link ScriptExecutionTask} to the priority queue. This method will schedule the
diff --git a/service/src/com/android/car/telemetry/databroker/DataBrokerImpl.java b/service/src/com/android/car/telemetry/databroker/DataBrokerImpl.java
index 695254c..b53899b 100644
--- a/service/src/com/android/car/telemetry/databroker/DataBrokerImpl.java
+++ b/service/src/com/android/car/telemetry/databroker/DataBrokerImpl.java
@@ -56,7 +56,7 @@
/**
* Implementation of the data path component of CarTelemetryService. Forwards the published data
* from publishers to consumers subject to the Controller's decision.
- * TODO(b/187743369): Handle thread-safety of member variables.
+ * All methods should be called from the telemetry thread unless otherwise specified as thread-safe.
*/
public class DataBrokerImpl implements DataBroker {
@@ -208,7 +208,7 @@
// get the metrics config from the DataSubscriber and remove the metrics config
if (mSubscriptionMap.get(metricsConfigName).size() != 0) {
removeMetricsConfiguration(mSubscriptionMap.get(metricsConfigName).get(0)
- .getMetricsConfig());
+ .getMetricsConfig().getName());
}
}
mSubscriptionMap.clear();
@@ -248,13 +248,13 @@
}
@Override
- public void removeMetricsConfiguration(MetricsConfig metricsConfig) {
+ public void removeMetricsConfiguration(String metricsConfigName) {
// TODO(b/187743369): pass status back to caller
- if (!mSubscriptionMap.containsKey(metricsConfig.getName())) {
+ if (!mSubscriptionMap.containsKey(metricsConfigName)) {
return;
}
// get the subscriptions associated with this MetricsConfig, remove it from the map
- List<DataSubscriber> dataSubscribers = mSubscriptionMap.remove(metricsConfig.getName());
+ List<DataSubscriber> dataSubscribers = mSubscriptionMap.remove(metricsConfigName);
// for each subscriber, remove it from publishers
for (DataSubscriber subscriber : dataSubscribers) {
AbstractPublisher publisher = mPublisherFactory.getPublisher(
@@ -271,7 +271,14 @@
// iterating, so it may or may not reflect any updates since the iterator was created.
// But since adding & polling from queue should happen in the same thread, the task queue
// should not be changed while tasks are being iterated and removed.
- mTaskQueue.removeIf(task -> task.isAssociatedWithMetricsConfig(metricsConfig));
+ mTaskQueue.removeIf(task -> task.isAssociatedWithMetricsConfig(metricsConfigName));
+ }
+
+ @Override
+ public void removeAllMetricsConfigurations() {
+ mPublisherFactory.removeAllDataSubscribers();
+ mSubscriptionMap.clear();
+ mTaskQueue.clear();
}
@Override
diff --git a/service/src/com/android/car/telemetry/databroker/ScriptExecutionTask.java b/service/src/com/android/car/telemetry/databroker/ScriptExecutionTask.java
index 648fc79..afe050a 100644
--- a/service/src/com/android/car/telemetry/databroker/ScriptExecutionTask.java
+++ b/service/src/com/android/car/telemetry/databroker/ScriptExecutionTask.java
@@ -63,11 +63,10 @@
}
/**
- * Indicates whether the task is associated with the given
- * {@link com.android.car.telemetry.TelemetryProto.MetricsConfig).
+ * Indicates whether the task is associated with MetricsConfig specified by the name.
*/
- public boolean isAssociatedWithMetricsConfig(TelemetryProto.MetricsConfig metricsConfig) {
- return mSubscriber.getMetricsConfig().equals(metricsConfig);
+ public boolean isAssociatedWithMetricsConfig(String metricsConfigName) {
+ return mSubscriber.getMetricsConfig().getName().equals(metricsConfigName);
}
/**
diff --git a/service/src/com/android/car/telemetry/publisher/PublisherFactory.java b/service/src/com/android/car/telemetry/publisher/PublisherFactory.java
index 2e08191..d777847 100644
--- a/service/src/com/android/car/telemetry/publisher/PublisherFactory.java
+++ b/service/src/com/android/car/telemetry/publisher/PublisherFactory.java
@@ -31,7 +31,7 @@
* <p>It doesn't instantiate all the publishers right away, as in some cases some publishers are
* not needed.
*
- * <p>Thread-safe.
+ * <p>Methods in this class must be called on telemetry thread unless specified as thread-safe.
*/
public class PublisherFactory {
private final Object mLock = new Object();
@@ -56,7 +56,7 @@
mRootDirectory = rootDirectory;
}
- /** Returns the publisher by given type. */
+ /** Returns the publisher by given type. This method is thread-safe. */
public AbstractPublisher getPublisher(TelemetryProto.Publisher.PublisherCase type) {
// No need to optimize locks, as this method is infrequently called.
synchronized (mLock) {
@@ -87,6 +87,21 @@
}
/**
+ * Removes all {@link com.android.car.telemetry.databroker.DataSubscriber} from all publishers.
+ */
+ public void removeAllDataSubscribers() {
+ if (mVehiclePropertyPublisher != null) {
+ mVehiclePropertyPublisher.removeAllDataSubscribers();
+ }
+ if (mCarTelemetrydPublisher != null) {
+ mCarTelemetrydPublisher.removeAllDataSubscribers();
+ }
+ if (mStatsPublisher != null) {
+ mStatsPublisher.removeAllDataSubscribers();
+ }
+ }
+
+ /**
* Sets the publisher failure consumer for all the publishers. This is expected to be called
* before {@link #getPublisher} method. This is not the best approach, but it suits for this
* case.