Modified MTRMetrics to vend specific object rather than id type (project-chip#32425)

- MTRMetrics vends MTRMetricData which has well defined schema
- Cleaned up internals of MTRMetricData
- Fixed potential unbounded growth if metrics are enabled on Darwin
- Removed stale metric keys that are not used and were meant to be
  removed in initial framework for metrics

Author: anush-apple

src/darwin/Framework/CHIP/MTRDeviceControllerDelegateBridge.mm

@@ -122,6 +122,10 @@
     id<MTRDeviceControllerDelegate> strongDelegate = mDelegate;
     MTRDeviceController * strongController = mController;
     if (strongDelegate && mQueue && strongController) {
+
+        // Always collect the metrics to avoid unbounded growth of the stats in the collector
+        MTRMetrics * metrics = [[MTRMetricsCollector sharedInstance] metricSnapshot:TRUE];
+
         if ([strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:)] ||
             [strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:metrics:)]) {
             dispatch_async(mQueue, ^{
@@ -133,8 +137,6 @@
 
                 // If the client implements the metrics delegate, prefer that over others
                 if ([strongDelegate respondsToSelector:@selector(controller:commissioningComplete:nodeID:metrics:)]) {
-                    // Create a snapshot and clear for next operation
-                    MTRMetrics * metrics = [[MTRMetricsCollector sharedInstance] metricSnapshot:TRUE];
                     [strongDelegate controller:strongController commissioningComplete:nsError nodeID:nodeID metrics:metrics];
                 } else {
                     [strongDelegate controller:strongController commissioningComplete:nsError nodeID:nodeID];

src/darwin/Framework/CHIP/MTRMetrics.h

@@ -20,6 +20,31 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
+/**
+ * Representation of metric data corresponding to a metric event.
+ */
+MTR_NEWLY_AVAILABLE
+@interface MTRMetricData : NSObject
+
+/**
+ * Value for the metric data. The value may be nil depending on the event emitted.
+ */
+@property (nonatomic, nullable, readonly, copy) NSNumber * value;
+
+/**
+ * Error code for the metric data. This value when valid, holds the error code value
+ * of the operation associated with the event.
+ */
+@property (nonatomic, nullable, readonly, copy) NSNumber * errorCode;
+
+/**
+ * Duration of event associated with the metric. This value may be nil depending on
+ * the event emitted.
+ */
+@property (nonatomic, nullable, readonly, copy) NSNumber * durationMicroseconds;
+
+@end
+
 /**
  * A representation of a collection of metrics data for an operation.
  */
@@ -35,13 +60,13 @@ MTR_NEWLY_AVAILABLE
 @property (nonatomic, readonly, copy) NSArray<NSString *> * allKeys;
 
 /**
- * @brief Returns metric object corresponding to the metric identified by its key
+ * @brief Returns metric data corresponding to the metric identified by its key.
  *
  * @param [in] key Name of the metric
  *
- * @return An object containing the metric data, nil if key is invalid
+ * @return An object containing the metric data, nil if key is invalid.
  */
-- (nullable id)valueForKey:(NSString *)key;
+- (nullable MTRMetricData *)metricDataForKey:(NSString *)key;
 
 @end
 

src/darwin/Framework/CHIP/MTRMetrics.mm

@@ -21,7 +21,7 @@
 #include <Matter/MTRMetrics.h>
 
 @implementation MTRMetrics {
-    NSMutableDictionary<NSString *, id> * _metricsData;
+    NSMutableDictionary<NSString *, MTRMetricData *> * _metricsData;
 }
 
 - (instancetype)init
@@ -42,8 +42,7 @@ - (instancetype)initWithCapacity:(NSUInteger)numItems
 {
     return [_metricsData allKeys];
 }
-
-- (nullable id)valueForKey:(NSString *)key
+- (nullable MTRMetricData *)metricDataForKey:(NSString *)key
 {
     if (!key) {
         MTR_LOG_ERROR("Cannot get metrics value for nil key");
@@ -53,7 +52,7 @@ - (nullable id)valueForKey:(NSString *)key
     return _metricsData[key];
 }
 
-- (void)setValue:(id _Nullable)value forKey:(NSString *)key
+- (void)setMetricData:(MTRMetricData * _Nullable)value forKey:(NSString *)key
 {
     if (!key) {
         MTR_LOG_ERROR("Cannot set metrics value for nil key");
@@ -63,7 +62,7 @@ - (void)setValue:(id _Nullable)value forKey:(NSString *)key
     [_metricsData setValue:value forKey:key];
 }
 
-- (void)removeValueForKey:(NSString *)key
+- (void)removeMetricDataForKey:(NSString *)key
 {
     if (!key) {
         MTR_LOG_ERROR("Cannot remove metrics value for nil key");

src/darwin/Framework/CHIP/MTRMetricsCollector.mm

@@ -18,6 +18,7 @@
 #import "MTRMetricsCollector.h"
 #import "MTRLogging_Internal.h"
 #include "MTRMetrics_Internal.h"
+#include <MTRMetrics.h>
 #import <MTRUnfairLock.h>
 #include <platform/Darwin/Tracing.h>
 #include <system/SystemClock.h>
@@ -26,13 +27,17 @@
 
 using MetricEvent = chip::Tracing::MetricEvent;
 
-static NSString * kMTRMetricDataValueKey = @"value";
-static NSString * kMTRMetricDataTimepointKey = @"time_point";
-static NSString * kMTRMetricDataDurationKey = @"duration_us";
-
-@implementation MTRMetricsData {
+@implementation MTRMetricData {
     chip::System::Clock::Microseconds64 _timePoint;
-    chip::System::Clock::Microseconds64 _duration;
+    MetricEvent::Type _type;
+}
+
+- (instancetype)init
+{
+    // Default is to create data for instant event type.
+    // The key can be anything since it is not really used in this context.
+    MetricEvent event(MetricEvent::Type::kInstantEvent, "");
+    return [self initWithMetricEvent:event];
 }
 
 - (instancetype)initWithMetricEvent:(const MetricEvent &)event
@@ -41,60 +46,50 @@ - (instancetype)initWithMetricEvent:(const MetricEvent &)event
         return nil;
     }
 
+    _type = event.type();
+
+    using EventType = MetricEvent::Type;
+    switch (_type) {
+    // Capture timepoint for begin and end to calculate duration
+    case EventType::kBeginEvent:
+    case EventType::kEndEvent:
+        _timePoint = chip::System::SystemClock().GetMonotonicMicroseconds64();
+        break;
+    case EventType::kInstantEvent:
+        _timePoint = chip::System::Clock::Microseconds64(0);
+        break;
+    }
+
     using ValueType = MetricEvent::Value::Type;
     switch (event.ValueType()) {
     case ValueType::kInt32:
         _value = [NSNumber numberWithInteger:event.ValueInt32()];
         break;
     case ValueType::kUInt32:
-        _value = [NSNumber numberWithInteger:event.ValueUInt32()];
+        _value = [NSNumber numberWithUnsignedInteger:event.ValueUInt32()];
         break;
     case ValueType::kChipErrorCode:
-        _value = [NSNumber numberWithInteger:event.ValueErrorCode()];
+        _errorCode = [NSNumber numberWithUnsignedInteger:event.ValueErrorCode()];
         break;
     case ValueType::kUndefined:
-    default:
-        _value = nil;
+        break;
     }
-
-    _timePoint = chip::System::SystemClock().GetMonotonicMicroseconds64();
-    _duration = chip::System::Clock::Microseconds64(0);
     return self;
 }
 
-- (void)setDurationFromMetricData:(MTRMetricsData *)fromData
-{
-    _duration = _timePoint - fromData->_timePoint;
-}
-
-- (NSNumber *)timePointMicroseconds
+- (void)setDurationFromMetricDataAndClearCounters:(MTRMetricData *)fromData
 {
-    return [NSNumber numberWithUnsignedLongLong:_timePoint.count()];
-}
+    auto duration = _timePoint - fromData->_timePoint;
+    _durationMicroseconds = [NSNumber numberWithUnsignedLongLong:duration.count()];
 
-- (NSNumber *)durationMicroseconds
-{
-    return [NSNumber numberWithUnsignedLongLong:_duration.count()];
+    // Clear timepoints to minimize history
+    _timePoint = fromData->_timePoint = chip::System::Clock::Microseconds64(0);
 }
 
 - (NSString *)description
 {
-    return [NSString stringWithFormat:@"MTRMetricsData: Value = %@, TimePoint = %@, Duration = %@ us", self.value, self.timePointMicroseconds, self.durationMicroseconds];
-}
-
-- (NSDictionary *)toDictionary
-{
-    NSMutableDictionary * dictRepresentation = [NSMutableDictionary dictionary];
-    if (self.value) {
-        [dictRepresentation setValue:self.value forKey:kMTRMetricDataValueKey];
-    }
-    if (auto tmPt = self.timePointMicroseconds) {
-        [dictRepresentation setValue:tmPt forKey:kMTRMetricDataTimepointKey];
-    }
-    if (auto duration = self.durationMicroseconds) {
-        [dictRepresentation setValue:duration forKey:kMTRMetricDataDurationKey];
-    }
-    return dictRepresentation;
+    return [NSString stringWithFormat:@"<MTRMetricData>: Type %d, Value = %@, Error Code = %@, Duration = %@ us",
+                     static_cast<int>(_type), self.value, self.errorCode, self.durationMicroseconds];
 }
 
 @end
@@ -123,8 +118,9 @@ void ShutdownMetricsCollection()
 
 @implementation MTRMetricsCollector {
     os_unfair_lock _lock;
-    NSMutableDictionary<NSString *, MTRMetricsData *> * _metricsDataCollection;
+    NSMutableDictionary<NSString *, MTRMetricData *> * _metricsDataCollection;
     chip::Tracing::signposts::DarwinTracingBackend _tracingBackend;
+    BOOL _tracingBackendRegistered;
 }
 
 + (instancetype)sharedInstance
@@ -152,32 +148,43 @@ - (instancetype)init
     }
     _lock = OS_UNFAIR_LOCK_INIT;
     _metricsDataCollection = [NSMutableDictionary dictionary];
+    _tracingBackendRegistered = FALSE;
     return self;
 }
 
 - (void)registerTracingBackend
 {
     std::lock_guard lock(_lock);
-    chip::Tracing::Register(_tracingBackend);
-    MTR_LOG_INFO("Registered tracing backend with the registry");
+
+    // Register only once
+    if (!_tracingBackendRegistered) {
+        chip::Tracing::Register(_tracingBackend);
+        MTR_LOG_INFO("Registered tracing backend with the registry");
+        _tracingBackendRegistered = TRUE;
+    }
 }
 
 - (void)unregisterTracingBackend
 {
     std::lock_guard lock(_lock);
-    chip::Tracing::Unregister(_tracingBackend);
-    MTR_LOG_INFO("Unregistered tracing backend with the registry");
+
+    // Unregister only if registered before
+    if (_tracingBackendRegistered) {
+        chip::Tracing::Unregister(_tracingBackend);
+        MTR_LOG_INFO("Unregistered tracing backend with the registry");
+        _tracingBackendRegistered = FALSE;
+    }
 }
 
 static inline NSString * suffixNameForMetricType(MetricEvent::Type type)
 {
     switch (type) {
     case MetricEvent::Type::kBeginEvent:
-        return @"-begin";
+        return @"_begin";
     case MetricEvent::Type::kEndEvent:
-        return @"-end";
+        return @"_end";
     case MetricEvent::Type::kInstantEvent:
-        return @"-instant";
+        return @"_instant";
     }
 }
 
@@ -211,14 +218,14 @@ - (void)handleMetricEvent:(MetricEvent)event
 
     // Create the new metric key based event type
     auto metricsKey = [NSString stringWithFormat:@"%s%@", event.key(), suffixNameForMetric(event)];
-    MTRMetricsData * data = [[MTRMetricsData alloc] initWithMetricEvent:event];
+    MTRMetricData * data = [[MTRMetricData alloc] initWithMetricEvent:event];
 
     // If End event, compute its duration using the Begin event
     if (event.type() == MetricEvent::Type::kEndEvent) {
         auto metricsBeginKey = [NSString stringWithFormat:@"%s%@", event.key(), suffixNameForMetricType(MetricEvent::Type::kBeginEvent)];
-        MTRMetricsData * beginMetric = _metricsDataCollection[metricsBeginKey];
+        MTRMetricData * beginMetric = _metricsDataCollection[metricsBeginKey];
         if (beginMetric) {
-            [data setDurationFromMetricData:beginMetric];
+            [data setDurationFromMetricDataAndClearCounters:beginMetric];
         } else {
             // Unbalanced end
             MTR_LOG_ERROR("Unable to find Begin event corresponding to Metric Event: %s", event.key());
@@ -230,7 +237,7 @@ - (void)handleMetricEvent:(MetricEvent)event
     // If the event is a begin or end event, implicitly emit a corresponding instant event
     if (event.type() == MetricEvent::Type::kBeginEvent || event.type() == MetricEvent::Type::kEndEvent) {
         MetricEvent instantEvent(MetricEvent::Type::kInstantEvent, event.key());
-        data = [[MTRMetricsData alloc] initWithMetricEvent:instantEvent];
+        data = [[MTRMetricData alloc] initWithMetricEvent:instantEvent];
         metricsKey = [NSString stringWithFormat:@"%s%@", event.key(), suffixNameForMetric(instantEvent)];
         [_metricsDataCollection setValue:data forKey:metricsKey];
     }
@@ -240,10 +247,9 @@ - (MTRMetrics *)metricSnapshot:(BOOL)resetCollection
 {
     std::lock_guard lock(_lock);
 
-    // Copy the MTRMetrics as NSDictionary
     MTRMetrics * metrics = [[MTRMetrics alloc] initWithCapacity:[_metricsDataCollection count]];
     for (NSString * key in _metricsDataCollection) {
-        [metrics setValue:[_metricsDataCollection[key] toDictionary] forKey:key];
+        [metrics setMetricData:_metricsDataCollection[key] forKey:key];
     }
 
     // Clear curent stats, if specified

src/darwin/Framework/CHIP/MTRMetrics_Internal.h

@@ -19,32 +19,13 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-/**
- * A representation of a metric data for an operation.
- */
-@interface MTRMetricsData : NSObject
-
-// Value for the metric. This can be null if the metric is just a fire event with no value
-@property (nonatomic, nullable, readonly, copy) NSNumber * value;
-
-// Relative time point at which the metric was emitted. This may be null.
-@property (nonatomic, nullable, readonly, copy) NSNumber * timePointMicroseconds;
-
-// During for the event. This may be null.
-@property (nonatomic, nullable, readonly, copy) NSNumber * durationMicroseconds;
-
-// Convert contents to a dictionary
-- (NSDictionary *)toDictionary;
-
-@end
-
 @interface MTRMetrics ()
 
 - (instancetype)initWithCapacity:(NSUInteger)numItems;
 
-- (void)setValue:(id _Nullable)value forKey:(NSString *)key;
+- (void)setMetricData:(MTRMetricData * _Nullable)value forKey:(NSString *)key;
 
-- (void)removeValueForKey:(NSString *)key;
+- (void)removeMetricDataForKey:(NSString *)key;
 
 @end